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

[Kent Gibson <warthog618@gmail.com>]

> 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