lkml.org 
[lkml]   [2019]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 1/2] pidfd: add P_PIDFD to waitid()
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 <christian@brauner.io> 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

\
 
 \ /
  Last update: 2019-07-27 18:42    [W:0.043 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site