When two client of the same gpio call pinctrl_select_state() for the
same functionality, we are seeing NULL pointer issue while accessing
desc->mux_owner.

Let's say two processes A, B executing in pin_request() for the same pin
and process A updates the desc->mux_usecount but not yet updated the
desc->mux_owner while process B see the desc->mux_usecount which got
updated by A path and further executes strcmp and while accessing
desc->mux_owner it crashes with NULL pointer.

Serialize the access to mux related setting with a spin lock.

	cpu0 (process A)			cpu1(process B)

pinctrl_select_state() {		  pinctrl_select_state() {
  pin_request() {				pin_request() {
  ...
						 ....
    } else {
         desc->mux_usecount++;
    						desc->mux_usecount && strcmp(desc->mux_owner, owner)) {

         if (desc->mux_usecount > 1)
               return 0;
         desc->mux_owner = owner;

  }						}

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
Changes in v2:
 - Used scoped_guard and renamed lock name as per suggestion from Linus.W .

 drivers/pinctrl/core.c   |   3 +
 drivers/pinctrl/core.h   |   2 +
 drivers/pinctrl/pinmux.c | 150 +++++++++++++++++++++------------------
 3 files changed, 86 insertions(+), 69 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4061890a1748..b00911421cf5 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -220,6 +220,9 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
 
 	/* Set owner */
 	pindesc->pctldev = pctldev;
+#ifdef CONFIG_PINMUX
+	spin_lock_init(&pindesc->mux_lock);
+#endif
 
 	/* Copy basic pin info */
 	if (pin->name) {
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 4e07707d2435..179e01dfacc2 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -12,6 +12,7 @@
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/radix-tree.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 #include <linux/pinctrl/machine.h>
@@ -177,6 +178,7 @@ struct pin_desc {
 	const char *mux_owner;
 	const struct pinctrl_setting_mux *mux_setting;
 	const char *gpio_owner;
+	spinlock_t mux_lock;
 #endif
 };
 
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 02033ea1c643..e4d535aabbb6 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -14,6 +14,7 @@
 
 #include <linux/array_size.h>
 #include <linux/ctype.h>
+#include <linux/cleanup.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -127,29 +128,31 @@ static int pin_request(struct pinctrl_dev *pctldev,
 	dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
 		pin, desc->name, owner);
 
-	if ((!gpio_range || ops->strict) &&
-	    desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
-		dev_err(pctldev->dev,
-			"pin %s already requested by %s; cannot claim for %s\n",
-			desc->name, desc->mux_owner, owner);
-		goto out;
-	}
+	scoped_guard(spinlock_irqsave, &desc->mux_lock) {
+		if ((!gpio_range || ops->strict) &&
+		     desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
+			dev_err(pctldev->dev,
+				"pin %s already requested by %s; cannot claim for %s\n",
+				desc->name, desc->mux_owner, owner);
+			goto out;
+		}
 
-	if ((gpio_range || ops->strict) && desc->gpio_owner) {
-		dev_err(pctldev->dev,
-			"pin %s already requested by %s; cannot claim for %s\n",
-			desc->name, desc->gpio_owner, owner);
-		goto out;
-	}
+		if ((gpio_range || ops->strict) && desc->gpio_owner) {
+			dev_err(pctldev->dev,
+				"pin %s already requested by %s; cannot claim for %s\n",
+				desc->name, desc->gpio_owner, owner);
+			goto out;
+		}
 
-	if (gpio_range) {
-		desc->gpio_owner = owner;
-	} else {
-		desc->mux_usecount++;
-		if (desc->mux_usecount > 1)
-			return 0;
+		if (gpio_range) {
+			desc->gpio_owner = owner;
+		} else {
+			desc->mux_usecount++;
+			if (desc->mux_usecount > 1)
+				return 0;
 
-		desc->mux_owner = owner;
+			desc->mux_owner = owner;
+		}
 	}
 
 	/* Let each pin increase references to this module */
@@ -178,12 +181,14 @@ static int pin_request(struct pinctrl_dev *pctldev,
 
 out_free_pin:
 	if (status) {
-		if (gpio_range) {
-			desc->gpio_owner = NULL;
-		} else {
-			desc->mux_usecount--;
-			if (!desc->mux_usecount)
-				desc->mux_owner = NULL;
+		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
+			if (gpio_range) {
+				desc->gpio_owner = NULL;
+			} else {
+				desc->mux_usecount--;
+				if (!desc->mux_usecount)
+					desc->mux_owner = NULL;
+			}
 		}
 	}
 out:
@@ -223,11 +228,13 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 		/*
 		 * A pin should not be freed more times than allocated.
 		 */
-		if (WARN_ON(!desc->mux_usecount))
-			return NULL;
-		desc->mux_usecount--;
-		if (desc->mux_usecount)
-			return NULL;
+		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
+			if (WARN_ON(!desc->mux_usecount))
+				return NULL;
+			desc->mux_usecount--;
+			if (desc->mux_usecount)
+				return NULL;
+		}
 	}
 
 	/*
@@ -239,13 +246,15 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 	else if (ops->free)
 		ops->free(pctldev, pin);
 
-	if (gpio_range) {
-		owner = desc->gpio_owner;
-		desc->gpio_owner = NULL;
-	} else {
-		owner = desc->mux_owner;
-		desc->mux_owner = NULL;
-		desc->mux_setting = NULL;
+	scoped_guard(spinlock_irqsave, &desc->mux_lock) {
+		if (gpio_range) {
+			owner = desc->gpio_owner;
+			desc->gpio_owner = NULL;
+		} else {
+			owner = desc->mux_owner;
+			desc->mux_owner = NULL;
+			desc->mux_setting = NULL;
+		}
 	}
 
 	module_put(pctldev->owner);
@@ -608,40 +617,43 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 		if (desc == NULL)
 			continue;
 
-		if (desc->mux_owner &&
-		    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
-			is_hog = true;
-
-		if (pmxops->strict) {
-			if (desc->mux_owner)
-				seq_printf(s, "pin %d (%s): device %s%s",
-					   pin, desc->name, desc->mux_owner,
+		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
+			if (desc->mux_owner &&
+			    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
+				is_hog = true;
+
+			if (pmxops->strict) {
+				if (desc->mux_owner)
+					seq_printf(s, "pin %d (%s): device %s%s",
+						   pin, desc->name, desc->mux_owner,
+						   is_hog ? " (HOG)" : "");
+				else if (desc->gpio_owner)
+					seq_printf(s, "pin %d (%s): GPIO %s",
+						   pin, desc->name, desc->gpio_owner);
+				else
+					seq_printf(s, "pin %d (%s): UNCLAIMED",
+						   pin, desc->name);
+			} else {
+				/* For non-strict controllers */
+				seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name,
+					   desc->mux_owner ? desc->mux_owner
+					   : "(MUX UNCLAIMED)",
+					   desc->gpio_owner ? desc->gpio_owner
+					   : "(GPIO UNCLAIMED)",
 					   is_hog ? " (HOG)" : "");
-			else if (desc->gpio_owner)
-				seq_printf(s, "pin %d (%s): GPIO %s",
-					   pin, desc->name, desc->gpio_owner);
+			}
+
+			/* If mux: print function+group claiming the pin */
+			if (desc->mux_setting)
+				seq_printf(s, " function %s group %s\n",
+					   pmxops->get_function_name(pctldev,
+						desc->mux_setting->func),
+					   pctlops->get_group_name(pctldev,
+						desc->mux_setting->group));
 			else
