| 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
| ||
|
Date: Fri, 2 Feb 2024 19:09:39 +0800
From: "Aiqun Yu (Maria)" <quic_aiquny@...cinc.com>
To: <andersson@...nel.org>, <linus.walleij@...aro.org>
CC: <kernel@...cinc.com>, <linux-gpio@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v4] pinctrl: Add lock to ensure the state atomization
On 2/2/2024 6:58 PM, Maria Yu wrote:
> Currently pinctrl_select_state is an export symbol and don't have
> effective re-entrance protect design. During async probing of devices
> it's possible to end up in pinctrl_select_state() from multiple
> contexts simultaneously, so make it thread safe.
> More over, when the real racy happened, the system frequently have
> printk message like:
> "not freeing pin xx (xxx) as part of deactivating group xxx - it is
> already used for some other setting".
> Finally the system crashed after the flood log.
> Add per pinctrl lock to ensure the old state and new state transition
> atomization.
> Also move dev error print message outside the region with interrupts
> disabled.
> Use scoped guard to simplify the lock protection needed code.
>
> Fixes: 4198a9b57106 ("pinctrl: avoid reload of p state in list iteration")
> Signed-off-by: Maria Yu <quic_aiquny@...cinc.com>
> ---
forget to add change log here:
v4-v3 changed:
#include <linux/cleanup.h> is removed because of rebase.
others is same to v3.
> drivers/pinctrl/core.c | 143 +++++++++++++++++++++--------------------
> drivers/pinctrl/core.h | 2 +
> 2 files changed, 75 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index ee56856cb80c..1f7d001d4c1e 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1061,6 +1061,7 @@ static struct pinctrl *create_pinctrl(struct device *dev,
> p->dev = dev;
> INIT_LIST_HEAD(&p->states);
> INIT_LIST_HEAD(&p->dt_maps);
> + spin_lock_init(&p->lock);
>
> ret = pinctrl_dt_to_map(p, pctldev);
> if (ret < 0) {
> @@ -1257,93 +1258,95 @@ static void pinctrl_link_add(struct pinctrl_dev *pctldev,
> static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> {
> struct pinctrl_setting *setting, *setting2;
> - struct pinctrl_state *old_state = READ_ONCE(p->state);
> + struct pinctrl_state *old_state;
> int ret;
>
> - if (old_state) {
> - /*
> - * For each pinmux setting in the old state, forget SW's record
> - * of mux owner for that pingroup. Any pingroups which are
> - * still owned by the new state will be re-acquired by the call
> - * to pinmux_enable_setting() in the loop below.
> - */
> - list_for_each_entry(setting, &old_state->settings, node) {
> - if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
> - continue;
> - pinmux_disable_setting(setting);
> + scoped_guard(spinlock_irqsave, &p->lock) {
> + old_state = p->state;
> + if (old_state) {
> + /*
> + * For each pinmux setting in the old state, forget SW's record
> + * of mux owner for that pingroup. Any pingroups which are
> + * still owned by the new state will be re-acquired by the call
> + * to pinmux_enable_setting() in the loop below.
> + */
> + list_for_each_entry(setting, &old_state->settings, node) {
> + if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
> + continue;
> + pinmux_disable_setting(setting);
> + }
> }
> - }
> -
> - p->state = NULL;
>
> - /* Apply all the settings for the new state - pinmux first */
> - list_for_each_entry(setting, &state->settings, node) {
> - switch (setting->type) {
> - case PIN_MAP_TYPE_MUX_GROUP:
> - ret = pinmux_enable_setting(setting);
> - break;
> - case PIN_MAP_TYPE_CONFIGS_PIN:
> - case PIN_MAP_TYPE_CONFIGS_GROUP:
> - ret = 0;
> - break;
> - default:
> - ret = -EINVAL;
> - break;
> - }
> + p->state = NULL;
>
> - if (ret < 0)
> - goto unapply_new_state;
> + /* Apply all the settings for the new state - pinmux first */
> + list_for_each_entry(setting, &state->settings, node) {
> + switch (setting->type) {
> + case PIN_MAP_TYPE_MUX_GROUP:
> + ret = pinmux_enable_setting(setting);
> + break;
> + case PIN_MAP_TYPE_CONFIGS_PIN:
> + case PIN_MAP_TYPE_CONFIGS_GROUP:
> + ret = 0;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
>
> - /* Do not link hogs (circular dependency) */
> - if (p != setting->pctldev->p)
> - pinctrl_link_add(setting->pctldev, p->dev);
> - }
> + if (ret < 0)
> + goto unapply_new_state;
>
> - /* Apply all the settings for the new state - pinconf after */
> - list_for_each_entry(setting, &state->settings, node) {
> - switch (setting->type) {
> - case PIN_MAP_TYPE_MUX_GROUP:
> - ret = 0;
> - break;
> - case PIN_MAP_TYPE_CONFIGS_PIN:
> - case PIN_MAP_TYPE_CONFIGS_GROUP:
> - ret = pinconf_apply_setting(setting);
> - break;
> - default:
> - ret = -EINVAL;
> - break;
> + /* Do not link hogs (circular dependency) */
> + if (p != setting->pctldev->p)
> + pinctrl_link_add(setting->pctldev, p->dev);
> }
>
> - if (ret < 0) {
> - goto unapply_new_state;
> - }
> + /* Apply all the settings for the new state - pinconf after */
> + list_for_each_entry(setting, &state->settings, node) {
> + switch (setting->type) {
> + case PIN_MAP_TYPE_MUX_GROUP:
> + ret = 0;
> + break;
> + case PIN_MAP_TYPE_CONFIGS_PIN:
> + case PIN_MAP_TYPE_CONFIGS_GROUP:
> + ret = pinconf_apply_setting(setting);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
>
> - /* Do not link hogs (circular dependency) */
> - if (p != setting->pctldev->p)
> - pinctrl_link_add(setting->pctldev, p->dev);
> - }
> + if (ret < 0)
> + goto unapply_new_state;
>
> - p->state = state;
> + /* Do not link hogs (circular dependency) */
> + if (p != setting->pctldev->p)
> + pinctrl_link_add(setting->pctldev, p->dev);
> + }
>
> - return 0;
> + p->state = state;
> +
> + return 0;
>
> unapply_new_state:
> - dev_err(p->dev, "Error applying setting, reverse things back\n");
>
> - list_for_each_entry(setting2, &state->settings, node) {
> - if (&setting2->node == &setting->node)
> - break;
> - /*
> - * All we can do here is pinmux_disable_setting.
> - * That means that some pins are muxed differently now
> - * than they were before applying the setting (We can't
> - * "unmux a pin"!), but it's not a big deal since the pins
> - * are free to be muxed by another apply_setting.
> - */
> - if (setting2->type == PIN_MAP_TYPE_MUX_GROUP)
> - pinmux_disable_setting(setting2);
> + list_for_each_entry(setting2, &state->settings, node) {
> + if (&setting2->node == &setting->node)
> + break;
> + /*
> + * All we can do here is pinmux_disable_setting.
> + * That means that some pins are muxed differently now
> + * than they were before applying the setting (We can't
> + * "unmux a pin"!), but it's not a big deal since the pins
> + * are free to be muxed by another apply_setting.
> + */
> + if (setting2->type == PIN_MAP_TYPE_MUX_GROUP)
> + pinmux_disable_setting(setting2);
> + }
> }
>
> + dev_err(p->dev, "Error applying setting, reverse things back\n");
> /* There's no infinite recursive loop here because p->state is NULL */
> if (old_state)
> pinctrl_select_state(p, old_state);
> diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
> index 837fd5bd903d..6844edd38b4a 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>
> @@ -91,6 +92,7 @@ struct pinctrl {
> struct pinctrl_state *state;
> struct list_head dt_maps;
> struct kref users;
> + spinlock_t lock;
> };
>
> /**
>
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
--
Thx and BRs,
Aiqun(Maria) Yu
Powered by blists - more mailing lists