gpiolib: use a read-write semaphore to protect the GPIO device list

From: Bartosz Golaszewski <[email protected]>

If we wait until the GPIO device's .release() callback gets invoked before we remove it from the global device list, then we risk that someone will look it up using gpio_device_find() between where we dropped the last reference and before .release() is done taking a reference again to an object that's being released.

The device must be removed when it's being unregistered - just like how we remove it from the GPIO bus.

Fixes ff2b13592299 ("gpio: make the gpiochip a real device")

Signed-off-by Bartosz Golaszewski <[email protected]>

From: Bartosz Golaszewski <[email protected]>

There are only two spots where we modify (add to or remove objects from) the GPIO device list. Readers should be able to access it concurrently. Replace the mutex with a read-write semaphore and adjust the locking operations accordingly.

Signed-off-by Bartosz Golaszewski <[email protected]>

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

---
  drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
  1 file changed, 23 insertions(+), 17 deletions(-)

drivers/gpio/gpiolib.c +23 -10

index 4c93cf73a826..be57f8d6aeae 100644
@@ -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;

Quoted 9 lines · Bartosz Golaszewski

From: Bartosz Golaszewski <[email protected]>

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 <[email protected]>

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.

Quoted 56 lines

---
  drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
  1 file changed, 23 insertions(+), 17 deletions(-)

drivers/gpio/gpiolib.c +23 -17

index 4c93cf73a826..be57f8d6aeae 100644
@@ -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);

drivers/gpio/gpiolib.c static struct gpio_desc *gpiod_find_and_request(struct device *consumer, · line 4166

Quoted 3 lines

drivers/gpio/gpiolib.c +0 -0

      if (ret) {
              if (!(ret == -EBUSY && flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
                      return ERR_PTR(ret);

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

From: Bartosz Golaszewski <[email protected]>

I'm still figuring out how to keep GPIO descriptors coherent while (mostly) lockless. In the meantime, I found a potential race-condition during GPIO descriptor lookup and also figured that the correct way to protect the GPIO device list is actually a read-write semaphore as we're not modifying the list very often and readers should be able to iterate over it concurrently.

The first patch in this series is new in v2. I realized that we must not wait until .release() to remove the GPIO device from the list as this is why pinning down the GPIO device list during lookup would never work - we always could end up re-taking a reference to an object that was being released if it got looked up between when the last reference is dropped and the object is finally removed from the device list.

v1 -> v2:
- add patch 1/3 to fix a release timing issue

Bartosz Golaszewski (3):
gpiolib: remove the GPIO device from the list when it's unregistered
gpiolib: replace the GPIO device mutex with a read-write semaphore
gpiolib: pin GPIO devices in place during descriptor lookup

 drivers/gpio/gpiolib-sysfs.c |  2 +-
 drivers/gpio/gpiolib.c       | 62 ++++++++++++++++++++----------------
 drivers/gpio/gpiolib.h       |  2 +-
 3 files changed, 36 insertions(+), 30 deletions(-)

--
2.40.1