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

[Kent Gibson <warthog618@gmail.com>]

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