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.

	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>
---
 drivers/pinctrl/core.c   |  3 +++
 drivers/pinctrl/core.h   |  2 ++
 drivers/pinctrl/pinmux.c | 21 +++++++++++++++++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 314ab93d7691..e451af82eff2 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->lock);
+#endif
 
 	/* Copy basic pin info */
 	if (pin->name) {
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 4e07707d2435..afc651ce3985 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 lock;
 #endif
 };
 
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index aae71a37219b..75735618646d 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -115,6 +115,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 	struct pin_desc *desc;
 	const struct pinmux_ops *ops = pctldev->desc->pmxops;
 	int status = -EINVAL;
+	unsigned long flags;
 
 	desc = pin_desc_get(pctldev, pin);
 	if (desc == NULL) {
@@ -127,6 +128,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 	dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
 		pin, desc->name, owner);
 
+	spin_lock_irqsave(&desc->lock, flags);
 	if ((!gpio_range || ops->strict) &&
 	    desc->mux_usecount && strcmp(desc->mux_owner, owner)) {
 		dev_err(pctldev->dev,
@@ -151,6 +153,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 
 		desc->mux_owner = owner;
 	}
+	spin_unlock_irqrestore(&desc->lock, flags);
 
 	/* Let each pin increase references to this module */
 	if (!try_module_get(pctldev->owner)) {
@@ -178,6 +181,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 
 out_free_pin:
 	if (status) {
+		spin_lock_irqsave(&desc->lock, flags);
 		if (gpio_range) {
 			desc->gpio_owner = NULL;
 		} else {
@@ -185,6 +189,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
 			if (!desc->mux_usecount)
 				desc->mux_owner = NULL;
 		}
+		spin_unlock_irqrestore(&desc->lock, flags);
 	}
 out:
 	if (status)
@@ -211,6 +216,7 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 	const struct pinmux_ops *ops = pctldev->desc->pmxops;
 	struct pin_desc *desc;
 	const char *owner;
+	unsigned long flags;
 
 	desc = pin_desc_get(pctldev, pin);
 	if (desc == NULL) {
@@ -223,11 +229,13 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 		/*
 		 * A pin should not be freed more times than allocated.
 		 */
+		spin_lock_irqsave(&desc->lock, flags);
 		if (WARN_ON(!desc->mux_usecount))
-			return NULL;
+			goto out;
 		desc->mux_usecount--;
 		if (desc->mux_usecount)
-			return NULL;
+			goto out;
+		spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
 	/*
@@ -239,6 +247,7 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 	else if (ops->free)
 		ops->free(pctldev, pin);
 
+	spin_lock_irqsave(&desc->lock, flags);
 	if (gpio_range) {
 		owner = desc->gpio_owner;
 		desc->gpio_owner = NULL;
@@ -247,10 +256,14 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 		desc->mux_owner = NULL;
 		desc->mux_setting = NULL;
 	}
+	spin_unlock_irqrestore(&desc->lock, flags);
 
 	module_put(pctldev->owner);
 
 	return owner;
+out:
+	spin_unlock_irqrestore(&desc->lock, flags);
+	return NULL;
 }
 
 /**
@@ -586,6 +599,7 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 	const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
 	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
 	unsigned int i, pin;
+	unsigned long flags;
 
 	if (!pmxops)
 		return 0;
@@ -611,6 +625,7 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 		if (desc == NULL)
 			continue;
 
+		spin_lock_irqsave(&desc->lock, flags);
 		if (desc->mux_owner &&
 		    !strcmp(desc->mux_owner, pinctrl_dev_get_name(pctldev)))
 			is_hog = true;
@@ -645,6 +660,8 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 					desc->mux_setting->group));
 		else
 			seq_putc(s, '\n');
+
+		spin_unlock_irqrestore(&desc->lock, flags);
 	}
 
 	mutex_unlock(&pctldev->mutex);
-- 
2.34.1

On Thu, Sep 12, 2024 at 4:25 PM Mukesh Ojha <quic_mojha@quicinc.com> wrote:

> 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.

Uh-oh it looks like a very real issue, weird that we didn't run into
it earlier.

I guess we were not parallelizing probe so much in the past so it
didn't happen for that reason.

>         /* Set owner */
>         pindesc->pctldev = pctldev;
> +#ifdef CONFIG_PINMUX
> +       spin_lock_init(&pindesc->lock);
> +#endif

Can we rename it "mux_lock" so it is clear what it is locking?

> @@ -115,6 +115,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
>         struct pin_desc *desc;
>         const struct pinmux_ops *ops = pctldev->desc->pmxops;
>         int status = -EINVAL;
> +       unsigned long flags;
>
>         desc = pin_desc_get(pctldev, pin);
>         if (desc == NULL) {
> @@ -127,6 +128,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
>         dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n",
>                 pin, desc->name, owner);
>
> +       spin_lock_irqsave(&desc->lock, flags);

Could you please rewrite all of these using scoped guards as that
avoids a lot of possible bugs?

#include <linux/cleanup.h>

guard(spinlock_irqsave)(&desc->mux_lock);

This means the lock will be released when you exit the
function .

tighter locks around a block of code are possible with:
scoped_guard(spinlock_irqsave, &desc->mux_lock) { ... }

It also removes the need to define a flags variable.

Yours,
Linus Walleij