Re: [PATCH] gpio: shared-proxy: Fix race between concurrent first users

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: shared-proxy: integer underflow of votecnt on proxy free error path
- [Low] gpio: shared-proxy: spurious warning and logic bypass in set_config
--

commit 2a16d83da9ca16fcf3fc8f06a64d08bb431d1954
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

gpio: shared-proxy: Fix race between concurrent first users

This commit addresses a race condition where concurrent consumers
requesting a shared proxy line could both observe the use count as 2
before any configuration occurs, resulting in a failure. It resolves
this by introducing a configuration counter and a per-proxy flag to
track actual configuration changes rather than just requests.

> diff --git a/drivers/gpio/gpio-shared-proxy.c b/drivers/gpio/gpio-shared-proxy.c
> index bc69b8729d196..0d9ef98a4229e 100644
> --- a/drivers/gpio/gpio-shared-proxy.c
> +++ b/drivers/gpio/gpio-shared-proxy.c

[ ... ]

> @@ -117,6 +141,11 @@ static void gpio_shared_proxy_free(struct gpio_chip *gc, unsigned int offset)
>  
>  	proxy->shared_desc->usecnt--;
>  
> +	if (proxy->configured) {
> +		proxy->configured = false;
> +		shared_desc->cfgcnt--;
> +	}
> +

[Severity: High]
If the hardware write fails in the earlier gpio_shared_proxy_set_unlocked()
call inside this function, proxy->voted_change remains true:

	if (proxy->voted_change) {
		ret = gpio_shared_proxy_set_unlocked(proxy, shared_desc->def_val);
		if (ret)
			dev_err(proxy->dev, ...);
	}

When we unconditionally clear proxy->configured and decrement cfgcnt
here despite the failure, can this allow a new proxy to bypass checks and
reset the global votecnt to 0?

If cfgcnt drops to 0 and votecnt is reset, a later reuse of this proxy
could see voted_change still set. If votecnt is 0 at that point,
calling gpio_shared_proxy_set_unlocked() again could skip the hardware
write but decrement votecnt, causing it to underflow to UINT_MAX.
Could we ensure voted_change is cleared, or configured is not cleared
if the write fails?

[ ... ]

> diff --git a/drivers/gpio/gpiolib-shared.h b/drivers/gpio/gpiolib-shared.h
> index 618756f6c6aaf..ae10506b7029f 100644
> --- a/drivers/gpio/gpiolib-shared.h
> +++ b/drivers/gpio/gpiolib-shared.h
> @@ -41,6 +41,7 @@ struct gpio_shared_desc {
>  	struct gpio_desc *desc;
>  	unsigned long cfg;
>  	unsigned int usecnt;
> +	unsigned int cfgcnt;
>  	unsigned int votecnt;
>  	int def_val;
>  	struct mutex mutex; /* serializes all proxy operations on this descriptor */

[Severity: Low]
Since concurrent probes are now tracked using cfgcnt rather than usecnt
to determine if the line has been initialized, does
gpio_shared_proxy_set_config() also need to be updated?

If two consumers concurrently request the line but haven't configured it,
usecnt is 2. If one of them then calls gpio_shared_proxy_set_config(),
it still checks usecnt > 1. This could cause a spurious conflict warning
if the requested configuration differs from the uninitialized state, or it
might bypass the hardware configuration entirely if they match, erroneously
assuming another consumer already applied it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015122740.971792-1-shengchao.guo@oss.qualcomm.com?part=1