Re: [PATCH v3 2/6] gpiolib: free the label of lines released after their chip was removed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpiolib: premature label clear in gpiod_free_commit() before gc->free()
--

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

gpiolib: free the label of lines released after their chip was removed

This commit moves desc_set_label(desc, NULL) in gpiod_free_commit() ahead of
the guard.gc check so that consumer labels are still freed when a requested
line is released after gpiochip_remove() has cleared gdev->chip.

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e5326c472a544..e923512febf15 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2666,7 +2666,12 @@ void gpiod_free_commit(struct gpio_desc *desc)
>  
>  	flags = READ_ONCE(desc->flags);
>  
> -	if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
> +	if (guard.gc && !test_bit(GPIOD_FLAG_REQUESTED, &flags))
> +		return;
> +
> +	desc_set_label(desc, NULL);
> +
> +	if (guard.gc) {
>  		if (guard.gc->free)
>  			guard.gc->free(guard.gc, gpiod_hwgpio(desc));

[Severity: High]
Can clearing desc->label before invoking guard.gc->free() and before clearing
GPIOD_FLAG_REQUESTED cause a concurrent for_each_hwgpio() or
for_each_requested_gpio() caller to pass ERR_PTR(-ENOMEM) to kfree()?

In drivers/gpio/gpiolib.c:gpiod_free_commit(), desc_set_label(desc, NULL) now
runs before guard.gc->free() and before GPIOD_FLAG_REQUESTED is cleared via
WRITE_ONCE(desc->flags, flags). Because guard.gc->free() can sleep or block on
driver locks (such as &chip->lock in gpio_sim_free(), &pctldev->mutex in
pinctrl_gpio_free(), or bus transfers in I2C/SPI/USB GPIO drivers), this leaves
a window where GPIOD_FLAG_REQUESTED is still set while desc->label is NULL.

For example, in drivers/gpio/gpio-sim.c:gpio_sim_dbg_show():

	guard(mutex)(&chip->lock);

	for_each_hwgpio(gc, i, label)

If gpiod_free_commit() clears desc->label and then blocks on &chip->lock in
gpio_sim_free() while gpio_sim_dbg_show() (or another caller such as
intel_restore_hostown() or lp_gpio_resume()) iterates the lines,
drivers/gpio/gpiolib.c:gpiochip_dup_line_label() will see GPIOD_FLAG_REQUESTED
still set:

	if (!test_bit(GPIOD_FLAG_REQUESTED, &desc->flags))
		return NULL;

	guard(srcu)(&desc->gdev->desc_srcu);

	label = kstrdup(gpiod_get_label(desc), GFP_KERNEL);
	if (!label)
		return ERR_PTR(-ENOMEM);

Since gpiod_get_label(desc) is already NULL, kstrdup(NULL, GFP_KERNEL) returns
NULL and gpiochip_dup_line_label() returns ERR_PTR(-ENOMEM).

Then in include/linux/gpio/driver.h:for_each_hwgpio_in_range():

#define for_each_hwgpio_in_range(_chip, _i, _base, _size, _label)			\
	for (CLASS(_gpiochip_for_each_data, _data)(&_label, &_i);			\
	     _i < _size;								\
	     _i++, kfree(_label), _label = NULL)					\
		for_each_if(!IS_ERR(_label = gpiochip_dup_line_label(_chip, _base + _i)))

for_each_if(!IS_ERR(_label = ...)) skips the loop body and the loop increment
calls kfree(ERR_PTR(-ENOMEM)). In addition, gpiolib_dbg_show() can observe
GPIOD_FLAG_REQUESTED with a NULL label and print (null) while guard.gc->free()
sleeps.

Should desc_set_label(desc, NULL) instead remain after guard.gc->free() (or run
after the if (guard.gc) block)?

> @@ -2685,7 +2690,6 @@ void gpiod_free_commit(struct gpio_desc *desc)
>  #ifdef CONFIG_OF_DYNAMIC
>  		WRITE_ONCE(desc->hog, NULL);
>  #endif
> -		desc_set_label(desc, NULL);
>  		WRITE_ONCE(desc->flags, flags);
>  #ifdef CONFIG_GPIO_CDEV
>  		WRITE_ONCE(desc->debounce_period_us, 0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-gpiolib-chip-removal-v3-0-aambrozewicz@nvidia.com?part=2