-				seq_printf(s, "pin %d (%s): UNCLAIMED",
-					   pin, desc->name);
-		} else {
-			/* For non-strict controllers */
-			seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name,
-				   desc->mux_owner ? desc->mux_owner
-				   : "(MUX UNCLAIMED)",
-				   desc->gpio_owner ? desc->gpio_owner
-				   : "(GPIO UNCLAIMED)",
-				   is_hog ? " (HOG)" : "");
-		}
+				seq_putc(s, '\n');
 
-		/* If mux: print function+group claiming the pin */
-		if (desc->mux_setting)
-			seq_printf(s, " function %s group %s\n",
-				   pmxops->get_function_name(pctldev,
-					desc->mux_setting->func),
-				   pctlops->get_group_name(pctldev,
-					desc->mux_setting->group));
-		else
-			seq_putc(s, '\n');
+		}
 	}
 
 	mutex_unlock(&pctldev->mutex);
-- 
2.34.1

On Sat, Sep 28, 2024 at 01:57:41AM +0530, Wasim Nazir wrote:

Hi Wasim,

Thanks for the review, there looks to be problem with your email client
while replying, please fix.

Please find my reply inline..

> From: Mukesh Ojha <quic_mojha@quicinc.com>
> 
> When two client of the same gpio call pinctrl_select_state() for the
> same functionality, we are seeing NULL pointer issue while accessing
> desc->mux_owner.
> 
> Let's say two processes A, B executing in pin_request() for the same pin
> and process A updates the desc->mux_usecount but not yet updated the
> desc->mux_owner while process B see the desc->mux_usecount which got
> updated by A path and further executes strcmp and while accessing
> desc->mux_owner it crashes with NULL pointer.
> 
> Serialize the access to mux related setting with a spin lock.
> 
> 	cpu0 (process A)			cpu1(process B)
> 
> pinctrl_select_state() {		  pinctrl_select_state() {
>   pin_request() {				pin_request() {
>   ...
> 						 ....
>     } else {
>          desc->mux_usecount++;
>     						desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
> 
>          if (desc->mux_usecount > 1)
>                return 0;
>          desc->mux_owner = owner;
> 
>   }						}
> 
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Changes in v2:
>  - Used scoped_guard and renamed lock name as per suggestion from Linus.W .
> 
>  drivers/pinctrl/core.c   |   3 +
>  drivers/pinctrl/core.h   |   2 +
>  drivers/pinctrl/pinmux.c | 150 +++++++++++++++++++++------------------
>  3 files changed, 86 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 4061890a1748..b00911421cf5 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -220,6 +220,9 @@ static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
>  
>  	/* Set owner */
>  	pindesc->pctldev = pctldev;
> +#ifdef CONFIG_PINMUX
> +	spin_lock_init(&pindesc->mux_lock);
> +#endif
>  
>  	/* Copy basic pin info */
>  	if (pin->name) {
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 4e07707d2435..179e01dfacc2 100644
> --- a/drivers/pinctrl/core.h
> +++ b/drivers/pinctrl/core.h
> @@ -12,6 +12,7 @@
>  #include <linux/list.h>
>  #include <linux/mutex.h>
>  #include <linux/radix-tree.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  
>  #include <linux/pinctrl/machine.h>
> @@ -177,6 +178,7 @@ struct pin_desc {
>  	const char *mux_owner;
>  	const struct pinctrl_setting_mux *mux_setting;
>  	const char *gpio_owner;
> +	spinlock_t mux_lock;
>  #endif
>  };
>  
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 02033ea1c643..e4d535aabbb6 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/array_size.h>
>  #include <linux/ctype.h>
> +#include <linux/cleanup.h>
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -127,29 +128,31 @@ static int pin_request(struct pinctrl_dev *pctldev,
>  	dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
>  		pin, desc->name, owner);
>  
> -	if ((!gpio_range || ops->strict) &&
> -	    desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
> -		dev_err(pctldev->dev,
> -			"pin %s already requested by %s; cannot claim for %s\n",
> -			desc->name, desc->mux_owner, owner);
> -		goto out;
> -	}
> +	scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> > Any reason to use spinlock_irqsave and not mutex ? If spinlock is needed
> > can we guard only the mux variables and exclude the printk
> > as the same lock is used with pin_show API too.

Good point, using spinlock_irqsave can make things worse and this can
stuck while writing log to console.

I remember now, why i did this,
v3: https://lore.kernel.org/lkml/20231225082305.12343-1-quic_aiquny@quicinc.com/

Later same patch in v4 was causing sleep while atomic issue.,
https://lore.kernel.org/lkml/8376074.NyiUUSuA9g@steina-w/

I better be correcting this to mutex here, should also have to increase
the range of this lock to cover mux-setting as well.
> 
> > Moreover, is the mux_usecount variable in pinmux_can_be_used_for_gpio()
> > needed guarding ?

It's a miss, thanks.,

> +		if ((!gpio_range || ops->strict) &&
> +		     desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
> +			dev_err(pctldev->dev,
> +				"pin %s already requested by %s; cannot claim for %s\n",
> +				desc->name, desc->mux_owner, owner);
> +			goto out;
> +		}
>  
> -	if ((gpio_range || ops->strict) && desc->gpio_owner) {
> -		dev_err(pctldev->dev,
> -			"pin %s already requested by %s; cannot claim for %s\n",
> -			desc->name, desc->gpio_owner, owner);
> -		goto out;
> -	}
> +		if ((gpio_range || ops->strict) && desc->gpio_owner) {
> +			dev_err(pctldev->dev,
> +				"pin %s already requested by %s; cannot claim for %s\n",
> +				desc->name, desc->gpio_owner, owner);
> +			goto out;
> +		}
>  
> -	if (gpio_range) {
> -		desc->gpio_owner = owner;
> -	} else {
> -		desc->mux_usecount++;
> -		if (desc->mux_usecount > 1)
> -			return 0;
> +		if (gpio_range) {
> +			desc->gpio_owner = owner;
> +		} else {
> +			desc->mux_usecount++;
> +			if (desc->mux_usecount > 1)
> +				return 0;
>  
> -		desc->mux_owner = owner;
> +			desc->mux_owner = owner;
> +		}
>  	}
>  
>  	/* Let each pin increase references to this module */
> @@ -178,12 +181,14 @@ static int pin_request(struct pinctrl_dev *pctldev,
>  
>  out_free_pin:
>  	if (status) {
> -		if (gpio_range) {
> -			desc->gpio_owner = NULL;
> -		} else {
> -			desc->mux_usecount--;
> -			if (!desc->mux_usecount)
> -				desc->mux_owner = NULL;
> +		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +			if (gpio_range) {
> +				desc->gpio_owner = NULL;
> +			} else {
> +				desc->mux_usecount--;
> +				if (!desc->mux_usecount)
> +					desc->mux_owner = NULL;
> +			}
>  		}
>  	}
>  out:
> @@ -223,11 +228,13 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  		/*
>  		 * A pin should not be freed more times than allocated.
>  		 */
> -		if (WARN_ON(!desc->mux_usecount))
> -			return NULL;
> -		desc->mux_usecount--;
> -		if (desc->mux_usecount)
> -			return NULL;
> +		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +			if (WARN_ON(!desc->mux_usecount))
> +				return NULL;
> +			desc->mux_usecount--;
> +			if (desc->mux_usecount)
> +				return NULL;
> +		}
>  	}
>  
>  	/*
> @@ -239,13 +246,15 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  	else if (ops->free)
>  		ops->free(pctldev, pin);
>  
> -	if (gpio_range) {
> -		owner = desc->gpio_owner;
> -		desc->gpio_owner = NULL;
> -	} else {
> -		owner = desc->mux_owner;
> -		desc->mux_owner = NULL;
> -		desc->mux_setting = NULL;
> +	scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +		if (gpio_range) {
> +			owner = desc->gpio_owner;
> +			desc->gpio_owner = NULL;
> +		} else {
> +			owner = desc->mux_owner;
> +			desc->mux_owner = NULL;
> +			desc->mux_setting = NULL;
> +		}
>  	}
>  
>  	module_put(pctldev->owner);
> @@ -608,40 +617,43 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
>  		if (desc == NULL)
>  			continue;
>  
> -		if (desc->mux_owner &&
> -		    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
> -			is_hog = true;
> -
> -		if (pmxops->strict) {
> -			if (desc->mux_owner)
> -				seq_printf(s, "pin %d (%s): device %s%s",
> -					   pin, desc->name, desc->mux_owner,
> +		scoped_guard(spinlock_irqsave, &desc->mux_lock) {
> +			if (desc->mux_owner &&
> +			    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
> +				is_hog = true;
> +
> +			if (pmxops->strict) {
> +				if (desc->mux_owner)
> +					seq_printf(s, "pin %d (%s): device %s%s",
> +						   pin, desc->name, desc->mux_owner,
> +						   is_hog ? " (HOG)" : "");
> +				else if (desc->gpio_owner)
> +					seq_printf(s, "pin %d (%s): GPIO %s",
> +						   pin, desc->name, desc->gpio_owner);
> +				else
> +					seq_printf(s, "pin %d (%s): UNCLAIMED",
> +						   pin, desc->name);
> +			} else {
> +				/* For non-strict controllers */
> +				seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name,
> +					   desc->mux_owner ? desc->mux_owner
> +					   : "(MUX UNCLAIMED)",
> +					   desc->gpio_owner ? desc->gpio_owner
> +					   : "(GPIO UNCLAIMED)",
>  					   is_hog ? " (HOG)" : "");
> -			else if (desc->gpio_owner)
> -				seq_printf(s, "pin %d (%s): GPIO %s",
> -					   pin, desc->name, desc->gpio_owner);
> +			}
> +
> +			/* If mux: print function+group claiming the pin */
> +			if (desc->mux_setting)
> +				seq_printf(s, " function %s group %s\n",
> +					   pmxops->get_function_name(pctldev,
> +						desc->mux_setting->func),
> +					   pctlops->get_group_name(pctldev,
> +						desc->mux_setting->group));
>  			else
> -				seq_printf(s, "pin %d (%s): UNCLAIMED",
> -					   pin, desc->name);
> -		} else {
> -			/* For non-strict controllers */
> -			seq_printf(s, "pin %d (%s): %s %s%s", pin, desc->name,
> -				   desc->mux_owner ? desc->mux_owner
> -				   : "(MUX UNCLAIMED)",
> -				   desc->gpio_owner ? desc->gpio_owner
> -				   : "(GPIO UNCLAIMED)",
> -				   is_hog ? " (HOG)" : "");
> -		}
> +				seq_putc(s, '\n');
>  
> -		/* If mux: print function+group claiming the pin */
> -		if (desc->mux_setting)
> -			seq_printf(s, " function %s group %s\n",
> -				   pmxops->get_function_name(pctldev,
> -					desc->mux_setting->func),
> -				   pctlops->get_group_name(pctldev,
> -					desc->mux_setting->group));
> -		else
> -			seq_putc(s, '\n');
> +		}
> > Since we are introducing a lock, do we need to guard mux-settings too ?

Yes, we should, I would need to take care at other places as well.

-Mukesh
>  	}
>  
>  	mutex_unlock(&pctldev->mutex);
> 
> -- 
> 2.34.1
> 
>