Sorry to keep pestering about the patch series, but with the addition of P_PIDFD, I react once again..
On Sat, Jul 27, 2019 at 1:53 AM Christian Brauner <[email protected]> wrote: > > --- a/kernel/exit.c > +++ b/kernel/exit.c > @@ -1555,6 +1555,7 @@ static long do_wait(struct wait_opts *wo) > static long kernel_waitid(int which, pid_t upid, struct waitid_info *infop, > int options, struct rusage *ru) > { > + struct fd f; Please don't do 'struct fd' at this level. That results in this ugly code later: > - put_pid(pid); > + if (which == P_PIDFD) > + fdput(f); > + else > + put_pid(pid); which just looks nasty. Instead, do all the 'file descriptor to pid' games here: > + case P_PIDFD: > + type = PIDTYPE_PID; > + if (upid < 0) > + return -EINVAL; > + > + f = fdget(upid); > + if (!f.file) > + return -EBADF; > + > + pid = pidfd_pid(f.file); > + if (IS_ERR(pid)) { > + fdput(f); > + return PTR_ERR(pid); > + } > break; and make thus just do something like pid = get_pid_from_fd(upid); if (IS_ERR(pid)) return PTR_ERR(pid); and now do that "fd to pid" in that helper function, and get the reference to 'struct pid *' there instead. Which you can actually do efficiently and lightly without even getting a ref to the 'struct file'. Something like struct pid *fd_to_pid(unsigned int fd) { struct fd f; struct pid *pid; f = fdget(fd); if (!f.file) return ERR_PTR(-EBADF); pid = pidfd_pid(f.file); if (!IS_ERR(pid)) get_pid(pid); fdput(f); return pid; } is the stupid and straightforward thing, but if you want to be *clever* you can actually avoid getting a reference to the 'struct file *" entirely, and do the fd->pid lookup under rcu_read_lock() instead. It's slightly more complex, but it avoids the fdget/fdput reference count games entirely. And then all that kernel_waitid() ever worries about is "struct pid *", and the ending goes back to just that simple put_pid(pid); return ret; instead. This was kind of my point of doing all the "find_get_pid()" games in the "switch()" statement - the different cases have different ways to look up what the "struct pid *" pointer should be, but they should all just look up a pid pointer, and then nothing else needs to care about 'type' any more. See? Hmm? Linus
