| Message ID | 20240102155949.73434-4-brgl@bgdev.pl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpiolib: use a read-write semaphore to protect the GPIO device list | expand |
On 02.01.2024 16:59, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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@linaro.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
On Mon, Jan 8, 2024 at 2:03 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 02.01.2024 16:59, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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@linaro.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 > Thanks for the report. I think it may have come too late in the release cycle as it has the potential to break a lot of things. I will back it out of my for-next branch. I'll resend it for v6.9. Bart
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);