[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

[Bartosz Golaszewski]

On Wed, 14 Oct 2026 06:36:45 +0200, Kent Gibson <warthog618@gmail.com> said:
> 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?
>

Sign... By adding it to the heap of GPIO issues found by sashiko. :(

We can't protect the flags with mutex or spinlock due to the multitude of
issues that forced us to use SRCU. We could maybe consider atomic_t for the
flags field and that would allow us to use various atomic ops and possibly it
would be enough to cover all the read-modify-write cases we have. I need to
think about it and will also ask the clanker for help.

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

Definitely! I don't see why I would ever be against extending the test
coverage. :)

Thanks,
Bartosz

> 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 Thu, Oct 15, 2026 at 06:36:26AM -0700, Bartosz Golaszewski wrote:
> On Wed, 14 Oct 2026 06:36:45 +0200, Kent Gibson <warthog618@gmail.com> said:
> > 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?
> >
> 
> Sign... By adding it to the heap of GPIO issues found by sashiko. :(
> 

Sorry - I was hoping to help take it off that heap.

> We can't protect the flags with mutex or spinlock due to the multitude of
> issues that forced us to use SRCU. We could maybe consider atomic_t for the
> flags field and that would allow us to use various atomic ops and possibly it
> would be enough to cover all the read-modify-write cases we have. I need to
> think about it and will also ask the clanker for help.
> 

I was thinking along the lines of reverting to atomic bitops directly
on the flags, e.g. reverting b7adfb6076ff, without changing the
underlying type to atomic_t.
Given that most of the time we are operating on individual flags.

With atomic_t you could also use xchg variants for bulk ops?

Anyway, let me know if there is anything I can do to assist.

> >> >
> >> > +	/*
> >> > +	 * 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.
> >
> 
> Definitely! I don't see why I would ever be against extending the test
> coverage. :)
> 

Ok, I'll try to give it some thought.

Cheers,
Kent.

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

[Adrian Ambrozewicz]

On Thu, Oct 15, 2026 at 11:37:14PM +0800, Kent Gibson wrote:
> On Thu, Oct 15, 2026 at 06:36:26AM -0700, Bartosz Golaszewski wrote:
> > We can't protect the flags with mutex or spinlock due to the multitude of
> > issues that forced us to use SRCU. We could maybe consider atomic_t for the
> > flags field and that would allow us to use various atomic ops and possibly it
> > would be enough to cover all the read-modify-write cases we have. I need to
> > think about it and will also ask the clanker for help.
>
> I was thinking along the lines of reverting to atomic bitops directly
> on the flags, e.g. reverting b7adfb6076ff, without changing the
> underlying type to atomic_t.

I'll leave the flags to you two. For whichever form you pick, there
are two places where my patches meet that code.

gpiod_free_commit() does the same read-modify-write: it reads the
flags, calls ->free(), which may sleep, and writes the word back.
Patch 8 of chip-removal v2 (for the debounce race Sashiko found on the
event clock fix) relies on that write: it resets debounce_period_us
first and makes the write a release store, pairing with the
test_and_set_bit() in gpiod_request_commit(), so that a new owner sees
the reset. With atomic bitops there, clearing GPIOD_FLAG_REQUESTED last
with clear_bit_unlock() would give the same ordering. v2 keeps the
release store, as that matches the code today.

v2 of this patch only drops the GPIOD_FLAG_IS_OUT lines from
gpio_v2_line_config_flags_to_desc_flags(), which b7adfb6076ff also
touches, so a revert and this patch only need rebasing over each other.

> > >> Doesn't this bypass the bitmap_free() calls at the end of the function and
> > >> leak mask and bits?
> > >
> > > This also looks like a valid concern in existing code.  Unlikely, but valid.
> > >
> > > It is also not something we can readily test.

It is: both the -EPERM return and the one after a failed
gpiochip_set_multiple() skip the bitmap_free() calls when the bitmaps
were allocated. The -EPERM path shows up with gpio-sim as it is: on a
chip with more lines than CONFIG_GPIOLIB_FASTPATH_LIMIT (512 by
default; I used 600), GPIO_V2_LINE_SET_VALUES_IOCTL on a line requested
as input leaves the two bitmaps to kmemleak. Unless Bart would rather take it
himself, I'll send the fix as patch 7 of chip-removal v2:

https://lore.kernel.org/r/20261012-gpiolib-chip-removal-v1-0-aambrozewicz@nvidia.com

Adrian

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

[Kent Gibson]

> gpiod_free_commit() does the same read-modify-write: it reads the
> flags, calls ->free(), which may sleep, and writes the word back.
> Patch 8 of chip-removal v2 (for the debounce race Sashiko found on the
> event clock fix) relies on that write: it resets debounce_period_us
> first and makes the write a release store, pairing with the
> test_and_set_bit() in gpiod_request_commit(), so that a new owner sees
> the reset. With atomic bitops there, clearing GPIOD_FLAG_REQUESTED last
> with clear_bit_unlock() would give the same ordering. v2 keeps the
> release store, as that matches the code today.
> 
> v2 of this patch only drops the GPIOD_FLAG_IS_OUT lines from
> gpio_v2_line_config_flags_to_desc_flags(), which b7adfb6076ff also
> touches, so a revert and this patch only need rebasing over each other.
> 
> > > >> Doesn't this bypass the bitmap_free() calls at the end of the function and
> > > >> leak mask and bits?
> > > >
> > > > This also looks like a valid concern in existing code.  Unlikely, but valid.
> > > >
> > > > It is also not something we can readily test.
> 
> It is: both the -EPERM return and the one after a failed
> gpiochip_set_multiple() skip the bitmap_free() calls when the bitmaps
> were allocated. The -EPERM path shows up with gpio-sim as it is: on a
> chip with more lines than CONFIG_GPIOLIB_FASTPATH_LIMIT (512 by
> default; I used 600), GPIO_V2_LINE_SET_VALUES_IOCTL on a line requested
> as input leaves the two bitmaps to kmemleak.

I was thinking specifically of the gpiochip_set_multiple() error path.
How do you elicit a gpiochip_set_multiple() failure from gpio-sim -
it always returns 0?

> Unless Bart would rather take it
> himself, I'll send the fix as patch 7 of chip-removal v2:
> 

Oh nuts, I overlooked the EPERM case.
Bart has already merged a patch for the gpiod_set_multiple() failure leak[1]
- you were Cced.
I would keep patches in your removal series focused on that and keep
patches for existing problems separate so we can get that series merged as
soon as possible.

I'll send out a patch for the EPERM path shortly.

Cheers,
Kent.

[1] https://lore.kernel.org/linux-gpio/179206624090.9766.14775829225106044242.b4-ty@oss.qualcomm.com/T/#t

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

[Bartosz Golaszewski]

On Fri, 16 Oct 2026 10:37:09 +0200, Kent Gibson <warthog618@gmail.com> said:
>
>> gpiod_free_commit() does the same read-modify-write: it reads the
>> flags, calls ->free(), which may sleep, and writes the word back.
>> Patch 8 of chip-removal v2 (for the debounce race Sashiko found on the
>> event clock fix) relies on that write: it resets debounce_period_us
>> first and makes the write a release store, pairing with the
>> test_and_set_bit() in gpiod_request_commit(), so that a new owner sees
>> the reset. With atomic bitops there, clearing GPIOD_FLAG_REQUESTED last
>> with clear_bit_unlock() would give the same ordering. v2 keeps the
>> release store, as that matches the code today.
>>
>> v2 of this patch only drops the GPIOD_FLAG_IS_OUT lines from
>> gpio_v2_line_config_flags_to_desc_flags(), which b7adfb6076ff also
>> touches, so a revert and this patch only need rebasing over each other.
>>
>> > > >> Doesn't this bypass the bitmap_free() calls at the end of the function and
>> > > >> leak mask and bits?
>> > > >
>> > > > This also looks like a valid concern in existing code.  Unlikely, but valid.
>> > > >
>> > > > It is also not something we can readily test.
>>
>> It is: both the -EPERM return and the one after a failed
>> gpiochip_set_multiple() skip the bitmap_free() calls when the bitmaps
>> were allocated. The -EPERM path shows up with gpio-sim as it is: on a
>> chip with more lines than CONFIG_GPIOLIB_FASTPATH_LIMIT (512 by
>> default; I used 600), GPIO_V2_LINE_SET_VALUES_IOCTL on a line requested
>> as input leaves the two bitmaps to kmemleak.
>
> I was thinking specifically of the gpiochip_set_multiple() error path.
> How do you elicit a gpiochip_set_multiple() failure from gpio-sim -
> it always returns 0?
>
>> Unless Bart would rather take it
>> himself, I'll send the fix as patch 7 of chip-removal v2:
>>
>
> Oh nuts, I overlooked the EPERM case.
> Bart has already merged a patch for the gpiod_set_multiple() failure leak[1]
> - you were Cced.
> I would keep patches in your removal series focused on that and keep
> patches for existing problems separate so we can get that series merged as
> soon as possible.
>
> I'll send out a patch for the EPERM path shortly.
>

I already sent out my final fixes PR for v7.3 but no worries, I'll keep
applying fixes next week and send another fixes PR during the merge window.

Bart

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

[Adrian Ambrozewicz]

On Fri, Oct 16, 2026 at 04:37:09PM +0800, Kent Gibson wrote:
> I was thinking specifically of the gpiochip_set_multiple() error path.
> How do you elicit a gpiochip_set_multiple() failure from gpio-sim -
> it always returns 0?

I don't; only the -EPERM path reproduces with gpio-sim. The
gpiochip_set_multiple() one I only know from reading the code.

> Oh nuts, I overlooked the EPERM case.
> Bart has already merged a patch for the gpiod_set_multiple() failure leak[1]
> - you were Cced.

Sorry, I missed that one.

> I would keep patches in your removal series focused on that and keep
> patches for existing problems separate so we can get that series merged as
> soon as possible.
>
> I'll send out a patch for the EPERM path shortly.

Thanks, I'll leave it to you and drop the leak fix from the series. I'll
run your patch through the kmemleak test above and reply with the result.

v2 of the removal series will then have the six removal fixes only. The
debounce fix depends on the reworked gpiod_free_commit() in patch 2, so
I'll send it on its own once the series is in.

Adrian

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

[Bartosz Golaszewski]

On Thu, 15 Oct 2026 17:37:14 +0200, Kent Gibson <warthog618@gmail.com> said:
>
>> We can't protect the flags with mutex or spinlock due to the multitude of
>> issues that forced us to use SRCU. We could maybe consider atomic_t for the
>> flags field and that would allow us to use various atomic ops and possibly it
>> would be enough to cover all the read-modify-write cases we have. I need to
>> think about it and will also ask the clanker for help.
>>
>
> I was thinking along the lines of reverting to atomic bitops directly
> on the flags, e.g. reverting b7adfb6076ff, without changing the
> underlying type to atomic_t.
> Given that most of the time we are operating on individual flags.
>

That's still two threads fighting over the flags effectively, right? Ideally
we should modify the entire set of flags at once. If that proves to be
impossible, going back to atomic bitops is probably the next best thing.

> With atomic_t you could also use xchg variants for bulk ops?
>

I'm not following, could you rephrase or give an example?

>>
>> Definitely! I don't see why I would ever be against extending the test
>> coverage. :)
>>
>
> Ok, I'll try to give it some thought.
>

Please also take a look at gpiolib-kunit.c. It may be a better fit for this
kind of testing.

Bart

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

[Kent Gibson]

On Fri, Oct 16, 2026 at 03:23:24AM -0500, Bartosz Golaszewski wrote:
> On Thu, 15 Oct 2026 17:37:14 +0200, Kent Gibson <warthog618@gmail.com> said:
> >
> >> We can't protect the flags with mutex or spinlock due to the multitude of
> >> issues that forced us to use SRCU. We could maybe consider atomic_t for the
> >> flags field and that would allow us to use various atomic ops and possibly it
> >> would be enough to cover all the read-modify-write cases we have. I need to
> >> think about it and will also ask the clanker for help.
> >>
> >
> > I was thinking along the lines of reverting to atomic bitops directly
> > on the flags, e.g. reverting b7adfb6076ff, without changing the
> > underlying type to atomic_t.
> > Given that most of the time we are operating on individual flags.
> >
> 
> That's still two threads fighting over the flags effectively, right? Ideally
> we should modify the entire set of flags at once. If that proves to be
> impossible, going back to atomic bitops is probably the next best thing.
> 

Yeah, that change wouldn't resolve that conflict - it is just about
deciding which framework you plan to use for the flags.
The IRQ flag problem, and any other flag contention lurking, needs to be
addressed separately either way.

> > With atomic_t you could also use xchg variants for bulk ops?
> >
> 
> I'm not following, could you rephrase or give an example?
> 

I mean atomic_xchg(), atomic_cmpxchg() etc.
I thought atomic_t was intended for counters, not bitmaps of flags, and
I don't have any example, but I expect the code to be pretty hideous - 
You would need to READ_ONCE(), check the flags are in a suitable state,
update them, then atomic_try_cmpxchg() with your updated value.
Rinse and repeat if the try_cmpxchg fails.

> >>
> >> Definitely! I don't see why I would ever be against extending the test
> >> coverage. :)
> >>
> >
> > Ok, I'll try to give it some thought.
> >
> 
> Please also take a look at gpiolib-kunit.c. It may be a better fit for this
> kind of testing.

Will do, but I was thinking of driving error paths in userspace libraries as
well.

Cheers,
Kent.

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.

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

[Adrian Ambrozewicz]

On Wed, Oct 14, 2026 at 12:56:24PM +0800, Kent Gibson 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.

Agreed, v2 will be "gpiolib: cdev: don't set GPIOD_FLAG_IS_OUT".

> Not a fan of comments describing why code that should not be here is not
> here.

Fair, I'll drop it.

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

Thanks for the review. v2 follows with those two changes.

Adrian