[PATCH] gpiolib: cdev: don't record a line direction the driver refused

[PATCH] gpiolib: cdev: don't record a line direction the driver refused

[Adrian Ambrozewicz]

When a line is requested or reconfigured through the v2 uAPI,
gpio_v2_line_config_flags_to_desc_flags() copies the requested
configuration into the descriptor flags, GPIOD_FLAG_IS_OUT included,
before gpiod_direction_input_nonotify() or
gpiod_direction_output_nonotify() asks the driver for the direction.
If the driver refuses, for instance with -EOPNOTSUPP from
.direction_input() for a line that can only be an output, the ioctl
fails but the flag keeps the refused direction. gpiod_free_commit()
does not touch GPIOD_FLAG_IS_OUT, so this outlives the failed request,
and GPIO_V2_GET_LINEINFO_IOCTL reports the line as an input while the
hardware still drives it, until a request without direction flags
makes gpiod_request_commit() read the direction back from the driver:

  GPIO_V2_GET_LINE_IOCTL, line 0, input:         -EOPNOTSUPP
  GPIO_V2_GET_LINEINFO_IOCTL, line 0:            input
  GPIO_V2_GET_LINE_IOCTL, line 0, no direction:  ok
  GPIO_V2_GET_LINEINFO_IOCTL, line 0:            output

GPIO_V2_LINE_SET_CONFIG_IOCTL updates the flags in the same order.
There the line stays requested, and since commit 92ac7de3175e
("gpiolib: don't allow setting values on input lines")
GPIO_V2_LINE_SET_VALUES_IOCTL on it then fails with -EPERM, though the
line is still an output. A line whose driver refuses to make it an
output is likewise reported as one.

gpiod_direction_input_nonotify() and gpiod_direction_output_nonotify()
set or clear GPIOD_FLAG_IS_OUT themselves once the driver has accepted
the direction, emulated open drain and open source included, and
nothing between the copy and those calls reads the flag. Leave it to
them, as the v1 uAPI already does.

The other flags that the helper copies stay as they are. Active low,
drive and bias are what gpiod_direction_*() applies, drive and bias
possibly before the driver refuses the direction; edges and event
clock are the character device's own. None of them is read back from
the driver, so there is no earlier state that is more accurate to go
back to. A failed request releases the line, which clears them, and a
failed reconfiguration leaves the line, like the lines before it, with
what was applied up to the failure.

