| 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: Tue, 20 Feb 2024 14:13:06 +0800
From: "Aiqun Yu (Maria)" <quic_aiquny@...cinc.com>
To: Alexander Stein <alexander.stein@...tq-group.com>, <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/9/2024 4:37 PM, Alexander Stein wrote:
> Am Freitag, 2. Februar 2024, 11:58:54 CET schrieb Maria Yu:
>> 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>
>> ---
>> 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
>
> This breaks pinctrl-imx on imx8qxp:
>
> [ 1.170727] imx8qxp-pinctrl system-controller:pinctrl: initialized IMX
> pinctrl driver
> [ 1.283968] BUG: sleeping function called from invalid context at kernel/
> locking/mutex.c:283
> [ 1.292089] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 70,
> name: kworker/u16:4
> [ 1.300341] preempt_count: 1, expected: 0
> [ 1.304337] RCU nest depth: 0, expected: 0
> [ 1.308423] CPU: 2 PID: 70 Comm: kworker/u16:4 Not tainted 6.8.0-rc3-
> next-20240209+ #2267 0b2aeebc4d64f1aef3abdd5fede2a9b5162eb867
> [ 1.320148] Hardware name: TQ-Systems i.MX8QXP TQMa8XQP on MBa8Xx (DT)
> [ 1.326667] Workqueue: events_unbound deferred_probe_work_func
> [ 1.332486] Call trace:
> [ 1.334918] dump_backtrace+0x90/0x10c
> [ 1.338653] show_stack+0x14/0x1c
> [ 1.341954] dump_stack_lvl+0x6c/0x80
> [ 1.345603] dump_stack+0x14/0x1c
> [ 1.348904] __might_resched+0x108/0x160
> [ 1.352813] __might_sleep+0x58/0xb0
> [ 1.356375] mutex_lock+0x20/0x74
> [ 1.359676] imx_scu_call_rpc+0x44/0x2e8
> [ 1.363586] imx_pinconf_set_scu+0x84/0x150
> [ 1.367756] imx_pinconf_set+0x48/0x7c
> [ 1.371491] pinconf_apply_setting+0x90/0x110
> [ 1.375835] pinctrl_commit_state+0xcc/0x28c
> [ 1.380092] pinctrl_select_state+0x18/0x28
> [ 1.384262] pinctrl_bind_pins+0x1e4/0x26c
> [ 1.388345] really_probe+0x60/0x3e0
> [ 1.391907] __driver_probe_device+0x84/0x198
> [ 1.396251] driver_probe_device+0x38/0x150
> [ 1.400421] __device_attach_driver+0xcc/0x194
> [ 1.404851] bus_for_each_drv+0x80/0xdc
> [ 1.408674] __device_attach+0x9c/0x1d0
> [ 1.412496] device_initial_probe+0x10/0x18
> [ 1.416666] bus_probe_device+0xa4/0xa8
> [ 1.420489] deferred_probe_work_func+0x9c/0xe8
> [ 1.425006] process_one_work+0x14c/0x40c
> [ 1.429002] worker_thread+0x304/0x414
> [ 1.432738] kthread+0xf4/0x100
> [ 1.435866] ret_from_fork+0x10/0x20
>
> With this commit pin_config_set callbacks need to be atomic suddenly which is
> a no-go for any device attached to i2c or spi and in this case IPC RPC.
> Once reverted systems start normally again.
Thanks for the info. It is a valid scenario of your report. I will also
re-visit this.
>
> Best regards,
> Alexander
>
--
Thx and BRs,
Aiqun(Maria) Yu
Powered by blists - more mailing lists