[PATCH 0/2] gpiolib: export the consumer's PID to user-space

[PATCH 0/2] gpiolib: export the consumer's PID to user-space

[Bartosz Golaszewski]

I've been asked several times independently over the course of last months
for a way to figure out the PID of the user-space process that's holding
a specific GPIO line. This does sound like a valid use-case as the user may
create a background process that requests some lines and then want to kill
it to release those lines.

These patches propose to extend the gpio_v2_line_info struct with the
consumer's PID which is set to the process ID for user-space consumers and
0 for kernel-space ones.

Bartosz Golaszewski (2):
  gpiolib: un-inline gpiod_request_user()
  gpiolib: cdev: export the consumer's PID

 drivers/gpio/gpiolib-cdev.c |  2 ++
 drivers/gpio/gpiolib.c      | 33 +++++++++++++++++++++++++++++----
 drivers/gpio/gpiolib.h      | 14 +++-----------
 include/uapi/linux/gpio.h   |  5 ++++-
 4 files changed, 38 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH 1/2] gpiolib: un-inline gpiod_request_user()

[Bartosz Golaszewski]

Pull this bit of code into gpiolib.c as we're soon be calling certain
symbols static in this compilation unit.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib.c | 11 +++++++++++
 drivers/gpio/gpiolib.h | 12 +-----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cc9c0a12259e..6768734b9e15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2009,6 +2009,17 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
 	return ret;
 }
 
+int gpiod_request_user(struct gpio_desc *desc, const char *label)
+{
+	int ret;
+
+	ret = gpiod_request(desc, label);
+	if (ret == -EPROBE_DEFER)
+		ret = -ENODEV;
+
+	return ret;
+}
+
 static bool gpiod_free_commit(struct gpio_desc *desc)
 {
 	bool			ret = false;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index d900ecdbac46..b35deb08a7f5 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -179,19 +179,9 @@ struct gpio_desc {
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
+int gpiod_request_user(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
 
-static inline int gpiod_request_user(struct gpio_desc *desc, const char *label)
-{
-	int ret;
-
-	ret = gpiod_request(desc, label);
-	if (ret == -EPROBE_DEFER)
-		ret = -ENODEV;
-
-	return ret;
-}
-
 int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
 int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
-- 
2.34.1

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

[Bartosz Golaszewski]

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(-)

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];
 };
 
 /**
-- 
2.34.1

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] gpiolib: un-inline gpiod_request_user()

[Andy Shevchenko]

On Fri, Sep 09, 2022 at 02:13:28PM +0200, Bartosz Golaszewski wrote:
> Pull this bit of code into gpiolib.c as we're soon be calling certain
> symbols static in this compilation unit.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/gpio/gpiolib.c | 11 +++++++++++
>  drivers/gpio/gpiolib.h | 12 +-----------
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cc9c0a12259e..6768734b9e15 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2009,6 +2009,17 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
>  	return ret;
>  }
>  
> +int gpiod_request_user(struct gpio_desc *desc, const char *label)
> +{
> +	int ret;
> +
> +	ret = gpiod_request(desc, label);
> +	if (ret == -EPROBE_DEFER)
> +		ret = -ENODEV;
> +
> +	return ret;
> +}
> +
>  static bool gpiod_free_commit(struct gpio_desc *desc)
>  {
>  	bool			ret = false;
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index d900ecdbac46..b35deb08a7f5 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -179,19 +179,9 @@ struct gpio_desc {
>  #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
>  
>  int gpiod_request(struct gpio_desc *desc, const char *label);
> +int gpiod_request_user(struct gpio_desc *desc, const char *label);
>  void gpiod_free(struct gpio_desc *desc);
>  
> -static inline int gpiod_request_user(struct gpio_desc *desc, const char *label)
> -{
> -	int ret;
> -
> -	ret = gpiod_request(desc, label);
> -	if (ret == -EPROBE_DEFER)
> -		ret = -ENODEV;
> -
> -	return ret;
> -}
> -
>  int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>  		unsigned long lflags, enum gpiod_flags dflags);
>  int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

+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.

-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

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

Re: [PATCH 1/2] gpiolib: un-inline gpiod_request_user()

[Kent Gibson]

On Fri, Sep 09, 2022 at 02:13:28PM +0200, Bartosz Golaszewski wrote:
> Pull this bit of code into gpiolib.c as we're soon be calling certain
> symbols static in this compilation unit.
> 

Nit: "we'll soon be calling certain static symbols"

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/gpio/gpiolib.c | 11 +++++++++++
>  drivers/gpio/gpiolib.h | 12 +-----------
>  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cc9c0a12259e..6768734b9e15 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2009,6 +2009,17 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
>  	return ret;
>  }
>  
> +int gpiod_request_user(struct gpio_desc *desc, const char *label)
> +{
> +	int ret;
> +
> +	ret = gpiod_request(desc, label);
> +	if (ret == -EPROBE_DEFER)
> +		ret = -ENODEV;
> +
> +	return ret;
> +}
> +
>  static bool gpiod_free_commit(struct gpio_desc *desc)
>  {
>  	bool			ret = false;
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index d900ecdbac46..b35deb08a7f5 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -179,19 +179,9 @@ struct gpio_desc {
>  #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
>  
>  int gpiod_request(struct gpio_desc *desc, const char *label);
> +int gpiod_request_user(struct gpio_desc *desc, const char *label);
>  void gpiod_free(struct gpio_desc *desc);
>  
> -static inline int gpiod_request_user(struct gpio_desc *desc, const char *label)
> -{
> -	int ret;
> -
> -	ret = gpiod_request(desc, label);
> -	if (ret == -EPROBE_DEFER)
> -		ret = -ENODEV;
> -
> -	return ret;
> -}
> -
>  int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>  		unsigned long lflags, enum gpiod_flags dflags);
>  int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce);
> -- 
> 2.34.1
> 

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

[Kent Gibson]

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
> 

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

[Bartosz Golaszewski]

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

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

[Kent Gibson]

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.

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

[Bartosz Golaszewski]

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

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

[Kent Gibson]

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.

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

[Bartosz Golaszewski]

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

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

[Kent Gibson]

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.

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

[Bartosz Golaszewski]

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

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

[Kent Gibson]

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.

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

[Bartosz Golaszewski]

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

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

[Kent Gibson]

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.

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Bartosz Golaszewski]

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

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

[Kent Gibson]

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.