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

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

[Mukesh Ojha]

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")
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/pinctrl/pinmux.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 0743190da59e..2c31e7f2a27a 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -236,18 +236,7 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 			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 +247,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;

---
base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
change-id: 20250708-pinmux-race-fix-32a1ef840941

Best regards,
-- 
Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

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

[Linus Walleij]

On Tue, Jul 8, 2025 at 9:59 AM Mukesh Ojha <mukesh.ojha@oss.qualcomm.com> wrote:

> 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")
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

Thanks Mukesh, patch applied!

Yours,
Linus Walleij