Message-ID: <9294a556-345e-4766-9b54-1a429bfb917e@lucifer.local>
Date: Wed, 16 Oct 2024 21:00:03 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Christian Brauner <christian@...uner.io>, Shuah Khan <shuah@...nel.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Vlastimil Babka <vbabka@...e.cz>, pedro.falcato@...il.com,
        linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate
 pid lookup

On Wed, Oct 16, 2024 at 03:00:55PM +0200, Christian Brauner wrote:
> On Fri, Oct 11, 2024 at 12:05:55PM +0100, Lorenzo Stoakes wrote:
> > The means by which a pid is determined from a pidfd is duplicated, with
> > some callers holding a reference to the (pid)fd, and others explicitly
> > pinning the pid.
> >
> > Introduce __pidfd_get_pid() which abstracts both approaches and provide
> > optional output parameters for file->f_flags and the fd (the latter of
> > which, if provided, prevents the function from decrementing the fd's
> > refernce count).
> >
> > Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
> > directory, utilised by the pidfd_send_signal() system call, providing a
> > pidfd_get_pid_proc() helper function to do so.
> >
> > Doing this allows us to eliminate open-coded pidfd pid lookup and to
> > consistently handle this in one place.
> >
> > This lays the groundwork for a subsequent patch which adds a new sentinel
> > pidfd to explicitly reference the current process (i.e. thread group
> > leader) without the need for a pidfd.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > ---
> >  include/linux/pid.h | 42 +++++++++++++++++++++++++++++++-
> >  kernel/pid.c        | 58 ++++++++++++++++++++++++++++++---------------
> >  kernel/signal.c     | 22 ++++-------------
> >  3 files changed, 84 insertions(+), 38 deletions(-)
> >
> > diff --git a/include/linux/pid.h b/include/linux/pid.h
> > index a3aad9b4074c..68b02eab7509 100644
> > --- a/include/linux/pid.h
> > +++ b/include/linux/pid.h
> > @@ -2,6 +2,7 @@
> >  #ifndef _LINUX_PID_H
> >  #define _LINUX_PID_H
> >
> > +#include <linux/file.h>
> >  #include <linux/pid_types.h>
> >  #include <linux/rculist.h>
> >  #include <linux/rcupdate.h>
> > @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
> >
> >  struct file;
> >
> > +
> > +/**
> > + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> > + *
> > + * @pidfd:      The pidfd whose pid we want, or the fd of a /proc/<pid> file if
> > + *              @alloc_proc is also set.
> > + * @pin_pid:    If set, then the reference counter of the returned pid is
> > + *              incremented. If not set, then @fd should be provided to pin the
> > + *              pidfd.
> > + * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
> > + *              of a pidfd, and this will be used to determine the pid.
> > + * @flags:      Output variable, if non-NULL, then the file->f_flags of the
> > + *              pidfd will be set here.
> > + * @fd:         Output variable, if non-NULL, then the pidfd reference will
> > + *              remain elevated and the caller will need to decrement it
> > + *              themselves.
> > + *
> > + * Returns: If successful, the pid associated with the pidfd, otherwise an
> > + *          error.
> > + */
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > +			    bool allow_proc, unsigned int *flags,
> > +			    struct fd *fd);
> > +
> > +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
> > +{
> > +	return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> > +			       /* allow_proc = */ false,
> > +			       flags, /* fd = */ NULL);
> > +}
> > +
> > +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> > +					    unsigned int *flags,
> > +					    struct fd *fd)
> > +{
> > +	return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> > +			       /* allow_proc = */ true,
> > +			       flags, fd);
> > +}
> > +
> >  struct pid *pidfd_pid(const struct file *file);
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> >  struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> >  int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> >  void do_notify_pidfd(struct task_struct *task);
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 2715afb77eab..25cc1c36a1b1 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -36,6 +36,7 @@
> >  #include <linux/pid_namespace.h>
> >  #include <linux/init_task.h>
> >  #include <linux/syscalls.h>
> > +#include <linux/proc_fs.h>
> >  #include <linux/proc_ns.h>
> >  #include <linux/refcount.h>
> >  #include <linux/anon_inodes.h>
> > @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> >  }
> >  EXPORT_SYMBOL_GPL(find_ge_pid);
> >
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > +			    bool allow_proc, unsigned int *flags,
> > +			    struct fd *fd)
>
> Hm, we should never return a struct fd. A struct fd is an inherently
> scoped-bound concept - or at least aims to be. Simply put, we always
> want to have the fdget() and the fdput() in the same scope as the file
> pointer you can access via fd_file() is only valid as long as we're in
> the syscall.
>
> Ideally we mostly use CLASS(fd/fd_raw) and nearly never fdget(). The
> point is that this is the wrong api to expose.
>
> It would probably be wiser if you added a pidfd based fdget() inspired
> primitive.

I think we can actually probably just avoid passing it back and pin the pid
instead of the fd, which keeps the scope as before and simplifies things
generally.

Let me experiment with that!

Powered by blists - more mailing lists