| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zvf7+Ll7sIwiYVsv@hu-mojha-hyd.qualcomm.com>
Date: Sat, 28 Sep 2024 18:22:08 +0530
From: Mukesh Ojha <quic_mojha@...cinc.com>
To: Wasim Nazir <quic_wasimn@...cinc.com>
CC: Linus Walleij <linus.walleij@...aro.org>, <linux-gpio@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] pinmux: Use sequential access to access desc->pinmux
data
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@...cinc.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@...cinc.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
>
>
Powered by blists - more mailing lists