| Message ID | 20220909121329.42004-3-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: export the consumer's PID to user-space | expand |
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. > > 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. ... > +int gpiod_request(struct gpio_desc *desc, const char *label) > +{ > + return gpiod_request_with_pid(desc, label, 0); Why not -1? I would expect this is the usual way of telling "don't use this PID". > +} ... > + desc_set_pid(desc, 0); Ditto. ... > + ret = gpiod_request_commit(desc, label, 0); Ditto.
On Fri, Sep 9, 2022 at 3:48 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > +Cc: Kees > > 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. > > > > 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. > > I'm wondering if there is any security implications and this PID > can be abused. > I was wondering about that too but nothing came to my mind. By default any user - even one who doesn't have permissions to access /dev/gpiochip* - can already figure out by browsing /proc/$PID/fd that a process does have some lines requested - but not which exactly. This provides that additional bit of knowledge to users who already do have permissions to call ioctl() on /dev/gpiochip*. Bart
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@bgdev.pl> > --- > 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. Cheers, Kent. > /* Space reserved for future use. */ > - __u32 padding[4]; > + __u32 padding[3]; > }; > > /** > -- > 2.34.1 >
On Sat, Sep 10, 2022 at 4:52 PM Kent Gibson <warthog618@gmail.com> 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@bgdev.pl> > > --- > > 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
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@gmail.com> 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@bgdev.pl> > > > --- > > > 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.
On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > [snip] > > > > > > 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. > O_CLOEXEC means "close on exec" not "close on fork". When you fork, you inherit all file descriptors from your parent. Only once you call execve() are the fds with this flag closed *in the child*. Bart
On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip] > > > > > > > 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. > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > you inherit all file descriptors from your parent. Only once you call > execve() are the fds with this flag closed *in the child*. > Ah, ok. You want to pass request fd ownership from parent to child?? Why not lock ownership to the parent, so O_CLOFORK, were that available? Cheers, Kent.
On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > [snip] > > > > > > > > > > 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. > > > > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > > you inherit all file descriptors from your parent. Only once you call > > execve() are the fds with this flag closed *in the child*. > > > > Ah, ok. > You want to pass request fd ownership from parent to child?? > Why not lock ownership to the parent, so O_CLOFORK, were that > available? > Because what if we want to request a line and then daemonize i.e. fork and exit in parent? It makes much more sense to keep the lines requested in the child IMO. During the BoF at Linux Plumbers it was suggested to use /proc/$PID/fdinfo to expose the information about which lines are requested but I can't figure out a way to do it elegantly. Bart
On Tue, Sep 13, 2022 at 10:54:26AM +0200, Bartosz Golaszewski wrote: > On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > [snip] > > > > > > > > > > > > > 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. > > > > > > > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > > > you inherit all file descriptors from your parent. Only once you call > > > execve() are the fds with this flag closed *in the child*. > > > > > > > Ah, ok. > > You want to pass request fd ownership from parent to child?? > > Why not lock ownership to the parent, so O_CLOFORK, were that > > available? > > > > Because what if we want to request a line and then daemonize i.e. fork > and exit in parent? It makes much more sense to keep the lines > requested in the child IMO. > Then you are doing it backwards - daemonize first ;-). Generally speaking, doesn't transfer of resource ownership to the forked child create havoc in multi-threaded apps? i.e. one thread requests a resource, another forks. The parent thread unknowingly loses ownership, and the forked child process only starts with a replica of the forking thread. > During the BoF at Linux Plumbers it was suggested to use > /proc/$PID/fdinfo to expose the information about which lines are > requested but I can't figure out a way to do it elegantly. > Yeah, missed that :-(. Makes sense. As each request fd can contain multiple lines on a particular chip, you would need to identify the gpiochip and the offsets for that request. So two fields - the gpiochip path, and the list of offsets. Is that already too clunky or am I missing something? Cheers, Kent.
On Tue, Sep 13, 2022 at 4:28 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Sep 13, 2022 at 10:54:26AM +0200, Bartosz Golaszewski wrote: > > On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > > > > you inherit all file descriptors from your parent. Only once you call > > > > execve() are the fds with this flag closed *in the child*. > > > > > > > > > > Ah, ok. > > > You want to pass request fd ownership from parent to child?? > > > Why not lock ownership to the parent, so O_CLOFORK, were that > > > available? > > > > > > > Because what if we want to request a line and then daemonize i.e. fork > > and exit in parent? It makes much more sense to keep the lines > > requested in the child IMO. > > > > Then you are doing it backwards - daemonize first ;-). > > Generally speaking, doesn't transfer of resource ownership to the forked > child create havoc in multi-threaded apps? i.e. one thread requests a > resource, another forks. The parent thread unknowingly loses ownership, > and the forked child process only starts with a replica of the forking > thread. > Yeah, sounds like a bad idea. > > During the BoF at Linux Plumbers it was suggested to use > > /proc/$PID/fdinfo to expose the information about which lines are > > requested but I can't figure out a way to do it elegantly. > > > > Yeah, missed that :-(. > > Makes sense. > > As each request fd can contain multiple lines on a particular chip, > you would need to identify the gpiochip and the offsets for that request. > So two fields - the gpiochip path, and the list of offsets. > > Is that already too clunky or am I missing something? > It's worse than that - we don't know the character device's filesystem path in gpiolib. Nor should we, as we can be in a different fs namespace when checking it than in which we were when we opened the device (which is also another concern for storing the path to the character device in struct gpiod_chip - unless we specify explicitly that it's the path that was used to open it). Since we don't know it - we can only get it from the file descriptor that the requesting process got after calling open() on the GPIO device. But this fd may have been closed in the meantime. I think I opened a can of worms with this one. :) Bartosz
On Tue, Sep 13, 2022 at 04:35:08PM +0200, Bartosz Golaszewski wrote: > On Tue, Sep 13, 2022 at 4:28 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Sep 13, 2022 at 10:54:26AM +0200, Bartosz Golaszewski wrote: > > > On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > > > > > you inherit all file descriptors from your parent. Only once you call > > > > > execve() are the fds with this flag closed *in the child*. > > > > > > > > > > > > > Ah, ok. > > > > You want to pass request fd ownership from parent to child?? > > > > Why not lock ownership to the parent, so O_CLOFORK, were that > > > > available? > > > > > > > > > > Because what if we want to request a line and then daemonize i.e. fork > > > and exit in parent? It makes much more sense to keep the lines > > > requested in the child IMO. > > > > > > > Then you are doing it backwards - daemonize first ;-). > > > > Generally speaking, doesn't transfer of resource ownership to the forked > > child create havoc in multi-threaded apps? i.e. one thread requests a > > resource, another forks. The parent thread unknowingly loses ownership, > > and the forked child process only starts with a replica of the forking > > thread. > > > > Yeah, sounds like a bad idea. > > > > During the BoF at Linux Plumbers it was suggested to use > > > /proc/$PID/fdinfo to expose the information about which lines are > > > requested but I can't figure out a way to do it elegantly. > > > > > > > Yeah, missed that :-(. > > > > Makes sense. > > > > As each request fd can contain multiple lines on a particular chip, > > you would need to identify the gpiochip and the offsets for that request. > > So two fields - the gpiochip path, and the list of offsets. > > > > Is that already too clunky or am I missing something? > > > > It's worse than that - we don't know the character device's filesystem > path in gpiolib. Nor should we, as we can be in a different fs > namespace when checking it than in which we were when we opened the > device (which is also another concern for storing the path to the > character device in struct gpiod_chip - unless we specify explicitly > that it's the path that was used to open it). Since we don't know it - > we can only get it from the file descriptor that the requesting > process got after calling open() on the GPIO device. But this fd may > have been closed in the meantime. I think I opened a can of worms with > this one. :) > Forgot that we don't have the path readily available in the kernel - would device name suffice? Cheers, Kent.
On Tue, Sep 13, 2022 at 4:55 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Sep 13, 2022 at 04:35:08PM +0200, Bartosz Golaszewski wrote: > > On Tue, Sep 13, 2022 at 4:28 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Tue, Sep 13, 2022 at 10:54:26AM +0200, Bartosz Golaszewski wrote: > > > > On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > > > > > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > > > > > > you inherit all file descriptors from your parent. Only once you call > > > > > > execve() are the fds with this flag closed *in the child*. > > > > > > > > > > > > > > > > Ah, ok. > > > > > You want to pass request fd ownership from parent to child?? > > > > > Why not lock ownership to the parent, so O_CLOFORK, were that > > > > > available? > > > > > > > > > > > > > Because what if we want to request a line and then daemonize i.e. fork > > > > and exit in parent? It makes much more sense to keep the lines > > > > requested in the child IMO. > > > > > > > > > > Then you are doing it backwards - daemonize first ;-). > > > > > > Generally speaking, doesn't transfer of resource ownership to the forked > > > child create havoc in multi-threaded apps? i.e. one thread requests a > > > resource, another forks. The parent thread unknowingly loses ownership, > > > and the forked child process only starts with a replica of the forking > > > thread. > > > > > > > Yeah, sounds like a bad idea. > > > > > > During the BoF at Linux Plumbers it was suggested to use > > > > /proc/$PID/fdinfo to expose the information about which lines are > > > > requested but I can't figure out a way to do it elegantly. > > > > > > > > > > Yeah, missed that :-(. > > > > > > Makes sense. > > > > > > As each request fd can contain multiple lines on a particular chip, > > > you would need to identify the gpiochip and the offsets for that request. > > > So two fields - the gpiochip path, and the list of offsets. > > > > > > Is that already too clunky or am I missing something? > > > > > > > It's worse than that - we don't know the character device's filesystem > > path in gpiolib. Nor should we, as we can be in a different fs > > namespace when checking it than in which we were when we opened the > > device (which is also another concern for storing the path to the > > character device in struct gpiod_chip - unless we specify explicitly > > that it's the path that was used to open it). Since we don't know it - > > we can only get it from the file descriptor that the requesting > > process got after calling open() on the GPIO device. But this fd may > > have been closed in the meantime. I think I opened a can of worms with > > this one. :) > > > > Forgot that we don't have the path readily available in the kernel - > would device name suffice? Maybe. I'm looking at what fdinfo shows in my vm and see things like: $ cat /proc/2353/fdinfo/10 pos: 0 flags: 02004000 mnt_id: 15 ino: 1052 inotify wd:1 ino:1 sdev:3c mask:fce ignored_mask:0 fhandle-bytes:c fhandle-type:1 f_handle:7f0dd9400100000000000000 I don't see any tools/libs readily available for parsing these. In libgpiod, if the user wanted to read the PID of the owner of the line, we'd need to manually go through the list of all fdinfo entries we have permissions to access and compare those against the line w'ere checking. We'd need of course first expose that info like: gpio chip:gpiochip2 lines:0,3,4,7 Does that make sense? Bart
On Tue, Sep 13, 2022 at 05:58:32PM +0200, Bartosz Golaszewski wrote: > On Tue, Sep 13, 2022 at 4:55 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Sep 13, 2022 at 04:35:08PM +0200, Bartosz Golaszewski wrote: > > > On Tue, Sep 13, 2022 at 4:28 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Tue, Sep 13, 2022 at 10:54:26AM +0200, Bartosz Golaszewski wrote: > > > > > On Tue, Sep 13, 2022 at 4:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > On Mon, Sep 12, 2022 at 11:56:17AM +0200, Bartosz Golaszewski wrote: > > > > > > > On Mon, Sep 12, 2022 at 11:53 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > > > > > > > > > [snip] > > > > > > > > > > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > > > > > > > > > O_CLOEXEC means "close on exec" not "close on fork". When you fork, > > > > > > > you inherit all file descriptors from your parent. Only once you call > > > > > > > execve() are the fds with this flag closed *in the child*. > > > > > > > > > > > > > > > > > > > Ah, ok. > > > > > > You want to pass request fd ownership from parent to child?? > > > > > > Why not lock ownership to the parent, so O_CLOFORK, were that > > > > > > available? > > > > > > > > > > > > > > > > Because what if we want to request a line and then daemonize i.e. fork > > > > > and exit in parent? It makes much more sense to keep the lines > > > > > requested in the child IMO. > > > > > > > > > > > > > Then you are doing it backwards - daemonize first ;-). > > > > > > > > Generally speaking, doesn't transfer of resource ownership to the forked > > > > child create havoc in multi-threaded apps? i.e. one thread requests a > > > > resource, another forks. The parent thread unknowingly loses ownership, > > > > and the forked child process only starts with a replica of the forking > > > > thread. > > > > > > > > > > Yeah, sounds like a bad idea. > > > > > > > > During the BoF at Linux Plumbers it was suggested to use > > > > > /proc/$PID/fdinfo to expose the information about which lines are > > > > > requested but I can't figure out a way to do it elegantly. > > > > > > > > > > > > > Yeah, missed that :-(. > > > > > > > > Makes sense. > > > > > > > > As each request fd can contain multiple lines on a particular chip, > > > > you would need to identify the gpiochip and the offsets for that request. > > > > So two fields - the gpiochip path, and the list of offsets. > > > > > > > > Is that already too clunky or am I missing something? > > > > > > > > > > It's worse than that - we don't know the character device's filesystem > > > path in gpiolib. Nor should we, as we can be in a different fs > > > namespace when checking it than in which we were when we opened the > > > device (which is also another concern for storing the path to the > > > character device in struct gpiod_chip - unless we specify explicitly > > > that it's the path that was used to open it). Since we don't know it - > > > we can only get it from the file descriptor that the requesting > > > process got after calling open() on the GPIO device. But this fd may > > > have been closed in the meantime. I think I opened a can of worms with > > > this one. :) > > > > > > > Forgot that we don't have the path readily available in the kernel - > > would device name suffice? > > Maybe. I'm looking at what fdinfo shows in my vm and see things like: > > $ cat /proc/2353/fdinfo/10 > pos: 0 > flags: 02004000 > mnt_id: 15 > ino: 1052 > inotify wd:1 ino:1 sdev:3c mask:fce ignored_mask:0 fhandle-bytes:c > fhandle-type:1 f_handle:7f0dd9400100000000000000 > For a gpio fd (reported as gpio-line by lsof) I only get the basics: pos: 0 flags: 02000000 mnt_id: 14 ino: 7661 > I don't see any tools/libs readily available for parsing these. In > libgpiod, if the user wanted to read the PID of the owner of the line, > we'd need to manually go through the list of all fdinfo entries we > have permissions to access and compare those against the line w'ere > checking. > > We'd need of course first expose that info like: > > gpio chip:gpiochip2 lines:0,3,4,7 > > Does that make sense? > Makes sense to me, though I don't claim to know anything about fdinfo field formatting. e.g. I also see fdinfo fields like this: eventfd-count: 0 eventfd-id: 1 so gpio-chip: gpiochip2 gpio-lines: 0,3,4,7 might be ok too. Cheers, Kent.
On Wed, Sep 14, 2022 at 12:17:39AM +0800, Kent Gibson wrote: > On Tue, Sep 13, 2022 at 05:58:32PM +0200, Bartosz Golaszewski wrote: > > On Tue, Sep 13, 2022 at 4:55 PM Kent Gibson <warthog618@gmail.com> wrote: ... > > We'd need of course first expose that info like: > > > > gpio chip:gpiochip2 lines:0,3,4,7 > > > > Does that make sense? > > Makes sense to me, though I don't claim to know anything about fdinfo > field formatting. > > e.g. I also see fdinfo fields like this: > > eventfd-count: 0 > eventfd-id: 1 > > so > > gpio-chip: gpiochip2 > gpio-lines: 0,3,4,7 > > might be ok too. Always think about two or more GPIO chips in the same process with 1 or more lines requested from each of them.
On Tue, Sep 13, 2022 at 7:09 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Sep 14, 2022 at 12:17:39AM +0800, Kent Gibson wrote: > > On Tue, Sep 13, 2022 at 05:58:32PM +0200, Bartosz Golaszewski wrote: > > > On Tue, Sep 13, 2022 at 4:55 PM Kent Gibson <warthog618@gmail.com> wrote: > > ... > > > > We'd need of course first expose that info like: > > > > > > gpio chip:gpiochip2 lines:0,3,4,7 > > > > > > Does that make sense? > > > > Makes sense to me, though I don't claim to know anything about fdinfo > > field formatting. > > > > e.g. I also see fdinfo fields like this: > > > > eventfd-count: 0 > > eventfd-id: 1 > > > > so > > > > gpio-chip: gpiochip2 > > gpio-lines: 0,3,4,7 > > > > might be ok too. > > Always think about two or more GPIO chips in the same process with 1 or more > lines requested from each of them. > That is fine because every file descriptor has its own fdinfo entry and so one entry can only contain information about a single request. Bart
On Tue, Sep 13, 2022 at 08:07:26PM +0300, Andy Shevchenko wrote: > On Wed, Sep 14, 2022 at 12:17:39AM +0800, Kent Gibson wrote: > > On Tue, Sep 13, 2022 at 05:58:32PM +0200, Bartosz Golaszewski wrote: > > > On Tue, Sep 13, 2022 at 4:55 PM Kent Gibson <warthog618@gmail.com> wrote: > > ... > > > > We'd need of course first expose that info like: > > > > > > gpio chip:gpiochip2 lines:0,3,4,7 > > > > > > Does that make sense? > > > > Makes sense to me, though I don't claim to know anything about fdinfo > > field formatting. > > > > e.g. I also see fdinfo fields like this: > > > > eventfd-count: 0 > > eventfd-id: 1 > > > > so > > > > gpio-chip: gpiochip2 > > gpio-lines: 0,3,4,7 > > > > might be ok too. > > Always think about two or more GPIO chips in the same process with 1 or more > lines requested from each of them. > I considered that - as Bart noted and as I stated earlier, each request fd is limited to one gpiochip and one set of offsets. And those are fixed for the lifetime of the request. Different requests will be different fds. But on the subject of repeats in fdinfo, I get the impression that multi-field fdinfo rows, e.g. the tfd rows here: pos: 0 flags: 02000002 mnt_id: 14 ino: 7661 tfd: 12 events: 19 data: c pos:0 ino:1ded sdev:d tfd: 14 events: 19 data: e pos:0 ino:1ded sdev:d are formatted that way as they may be repeated, so they are getting all their ducks in a row, as it were. So perhaps the gpio-lines could be exploded into repeated gpio-line rows? That may be going overboard as we are only encoding one field per line at the moment, not a struct as in the tfd case, but might we ever want to add more details? Again, just speculating based on the few examples I have on hand. Cheers, Kent.
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; /* Space reserved for future use. */ - __u32 padding[4]; + __u32 padding[3]; }; /**
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. 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@bgdev.pl> --- 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(-)