[PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Bartosz Golaszewski]

Add fdinfo output for file descriptors created for user-space line
requests in GPIO uAPI v2. The fdinfo file now contains the name of the
GPIO device that is the "parent" of the request as well as offsets of
the lines requested. This allows user-space to parse the /proc/$PID/fdinfo
entries and deduct the PID of the process that requested a specific line.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpiolib-cdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f8041d4898d1..0f7b5562c410 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1497,6 +1497,21 @@ static int linereq_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
+static void linereq_show_fdinfo(struct seq_file *out, struct file *file)
+{
+	struct linereq *lr = file->private_data;
+	struct device *dev = &lr->gdev->dev;
+	u16 i;
+
+	seq_printf(out, "gpio-device:\t%s\n", dev_name(dev));
+
+	for (i = 0; i < lr->num_lines; i++)
+		seq_printf(out, "gpio-line:\t%d\n",
+			   gpio_chip_hwgpio(lr->lines[i].desc));
+}
+#endif
+
 static const struct file_operations line_fileops = {
 	.release = linereq_release,
 	.read = linereq_read,
@@ -1507,6 +1522,9 @@ static const struct file_operations line_fileops = {
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = linereq_ioctl_compat,
 #endif
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo = linereq_show_fdinfo,
+#endif
 };
 
 static int linereq_create(struct gpio_device *gdev, void __user *ip)
-- 
2.34.1

Re: [PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Kent Gibson]

On Tue, Sep 20, 2022 at 03:54:35PM +0200, Bartosz Golaszewski wrote:
> Add fdinfo output for file descriptors created for user-space line
> requests in GPIO uAPI v2. The fdinfo file now contains the name of the
> GPIO device that is the "parent" of the request as well as offsets of
> the lines requested. This allows user-space to parse the /proc/$PID/fdinfo
> entries and deduct the PID of the process that requested a specific line.
> 
              deduce

Using deduct in this sense is unusual (not sure I've ever heard it used
that way, and some dictionaries don't even acknowledge it as a valid
option). In common usage deduct = subtract.

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/gpio/gpiolib-cdev.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index f8041d4898d1..0f7b5562c410 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1497,6 +1497,21 @@ static int linereq_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PROC_FS
> +static void linereq_show_fdinfo(struct seq_file *out, struct file *file)
> +{
> +	struct linereq *lr = file->private_data;
> +	struct device *dev = &lr->gdev->dev;
> +	u16 i;
> +
> +	seq_printf(out, "gpio-device:\t%s\n", dev_name(dev));
> +

So "device" rather than "chip"?

I realise it is the device name being reported here, but don't we use chip
for this in user-space?

> +	for (i = 0; i < lr->num_lines; i++)
> +		seq_printf(out, "gpio-line:\t%d\n",
> +			   gpio_chip_hwgpio(lr->lines[i].desc));
> +}

So the exploded form.  That works for me - in case we ever need to add
more line details, and also makes it easier to grep for a particular line.

Other than the comment nit and the device/chip question it looks good to me.

Cheers,
Kent.

> +#endif
> +
>  static const struct file_operations line_fileops = {
>  	.release = linereq_release,
>  	.read = linereq_read,
> @@ -1507,6 +1522,9 @@ static const struct file_operations line_fileops = {
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl = linereq_ioctl_compat,
>  #endif
> +#ifdef CONFIG_PROC_FS
> +	.show_fdinfo = linereq_show_fdinfo,
> +#endif
>  };
>  
>  static int linereq_create(struct gpio_device *gdev, void __user *ip)
> -- 
> 2.34.1
> 

Re: [PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 03:54:35PM +0200, Bartosz Golaszewski wrote:
> Add fdinfo output for file descriptors created for user-space line
> requests in GPIO uAPI v2. The fdinfo file now contains the name of the
> GPIO device that is the "parent" of the request as well as offsets of
> the lines requested. This allows user-space to parse the /proc/$PID/fdinfo
> entries and deduct the PID of the process that requested a specific line.

In principle I'm fine, but see below.

> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> ---
>  drivers/gpio/gpiolib-cdev.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index f8041d4898d1..0f7b5562c410 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1497,6 +1497,21 @@ static int linereq_release(struct inode *inode, struct file *file)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PROC_FS
> +static void linereq_show_fdinfo(struct seq_file *out, struct file *file)
> +{
> +	struct linereq *lr = file->private_data;
> +	struct device *dev = &lr->gdev->dev;
> +	u16 i;
> +
> +	seq_printf(out, "gpio-device:\t%s\n", dev_name(dev));
> +
> +	for (i = 0; i < lr->num_lines; i++)
> +		seq_printf(out, "gpio-line:\t%d\n",
> +			   gpio_chip_hwgpio(lr->lines[i].desc));

Hmm... Not sure which variant is better (as for machine parsing and for human),
but I was thinking of

	gpio-lines: 1,4,6, ...

Also don't forget that sizes over PAGE_SIZE in sysfs sometimes problematic and
racy.(the commit 888be6067b97 ("ACPI: sysfs: Fix a buffer overrun problem with
description_show()") for the reference).

> +}
> +#endif
> +
>  static const struct file_operations line_fileops = {
>  	.release = linereq_release,
>  	.read = linereq_read,
> @@ -1507,6 +1522,9 @@ static const struct file_operations line_fileops = {
>  #ifdef CONFIG_COMPAT
>  	.compat_ioctl = linereq_ioctl_compat,
>  #endif
> +#ifdef CONFIG_PROC_FS
> +	.show_fdinfo = linereq_show_fdinfo,
> +#endif
>  };
>  
>  static int linereq_create(struct gpio_device *gdev, void __user *ip)
> -- 
> 2.34.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 08:19:25PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 20, 2022 at 03:54:35PM +0200, Bartosz Golaszewski wrote:

...

> Also don't forget that sizes over PAGE_SIZE in sysfs sometimes problematic and
> racy.(the commit 888be6067b97 ("ACPI: sysfs: Fix a buffer overrun problem with
> description_show()") for the reference).

This is not the commit I wanted to point to... But suddenly can't find easily
the one I remembered happened in the kernel.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 08:29:29PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 20, 2022 at 08:19:25PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 20, 2022 at 03:54:35PM +0200, Bartosz Golaszewski wrote:

...

> > Also don't forget that sizes over PAGE_SIZE in sysfs sometimes problematic and
> > racy.(the commit 888be6067b97 ("ACPI: sysfs: Fix a buffer overrun problem with
> > description_show()") for the reference).
> 
> This is not the commit I wanted to point to... But suddenly can't find easily
> the one I remembered happened in the kernel.

Found the one:
00ee22c28915 ("PM / wakeup: Use seq_open() to show wakeup stats")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Bartosz Golaszewski]

On Tue, Sep 20, 2022 at 7:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Sep 20, 2022 at 03:54:35PM +0200, Bartosz Golaszewski wrote:
> > Add fdinfo output for file descriptors created for user-space line
> > requests in GPIO uAPI v2. The fdinfo file now contains the name of the
> > GPIO device that is the "parent" of the request as well as offsets of
> > the lines requested. This allows user-space to parse the /proc/$PID/fdinfo
> > entries and deduct the PID of the process that requested a specific line.
>
> In principle I'm fine, but see below.
>
> > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index f8041d4898d1..0f7b5562c410 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -1497,6 +1497,21 @@ static int linereq_release(struct inode *inode, struct file *file)
> >       return 0;
> >  }
> >
> > +#ifdef CONFIG_PROC_FS
> > +static void linereq_show_fdinfo(struct seq_file *out, struct file *file)
> > +{
> > +     struct linereq *lr = file->private_data;
> > +     struct device *dev = &lr->gdev->dev;
> > +     u16 i;
> > +
> > +     seq_printf(out, "gpio-device:\t%s\n", dev_name(dev));
> > +
> > +     for (i = 0; i < lr->num_lines; i++)
> > +             seq_printf(out, "gpio-line:\t%d\n",
> > +                        gpio_chip_hwgpio(lr->lines[i].desc));
>
> Hmm... Not sure which variant is better (as for machine parsing and for human),
> but I was thinking of
>

IMO it's pretty clear that the variant in this patch is easier for
machine parsing - one less tokenization.

>         gpio-lines: 1,4,6, ...
>
> Also don't forget that sizes over PAGE_SIZE in sysfs sometimes problematic and
> racy.(the commit 888be6067b97 ("ACPI: sysfs: Fix a buffer overrun problem with
> description_show()") for the reference).
>

In most systems PAGE_SIZE will be at least 4096 bytes. With this patch
a single "gpio-line:\t65535\n" is at most 17 bytes long x max 64 lines
= 1088 bytes. We're still way below the size where it would be
problematic.

Bart

> > +}
> > +#endif
> > +
> >  static const struct file_operations line_fileops = {
> >       .release = linereq_release,
> >       .read = linereq_read,
> > @@ -1507,6 +1522,9 @@ static const struct file_operations line_fileops = {
> >  #ifdef CONFIG_COMPAT
> >       .compat_ioctl = linereq_ioctl_compat,
> >  #endif
> > +#ifdef CONFIG_PROC_FS
> > +     .show_fdinfo = linereq_show_fdinfo,
> > +#endif
> >  };
> >
> >  static int linereq_create(struct gpio_device *gdev, void __user *ip)
> > --
> > 2.34.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH] gpiolib: cdev: add fdinfo output for line request file descriptors

[Andy Shevchenko]

On Tue, Sep 20, 2022 at 10:35:25PM +0200, Bartosz Golaszewski wrote:
> On Tue, Sep 20, 2022 at 7:19 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Sep 20, 2022 at 03:54:35PM +0200, Bartosz Golaszewski wrote:

...

> > > +     seq_printf(out, "gpio-device:\t%s\n", dev_name(dev));
> > > +
> > > +     for (i = 0; i < lr->num_lines; i++)
> > > +             seq_printf(out, "gpio-line:\t%d\n",
> > > +                        gpio_chip_hwgpio(lr->lines[i].desc));
> >
> > Hmm... Not sure which variant is better (as for machine parsing and for human),
> > but I was thinking of
> 
> IMO it's pretty clear that the variant in this patch is easier for
> machine parsing - one less tokenization.

For human it's harder in my opinion... But okay.

> >         gpio-lines: 1,4,6, ...
> >
> > Also don't forget that sizes over PAGE_SIZE in sysfs sometimes problematic and
> > racy.(the commit 888be6067b97 ("ACPI: sysfs: Fix a buffer overrun problem with
> > description_show()") for the reference).
> 
> In most systems PAGE_SIZE will be at least 4096 bytes. With this patch
> a single "gpio-line:\t65535\n" is at most 17 bytes long x max 64 lines
> = 1088 bytes. We're still way below the size where it would be
> problematic.

Okay, for now it's 64, but it will be problematic starting from ~300 requested
lines. Also, in case we add something more, who knows?

-- 
With Best Regards,
Andy Shevchenko