Re: [PATCH] pinmux: fix race causing mux_owner NULL with active mux_usecount

[Mukesh Ojha]

On Fri, Jun 20, 2025 at 10:13:24PM +0530, Mukesh Ojha wrote:
> Commit  ("pinmux: Use sequential access to access desc->pinmux data")

commit 5a3e85c3c397 ("pinmux: Use sequential access to access desc->pinmux data") 

> tried to address the issue when two client of the same gpio calls
> pinctrl_select_state() for the same functionality, was resulting in
> NULL pointer issue while accessing desc->mux_owner. However, issue
> was not completely fixed due to the way it was handled and it can
> still result in the same NULL pointer.
> 
> The issue occurs due to the following interleaving:
> 
>      cpu0 (process A)                   cpu1 (process B)
> 
>       pin_request() {                   pin_free() {
> 
>                                          mutex_lock()
>                                          desc->mux_usecount--; //becomes 0
>                                          ..
>                                          mutex_unlock()
> 
>   mutex_lock(desc->mux)
>   desc->mux_usecount++; // becomes 1
>   desc->mux_owner = owner;
>   mutex_unlock(desc->mux)
> 
>                                          mutex_lock(desc->mux)
>                                          desc->mux_owner = NULL;
>                                          mutex_unlock(desc->mux)
> 
> This sequence leads to a state where the pin appears to be in use
> (`mux_usecount == 1`) but has no owner (`mux_owner == NULL`), which can
> cause NULL pointer on next pin_request on the same pin.
> 
> Ensure that updates to mux_usecount and mux_owner are performed
> atomically under the same lock. Only clear mux_owner when mux_usecount
> reaches zero and no new owner has been assigned.
>

Fixes: 5a3e85c3c397 ("pinmux: Use sequential access to access desc->pinmux data")

I missed adding the commit SHA and the 'Fixes' tag. However, I’ll wait before sending the next version.

-Mukesh

> Signed-off-by: Mukesh Ojha <mukesh.ojha@xxxxxxxxxxxxxxxx>
> ---
>  drivers/pinctrl/pinmux.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 0743190da59e..1cea04d57ca2 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -235,19 +235,9 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  			desc->mux_usecount--;
>  			if (desc->mux_usecount)
>  				return NULL;
> -		}
> -	}
>  
> -	/*
> -	 * If there is no kind of request function for the pin we just assume
> -	 * we got it by default and proceed.
> -	 */
> -	if (gpio_range && ops->gpio_disable_free)
> -		ops->gpio_disable_free(pctldev, gpio_range, pin);
> -	else if (ops->free)
> -		ops->free(pctldev, pin);
> +		}
>  
> -	scoped_guard(mutex, &desc->mux_lock) {
>  		if (gpio_range) {
>  			owner = desc->gpio_owner;
>  			desc->gpio_owner = NULL;
> @@ -258,6 +248,15 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  		}
>  	}
>  
> +	/*
> +	 * If there is no kind of request function for the pin we just assume
> +	 * we got it by default and proceed.
> +	 */
> +	if (gpio_range && ops->gpio_disable_free)
> +		ops->gpio_disable_free(pctldev, gpio_range, pin);
> +	else if (ops->free)
> +		ops->free(pctldev, pin);
> +
>  	module_put(pctldev->owner);
>  
>  	return owner;
> -- 
> 2.34.1
>