Re: [GIT PULL] pidfd updates

[Linus Torvalds]

On Fri, Apr 21, 2023 at 6:42 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> This adds a new pidfd_prepare() helper which allows the caller to
> reserve a pidfd number and allocates a new pidfd file that stashes the
> provided struct pid.

So I've pulled this, but I have to say that I think the "cleanup" part
is pretty nasty - and that's also visible in the interface.

In particular, pidfd_prepare() ends up returning two things, so you
have that ugly "return by reference of the third arguments", but you
also end up not being able to have one single cleanup, you have to
(continue) to do a pair of them:

    put_unused_fd(pidfd);
    fput(pidfd_file);

and I really think the old model of just having a single return value,
and doing a single "close()" was a much nicer in many ways.

Now, the reason I pulled is that I agree that actually making the fd
*visible* to user space is a big mistake.

But I really think a potentially much nicer model would have been to
extend our "get_unused_fd_flags()" model.

IOW, we could have instead marked the 'struct file *' in the file
descriptor table as being "not ready yet".

I wonder how nasty it would have been to have the low bit of the
'struct file *' mark "not ready to be used yet" or something similar.
You already can't just access the 'fdt->fd[]' array willy-nilly since
we have both normal RCU issues _and_ the somewhat unusual spectre
array indexing issues.

So looking around with

    git grep -e '->fd\['

we seem to be pretty good about that and it probably wouldn't be too
horrid to add a "check low bit isn't set" to the rules.

Then pidfd_prepare() could actually install the file pointer in the fd
table, just marked as "not ready", and then instead of "fd_install()",
yuo'd have "fd_expose(fd)" or something.

I dislike interfaces that return two different things. Particularly
ones that are supposed to be there to make things easy for the user. I
think your pidfd_prepare() helper fails that "make it easy to use"
test.

Hmm?

Anyway, I think it's an improvement even so, but I wanted to just say
that I think this could maybe have been done with a nicer model.

               Linus