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

[Kent Gibson <warthog618@gmail.com>]

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.