[PATCH v3] pinmux: Use sequential access to access desc->pinmux data

From: Mukesh Ojha
Date: Mon Oct 14 2024 - 15:30:06 EST

Next message: Abhinav Kumar: "Re: [PATCH v2 2/2] drm/msm/dsi: fix 32-bit signed integer extension in pclk_rate calculation"
Previous message: Jonathan Corbet: "Re: [PATCH] docs: update sysfs docs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 mutex 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@xxxxxxxxxxx>
---
Changes in v3:
- Used mutex instead of spinlock as per comment from Wasim.
- Covered one more function pinmux_can_be_used_for_gpio() where
the lock should be taken as mux->setting is being accessed
as per comment from Wasim.

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 | 1 +
drivers/pinctrl/pinmux.c | 173 ++++++++++++++++++++++-----------------
3 files changed, 100 insertions(+), 77 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 4061890a1748..b3eec63c00ba 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
+ mutex_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..d6c24978e708 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -177,6 +177,7 @@ struct pin_desc {
const char *mux_owner;
const struct pinctrl_setting_mux *mux_setting;
const char *gpio_owner;
+ struct mutex mux_lock;
#endif
};

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 02033ea1c643..0743190da59e 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>
@@ -93,6 +94,7 @@ bool pinmux_can_be_used_for_gpio(struct pinctrl_dev *pctldev, unsigned int pin)
if (!desc || !ops)
return true;

+ guard(mutex)(&desc->mux_lock);
if (ops->strict && desc->mux_usecount)
return false;

@@ -127,29 +129,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(mutex, &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 +182,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(mutex, &desc->mux_lock) {
+ if (gpio_range) {
+ desc->gpio_owner = NULL;
+ } else {
+ desc->mux_usecount--;
+ if (!desc->mux_usecount)
+ desc->mux_owner = NULL;
+ }
}
}
out:
@@ -219,15 +225,17 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
return NULL;
}

- if (!gpio_range) {
- /*
- * 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(mutex, &desc->mux_lock) {
+ if (!gpio_range) {
+ /*
+ * 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;
+ }
}

/*
@@ -239,13 +247,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(mutex, &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);
@@ -458,7 +468,8 @@ int pinmux_enable_setting(const struct pinctrl_setting *setting)
pins[i]);
continue;
}
- desc->mux_setting = &(setting->data.mux);
+ scoped_guard(mutex, &desc->mux_lock)
+ desc->mux_setting = &(setting->data.mux);
}

ret = ops->set_mux(pctldev, setting->data.mux.func,
@@ -472,8 +483,10 @@ int pinmux_enable_setting(const struct pinctrl_setting *setting)
err_set_mux:
for (i = 0; i < num_pins; i++) {
desc = pin_desc_get(pctldev, pins[i]);
- if (desc)
- desc->mux_setting = NULL;
+ if (desc) {
+ scoped_guard(mutex, &desc->mux_lock)
+ desc->mux_setting = NULL;
+ }
}
err_pin_request:
/* On error release all taken pins */
@@ -492,6 +505,7 @@ void pinmux_disable_setting(const struct pinctrl_setting *setting)
unsigned int num_pins = 0;
int i;
struct pin_desc *desc;
+ bool is_equal;

if (pctlops->get_group_pins)
ret = pctlops->get_group_pins(pctldev, setting->data.mux.group,
@@ -517,7 +531,10 @@ void pinmux_disable_setting(const struct pinctrl_setting *setting)
pins[i]);
continue;
}
- if (desc->mux_setting == &(setting->data.mux)) {
+ scoped_guard(mutex, &desc->mux_lock)
+ is_equal = (desc->mux_setting == &(setting->data.mux));
+
+ if (is_equal) {
pin_free(pctldev, pins[i], NULL);
} else {
const char *gname;
@@ -608,40 +625,42 @@ 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(mutex, &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

Next message: Abhinav Kumar: "Re: [PATCH v2 2/2] drm/msm/dsi: fix 32-bit signed integer extension in pclk_rate calculation"
Previous message: Jonathan Corbet: "Re: [PATCH] docs: update sysfs docs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]