Re: [PATCH 2/2] gpiolib: cdev: export the consumer's PID

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, Sep 12, 2022 at 10:52:53AM +0200, Bartosz Golaszewski wrote:
> On Sat, Sep 10, 2022 at 4:52 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
> >
> > On Fri, Sep 09, 2022 at 02:13:29PM +0200, Bartosz Golaszewski wrote:
> > > It's useful for user-space to be able to know the PIDs of processes
> > > holding GPIO lines in case they have the permissions and need to kill
> > > them.
> > >
> >
> > "the PID of the process holding a GPIO line"
> >
> > As written it could be interpreted to imply that multiple processes can
> > hold a line, so go singular to remove that possibility.
> >
> > > Extend the gpio_v2_line_info structure with the consumer_pid field
> > > that's set to the PID of the user-space process or 0 if the user lives
> > > in the kernel.
> > >
> > > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c |  2 ++
> > >  drivers/gpio/gpiolib.c      | 24 +++++++++++++++++++-----
> > >  drivers/gpio/gpiolib.h      |  2 ++
> > >  include/uapi/linux/gpio.h   |  5 ++++-
> > >  4 files changed, 27 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index f8041d4898d1..9b6d518680dc 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -2120,6 +2120,8 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> > >       if (desc->label)
> > >               strscpy(info->consumer, desc->label, sizeof(info->consumer));
> > >
> > > +     info->consumer_pid = desc->pid;
> > > +
> > >       /*
> > >        * Userspace only need to know that the kernel is using this GPIO so
> > >        * it can't use it.
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 6768734b9e15..0c9d1639b04d 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -96,6 +96,11 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
> > >       d->label = label;
> > >  }
> > >
> > > +static inline void desc_set_pid(struct gpio_desc *d, pid_t pid)
> > > +{
> > > +     d->pid = pid;
> > > +}
> > > +
> > >  /**
> > >   * gpio_to_desc - Convert a GPIO number to its descriptor
> > >   * @gpio: global GPIO number
> > > @@ -1892,7 +1897,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
> > >   * on each other, and help provide better diagnostics in debugfs.
> > >   * They're called even less than the "set direction" calls.
> > >   */
> > > -static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > > +static int
> > > +gpiod_request_commit(struct gpio_desc *desc, const char *label, pid_t pid)
> > >  {
> > >       struct gpio_chip        *gc = desc->gdev->chip;
> > >       int                     ret;
> > > @@ -1913,6 +1919,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > >
> > >       if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > >               desc_set_label(desc, label ? : "?");
> > > +             desc_set_pid(desc, pid);
> > >       } else {
> > >               ret = -EBUSY;
> > >               goto out_free_unlock;
> > > @@ -1987,7 +1994,8 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
> > >               return; \
> > >       } while (0)
> > >
> > > -int gpiod_request(struct gpio_desc *desc, const char *label)
> > > +static int
> > > +gpiod_request_with_pid(struct gpio_desc *desc, const char *label, pid_t pid)
> > >  {
> > >       int ret = -EPROBE_DEFER;
> > >       struct gpio_device *gdev;
> > > @@ -1996,7 +2004,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
> > >       gdev = desc->gdev;
> > >
> > >       if (try_module_get(gdev->owner)) {
> > > -             ret = gpiod_request_commit(desc, label);
> > > +             ret = gpiod_request_commit(desc, label, pid);
> > >               if (ret)
> > >                       module_put(gdev->owner);
> > >               else
> > > @@ -2009,11 +2017,16 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
> > >       return ret;
> > >  }
> > >
> > > +int gpiod_request(struct gpio_desc *desc, const char *label)
> > > +{
> > > +     return gpiod_request_with_pid(desc, label, 0);
> > > +}
> > > +
> > >  int gpiod_request_user(struct gpio_desc *desc, const char *label)
> > >  {
> > >       int ret;
> > >
> > > -     ret = gpiod_request(desc, label);
> > > +     ret = gpiod_request_with_pid(desc, label, current->pid);
> > >       if (ret == -EPROBE_DEFER)
> > >               ret = -ENODEV;
> > >
> > > @@ -2042,6 +2055,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> > >               }
> > >               kfree_const(desc->label);
> > >               desc_set_label(desc, NULL);
> > > +             desc_set_pid(desc, 0);
> > >               clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > >               clear_bit(FLAG_REQUESTED, &desc->flags);
> > >               clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > > @@ -2140,7 +2154,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
> > >               return desc;
> > >       }
> > >
> > > -     ret = gpiod_request_commit(desc, label);
> > > +     ret = gpiod_request_commit(desc, label, 0);
> > >       if (ret < 0)
> > >               return ERR_PTR(ret);
> > >
> > > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> > > index b35deb08a7f5..d1535677e162 100644
> > > --- a/drivers/gpio/gpiolib.h
> > > +++ b/drivers/gpio/gpiolib.h
> > > @@ -165,6 +165,8 @@ struct gpio_desc {
> > >
> > >       /* Connection label */
> > >       const char              *label;
> > > +     /* Consumer's PID (if consumer is in user-space, otherwise 0) */
> > > +     pid_t                   pid;
> > >       /* Name of the GPIO */
> > >       const char              *name;
> > >  #ifdef CONFIG_OF_DYNAMIC
> > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> > > index cb9966d49a16..37f10021d1aa 100644
> > > --- a/include/uapi/linux/gpio.h
> > > +++ b/include/uapi/linux/gpio.h
> > > @@ -219,6 +219,8 @@ struct gpio_v2_line_request {
> > >   * gpio_v2_line_flag, such as %GPIO_V2_LINE_FLAG_ACTIVE_LOW,
> > >   * %GPIO_V2_LINE_FLAG_OUTPUT etc, added together.
> > >   * @attrs: the configuration attributes associated with the line
> > > + * @consumer_pid: process ID of the user-space consumer or 0 if the consumer
> > > + * lives in kernel space
> > >   * @padding: reserved for future use
> > >   */
> > >  struct gpio_v2_line_info {
> > > @@ -228,8 +230,9 @@ struct gpio_v2_line_info {
> > >       __u32 num_attrs;
> > >       __aligned_u64 flags;
> > >       struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
> > > +     __s32 consumer_pid;
> >
> > My knee-jerk reaction here was to make the pid unsigned, as we never
> > pass a negative PID.
> > Keeping in mind that the existing kernel will return 0 for this field
> > (the existing padding), so 0 needs to be excluded from valid PIDs
> > anyway.
> >
> > Andy suggests returning -1 for kernel held lines.
> > In that case 0 would mean "old kernel", while -1 would mean "kernel
> > held".
> >
> > As libgpiod will have to convert the 0 to -1 when returning the PID to
> > user-space as a pid_t, I'm good with the uAPI using 0 to mean
> > "no PID available" for all cases. I'm still open to passing -1 for
> > kernel held is there is a use case for it, but I don't see one.
> >
> 
> Using -1 sounds good but I've just realized there's a different
> problem. A process holding a file descriptor may fork and both the
> parent and the child will keep the same file descriptors open. Now
> we'll have two processes (with different PIDs) holding the same GPIO
> lines (specifically holding a file descriptor to the same anonymous
> inode).
> 
> This already poses a problem for this patch as we'd need to return an
> array of PIDs which we don't have the space for but also is a
> situation which we haven't discussed previously IIRC - two processes
> keeping the same GPIO lines requested.
> 
> I don't have any good idea on how to address this yet. One thing off
> the top of my head is: close the parent's file descriptor from kernel
> space (is it even possible?) on fork() (kind of like the close() on
> exec flag).
> 
> I need to think about it more.
> 

I thought the O_CLOEXEC was set on the request fds exactly to prevent this
case - only one process can hold the request fd.

Cheers,
Kent.