Re: [PATCH] pinctrl: meson: amlogic-a4: use nolock get range

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Lockless iteration of `pctldev->gpio_ranges` introduces a data race against concurrent list modifications.

Pre-existing issues:
- [High] Missing validation of `pinctrl_find_gpio_range_from_pin_nolock()` return value leading to NULL pointer dereference.
--

commit f9e63102e7090909caca32b069c2328643c7a837
Author: Xianwei Zhao <[email protected]>

pinctrl: meson: amlogic-a4: use nolock get range

This commit updates the amlogic-a4 pinctrl driver to use the nolock
variant of pinctrl_find_gpio_range_from_pin() when configuring pins or
setting GPIO values. This avoids taking the lock and allows the code to be
safely called from interrupt context.

> diff --git a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
> index 35d27626a336b..29702b363e222 100644
> --- a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
> +++ b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
> @@ -250,7 +250,7 @@ static int aml_pmx_set_mux(struct pinctrl_dev *pctldev, unsigned int fselector,
>  	int i;
>  
>  	for (i = 0; i < group->npins; i++) {
> -		range =  pinctrl_find_gpio_range_from_pin(pctldev, group->pins[i]);
> +		range =  pinctrl_find_gpio_range_from_pin_nolock(pctldev, group->pins[i]);

[Severity: High]
Can this introduce a data race against concurrent list modifications?

The pctldev->gpio_ranges list is a standard doubly-linked list. Functions
like aml_pmx_set_mux() can be reached via pinctrl_select_state(), which
does not hold pctldev->mutex.

Because aml_pctl_probe() exposes the pinctrl device globally via
devm_pinctrl_register() before adding GPIO ranges via gpiochip_add_data(),
concurrent consumer probes could invoke these lockless traversals exactly
while list_add_tail() is actively mutating the list under pctldev->mutex.
Could this traverse corrupted list pointers and cause an infinite loop or
kernel panic?

[Severity: High]
This is a pre-existing issue, but does this missing NULL check lead to a
dereference if a pin is not found?

If a requested pin does not belong to any registered GPIO range,
pinctrl_find_gpio_range_from_pin_nolock() returns NULL. The returned range
pointer is then passed directly to aml_pctl_set_function() which
immediately dereferences it via range->gc in the gpio_chip_to_bank() macro:

aml_pctl_set_function()
    struct aml_gpio_bank *bank = gpio_chip_to_bank(range->gc);

>  		aml_pctl_set_function(info, range, group->pins[i], group->func[i]);
>  	}
>  

[ ... ]

> @@ -449,7 +449,7 @@ static int aml_pinconf_disable_bias(struct aml_pinctrl *info,
>  				    unsigned int pin)
>  {
>  	struct pinctrl_gpio_range *range =
> -			 pinctrl_find_gpio_range_from_pin(info->pctl, pin);
> +			 pinctrl_find_gpio_range_from_pin_nolock(info->pctl, pin);
>  	struct aml_gpio_bank *bank = gpio_chip_to_bank(range->gc);

[Severity: High]
This is a pre-existing issue, but as with aml_pmx_set_mux(), the return
value of pinctrl_find_gpio_range_from_pin_nolock() is immediately
dereferenced via range->gc. Should there be a check for NULL here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic