| 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: <32c76177-83c1-48c5-8198-b7347b83d5db@samsung.com>
Date: Mon, 8 Jan 2024 14:03:15 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Bartosz Golaszewski <brgl@...ev.pl>, Linus Walleij
<linus.walleij@...aro.org>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, Bartosz
Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v2 3/3] gpiolib: pin GPIO devices in place during
descriptor lookup
On 02.01.2024 16:59, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> There's time between when we locate the relevant descriptor during
> lookup and when we actually take the reference to its parent GPIO
> device where - if the GPIO device in question is removed - we'll end up
> with a dangling pointer to freed memory. Make sure devices cannot be
> removed until we hold a new reference to the device.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
This patch landed in linux-next as commit db660b9a9f86 ("gpiolib: pin
GPIO devices in place during descriptor lookup"). Unfortunately it
introduces a following lock-dep warning:
============================================
WARNING: possible recursive locking detected
6.7.0-rc7-00062-gdb660b9a9f86 #7819 Not tainted
--------------------------------------------
kworker/u4:2/27 is trying to acquire lock:
c13f4e1c (gpio_devices_sem){++++}-{3:3}, at: gpio_device_find+0x30/0x94
but task is already holding lock:
c13f4e1c (gpio_devices_sem){++++}-{3:3}, at:
gpiod_find_and_request+0x44/0x594
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(gpio_devices_sem);
lock(gpio_devices_sem);
*** DEADLOCK ***
May be due to missing lock nesting notation
4 locks held by kworker/u4:2/27:
#0: c1c06ca8 ((wq_completion)events_unbound){+.+.}-{0:0}, at:
process_one_work+0x148/0x608
#1: e093df20 ((work_completion)(&entry->work)){+.+.}-{0:0}, at:
process_one_work+0x148/0x608
#2: c1f3048c (&dev->mutex){....}-{3:3}, at:
__driver_attach_async_helper+0x38/0xec
#3: c13f4e1c (gpio_devices_sem){++++}-{3:3}, at:
gpiod_find_and_request+0x44/0x594
stack backtrace:
CPU: 0 PID: 27 Comm: kworker/u4:2 Not tainted
6.7.0-rc7-00062-gdb660b9a9f86 #7819
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound async_run_entry_fn
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from __lock_acquire+0x1300/0x2984
__lock_acquire from lock_acquire+0x130/0x37c
lock_acquire from down_read+0x44/0x224
down_read from gpio_device_find+0x30/0x94
gpio_device_find from of_get_named_gpiod_flags+0xa4/0x3a8
of_get_named_gpiod_flags from of_find_gpio+0x80/0x168
of_find_gpio from gpiod_find_and_request+0x120/0x594
gpiod_find_and_request from gpiod_get_optional+0x54/0x90
gpiod_get_optional from reg_fixed_voltage_probe+0x200/0x400
reg_fixed_voltage_probe from platform_probe+0x5c/0xb8
platform_probe from really_probe+0xe0/0x400
really_probe from __driver_probe_device+0x9c/0x1f0
__driver_probe_device from driver_probe_device+0x30/0xc0
driver_probe_device from __driver_attach_async_helper+0x54/0xec
__driver_attach_async_helper from async_run_entry_fn+0x40/0x154
async_run_entry_fn from process_one_work+0x204/0x608
process_one_work from worker_thread+0x1e0/0x498
worker_thread from kthread+0x104/0x138
kthread from ret_from_fork+0x14/0x28
Exception stack(0xe093dfb0 to 0xe093dff8)
...
Taking gpio_devices_sem more than once for reading is safe, but it looks
that it needs some lock-dep annotations to to make it happy and avoid
the above warning.
> ---
> drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
> 1 file changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 4c93cf73a826..be57f8d6aeae 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4134,27 +4134,33 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
> struct gpio_desc *desc;
> int ret;
>
> - desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
> - if (gpiod_not_found(desc) && platform_lookup_allowed) {
> + scoped_guard(rwsem_read, &gpio_devices_sem) {
> + desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
> + &flags, &lookupflags);
> + if (gpiod_not_found(desc) && platform_lookup_allowed) {
> + /*
> + * Either we are not using DT or ACPI, or their lookup
> + * did not return a result. In that case, use platform
> + * lookup as a fallback.
> + */
> + dev_dbg(consumer,
> + "using lookup tables for GPIO lookup\n");
> + desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> + }
> +
> + if (IS_ERR(desc)) {
> + dev_dbg(consumer, "No GPIO consumer %s found\n",
> + con_id);
> + return desc;
> + }
> +
> /*
> - * Either we are not using DT or ACPI, or their lookup did not
> - * return a result. In that case, use platform lookup as a
> - * fallback.
> + * If a connection label was passed use that, else attempt to
> + * use the device name as label
> */
> - dev_dbg(consumer, "using lookup tables for GPIO lookup\n");
> - desc = gpiod_find(consumer, con_id, idx, &lookupflags);
> + ret = gpiod_request(desc, label);
> }
>
> - if (IS_ERR(desc)) {
> - dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
> - return desc;
> - }
> -
> - /*
> - * If a connection label was passed use that, else attempt to use
> - * the device name as label
> - */
> - ret = gpiod_request(desc, label);
> if (ret) {
> if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
> return ERR_PTR(ret);
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Powered by blists - more mailing lists