diff mbox series

[v2,3/3] gpiolib: pin GPIO devices in place during descriptor lookup

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

Commit Message

Bartosz Golaszewski Jan. 2, 2024, 3:59 p.m. UTC
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>
---
 drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

Comments

Marek Szyprowski Jan. 8, 2024, 1:03 p.m. UTC | #1
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
Bartosz Golaszewski Jan. 8, 2024, 3:39 p.m. UTC | #2
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 mbox series

Patch

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);