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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

Bart