Reproduced under QEMU with a test GPIO chip whose line 0 refuses input
and line 1 refuses output: both refused requests and both refused
reconfigurations leave the line info with the refused direction, and
setting the value of the reconfigured output fails with -EPERM. With
this change the line info shows the direction of the hardware in all
four cases and setting the value works. First seen on real hardware
with v6.12.

Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
Cc: stable@vger.kernel.org
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
 drivers/gpio/gpiolib-cdev.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5d53bfcdf726..9bbee3a1f9c9 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
 {
 	unsigned long flags = READ_ONCE(*flagsp);
 
+	/*
+	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
+	 * only update it once the chip has accepted the direction.
+	 */
 	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
 		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
 
-	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
-		set_bit(GPIOD_FLAG_IS_OUT, &flags);
-	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
-		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
-
 	assign_bit(GPIOD_FLAG_EDGE_RISING, &flags,
 		   lflags & GPIO_V2_LINE_FLAG_EDGE_RISING);
 	assign_bit(GPIOD_FLAG_EDGE_FALLING, &flags,

base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.43.0

Re: [PATCH] gpiolib: cdev: don't record a line direction the driver refused

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: memory leak in gpiod_set_array_value_complex() on error paths
- [High] gpio: non-atomic desc->flags update clobbers concurrent IRQ flags
--

commit 910242af9150f094f4676cb26fb8b58cc8edfa96
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

gpiolib: cdev: don't record a line direction the driver refused

The commit modifies gpio_v2_line_config_flags_to_desc_flags() to avoid setting
the GPIOD_FLAG_IS_OUT flag before the hardware direction is confirmed by the
driver. This fixes an issue where refused configurations would leave the line
erroneously reported as an output or input.

> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 5d53bfcdf7268..9bbee3a1f9c9c 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
>  {
>  	unsigned long flags = READ_ONCE(*flagsp);

[Severity: High]
Does this non-atomic read-modify-write clobber concurrent atomic updates to
desc->flags made by the IRQ subsystem?

If a driver or IRQ domain calls gpiochip_lock_as_irq() or
gpiochip_enable_irq() concurrently, they atomically update bits like
GPIOD_FLAG_USED_AS_IRQ:

	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);

Because the IRQ subsystem does not check GPIOD_FLAG_REQUESTED before using
the line and does not share locks with the cdev ioctl, these atomic updates
can happen between this READ_ONCE() and the subsequent
WRITE_ONCE(*flagsp, flags) at the end of this function.

Wouldn't the atomic updates be silently overwritten when this function writes
back the stale flags value?

Losing these flags disables the safety check in
gpiod_direction_output_nonotify(), which could allow user-space to force an
active interrupt line into output mode.

>  
> +	/*
> +	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
> +	 * only update it once the chip has accepted the direction.
> +	 */
>  	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
>  		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
>  
> -	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
> -		set_bit(GPIOD_FLAG_IS_OUT, &flags);
> -	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
> -		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
> -

[Severity: High]
By leaving GPIOD_FLAG_IS_OUT unset when a driver refuses the output direction,
does this create a memory leak in gpiod_set_array_value_complex() when
user-space subsequently calls GPIO_V2_LINE_SET_VALUES_IOCTL?

Looking at gpiod_set_array_value_complex() in drivers/gpio/gpiolib.c, mask
and bits can be dynamically allocated with bitmap_alloc() for large GPIO
chips:

			if (unlikely(!test_bit(GPIOD_FLAG_IS_OUT, &desc->flags)))
				return -EPERM;

If a descriptor is not configured as an output, the function immediately
returns -EPERM. 

Doesn't this bypass the bitmap_free() calls at the end of the function and
leak mask and bits?

The same direct return leak appears to occur if the driver's set_multiple
callback fails:

		if (count != 0) {
			ret = gpiochip_set_multiple(guard.gc, mask, bits);
			if (ret)
				return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-gpiolib-cdev-direction-v1-1-aambrozewicz@nvidia.com?part=1

Re: [PATCH] gpiolib: cdev: don't record a line direction the driver refused

[Kent Gibson]

On Mon, Oct 12, 2026 at 02:04:58PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] gpio: memory leak in gpiod_set_array_value_complex() on error paths
> - [High] gpio: non-atomic desc->flags update clobbers concurrent IRQ flags
> --
> 
> commit 910242af9150f094f4676cb26fb8b58cc8edfa96
> Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
> 

Hi Bart,

following this up with you directly since Sashiko doesn't currently let
original authors or maintainers know about problems it discovers in existing
code, just the patch author and the list.

> gpiolib: cdev: don't record a line direction the driver refused
> 
> The commit modifies gpio_v2_line_config_flags_to_desc_flags() to avoid setting
> the GPIOD_FLAG_IS_OUT flag before the hardware direction is confirmed by the
> driver. This fixes an issue where refused configurations would leave the line
> erroneously reported as an output or input.
> 
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index 5d53bfcdf7268..9bbee3a1f9c9c 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
> >  {
> >  	unsigned long flags = READ_ONCE(*flagsp);
> 
> [Severity: High]
> Does this non-atomic read-modify-write clobber concurrent atomic updates to
> desc->flags made by the IRQ subsystem?
> 
> If a driver or IRQ domain calls gpiochip_lock_as_irq() or
> gpiochip_enable_irq() concurrently, they atomically update bits like
> GPIOD_FLAG_USED_AS_IRQ:
> 
> 	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
> 	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);
> 
> Because the IRQ subsystem does not check GPIOD_FLAG_REQUESTED before using
> the line and does not share locks with the cdev ioctl, these atomic updates
> can happen between this READ_ONCE() and the subsequent
> WRITE_ONCE(*flagsp, flags) at the end of this function.
> 
> Wouldn't the atomic updates be silently overwritten when this function writes
> back the stale flags value?
> 
> Losing these flags disables the safety check in
> gpiod_direction_output_nonotify(), which could allow user-space to force an
> active interrupt line into output mode.
> 

This isn't a scenario that I'd considered - I was probably assuming
GPIOD_FLAG_REQUESTED being set would provide protection by making the
line unavailable.

How would you like to handle this?

> >  
> > +	/*
> > +	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
> > +	 * only update it once the chip has accepted the direction.
> > +	 */
> >  	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
> >  		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
> >  
> > -	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
> > -		set_bit(GPIOD_FLAG_IS_OUT, &flags);
> > -	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
> > -		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
> > -
> 
> [Severity: High]
> By leaving GPIOD_FLAG_IS_OUT unset when a driver refuses the output direction,
> does this create a memory leak in gpiod_set_array_value_complex() when
> user-space subsequently calls GPIO_V2_LINE_SET_VALUES_IOCTL?
> 
> Looking at gpiod_set_array_value_complex() in drivers/gpio/gpiolib.c, mask
> and bits can be dynamically allocated with bitmap_alloc() for large GPIO
> chips:
> 
> 			if (unlikely(!test_bit(GPIOD_FLAG_IS_OUT, &desc->flags)))
> 				return -EPERM;
> 
> If a descriptor is not configured as an output, the function immediately
> returns -EPERM. 
> 
> Doesn't this bypass the bitmap_free() calls at the end of the function and
> leak mask and bits?
> 
> The same direct return leak appears to occur if the driver's set_multiple
> callback fails:
> 
> 		if (count != 0) {
> 			ret = gpiochip_set_multiple(guard.gc, mask, bits);
> 			if (ret)
> 				return ret;
> 

This also looks like a valid concern in existing code.  Unlikely, but valid.

It is also not something we can readily test.
Would you be open to extending gpio-sim so we can simulate constrained drivers
to better test alternate paths in gpiolib and cdev?
e.g. input only, output only, not supporting bias, supporting open drain, open
source, debounce, ...?
I haven't put any thought into what that might look like on the
userspace side, much less done any actual work on it, - just wondering if you
had or would at least be ok with it.

Cheers,
Kent.

> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012-gpiolib-cdev-direction-v1-1-aambrozewicz@nvidia.com?part=1

Re: [PATCH] gpiolib: cdev: don't record a line direction the driver refused

[Kent Gibson]

On Mon, Oct 12, 2026 at 04:49:18PM +0300, Adrian Ambrozewicz wrote:

I would change the subject to "don't set GPIOD_FLAG_IS_OUT" as cdev
doesn't interact directly with the driver - gpiolib does, and the change
here is unconditional.

> When a line is requested or reconfigured through the v2 uAPI,
> gpio_v2_line_config_flags_to_desc_flags() copies the requested
> configuration into the descriptor flags, GPIOD_FLAG_IS_OUT included,
> before gpiod_direction_input_nonotify() or
> gpiod_direction_output_nonotify() asks the driver for the direction.
> If the driver refuses, for instance with -EOPNOTSUPP from
> .direction_input() for a line that can only be an output, the ioctl
> fails but the flag keeps the refused direction. gpiod_free_commit()
> does not touch GPIOD_FLAG_IS_OUT, so this outlives the failed request,
> and GPIO_V2_GET_LINEINFO_IOCTL reports the line as an input while the
> hardware still drives it, until a request without direction flags
> makes gpiod_request_commit() read the direction back from the driver:
> 
>   GPIO_V2_GET_LINE_IOCTL, line 0, input:         -EOPNOTSUPP
>   GPIO_V2_GET_LINEINFO_IOCTL, line 0:            input
>   GPIO_V2_GET_LINE_IOCTL, line 0, no direction:  ok
>   GPIO_V2_GET_LINEINFO_IOCTL, line 0:            output
> 
> GPIO_V2_LINE_SET_CONFIG_IOCTL updates the flags in the same order.
> There the line stays requested, and since commit 92ac7de3175e
> ("gpiolib: don't allow setting values on input lines")
> GPIO_V2_LINE_SET_VALUES_IOCTL on it then fails with -EPERM, though the
> line is still an output. A line whose driver refuses to make it an
> output is likewise reported as one.
> 
> gpiod_direction_input_nonotify() and gpiod_direction_output_nonotify()
> set or clear GPIOD_FLAG_IS_OUT themselves once the driver has accepted
> the direction, emulated open drain and open source included, and
> nothing between the copy and those calls reads the flag. Leave it to
> them, as the v1 uAPI already does.
> 
> The other flags that the helper copies stay as they are. Active low,
> drive and bias are what gpiod_direction_*() applies, drive and bias
> possibly before the driver refuses the direction; edges and event
> clock are the character device's own. None of them is read back from
> the driver, so there is no earlier state that is more accurate to go
> back to. A failed request releases the line, which clears them, and a
> failed reconfiguration leaves the line, like the lines before it, with
> what was applied up to the failure.
> 
> Reproduced under QEMU with a test GPIO chip whose line 0 refuses input
> and line 1 refuses output: both refused requests and both refused
> reconfigurations leave the line info with the refused direction, and
> setting the value of the reconfigured output fails with -EPERM. With
> this change the line info shows the direction of the hardware in all
> four cases and setting the value works. First seen on real hardware
> with v6.12.
> 
> Fixes: 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")
> Cc: stable@vger.kernel.org
> Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
> ---
>  drivers/gpio/gpiolib-cdev.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 5d53bfcdf726..9bbee3a1f9c9 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
>  {
>  	unsigned long flags = READ_ONCE(*flagsp);
>  
> +	/*
> +	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
> +	 * only update it once the chip has accepted the direction.
> +	 */

Not a fan of comments describing why code that should not be here is not
here.  The equivalent functions linehandle_flags_to_desc_flags() and
gpiod_configure_flags() don't have any mention of GPIOD_FLAG_IS_OUT, so
why here?

If this is part of a policy change to "gpiolib should manage GPIOD_FLAG_IS_OUT"
then gpiolib.h would be a more appropriate location.
Not that I'm suggesting adding such a comment - just that a comment here is
wrong for multiple reasons.

>  	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
>  		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
>  
> -	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
> -		set_bit(GPIOD_FLAG_IS_OUT, &flags);
> -	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
> -		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
> -

Having gpiolib manage GPIOD_FLAG_IS_OUT to keep it consistent with the
driver state makes sense, so no problem with the code change itself.

Cheers,
Kent.