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

PATCH Superseded Bartosz Golaszewski 1 participant 4 messages Dec 21, 2023 GPIO

lore

Share & tools

lore

Bartosz Golaszewski

author

2023-12-21 18:43 UTC #0 lore raw

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

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.

Bartosz Golaszewski (2):
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       | 58 ++++++++++++++++++++----------------
 drivers/gpio/gpiolib.h       |  2 +-
 3 files changed, 34 insertions(+), 28 deletions(-)

Signature · 1 line

2.40.1

Bartosz Golaszewski

author

2023-12-21 18:43 UTC #1 lore raw

PATCH 1/2 gpiolib: replace the GPIO device mutex with a read-write semaphore

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]>

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

drivers/gpio/gpiolib-sysfs.c +1 -1

index ae4fc013b675..e4159344a0ea 100644
@@ -771,7 +771,7 @@ int gpiochip_sysfs_register_all(void)
 	struct gpio_device *gdev;
 	int ret;
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (gdev->mockdev)

drivers/gpio/gpiolib.c +9 -9

index c9ca809b55de..1baeb6778ec6 100644
@@ -98,7 +98,7 @@ static DEFINE_MUTEX(gpio_lookup_lock);
 static LIST_HEAD(gpio_lookup_list);
 
 LIST_HEAD(gpio_devices);
-DEFINE_MUTEX(gpio_devices_lock);
+DECLARE_RWSEM(gpio_devices_sem);
 
 static DEFINE_MUTEX(gpio_machine_hogs_mutex);
 static LIST_HEAD(gpio_machine_hogs);

@@ -131,7 +131,7 @@ struct gpio_desc *gpio_to_desc(unsigned gpio)
 {
 	struct gpio_device *gdev;
 
-	scoped_guard(mutex, &gpio_devices_lock) {
+	scoped_guard(rwsem_read, &gpio_devices_sem) {
 		list_for_each_entry(gdev, &gpio_devices, list) {
 			if (gdev->base <= gpio &&
 			    gdev->base + gdev->ngpio > gpio)

@@ -415,7 +415,7 @@ static struct gpio_desc *gpio_name_to_desc(const char * const name)
 	if (!name)
 		return NULL;
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		struct gpio_desc *desc;

@@ -664,7 +664,7 @@ static void gpiodev_release(struct device *dev)
 {
 	struct gpio_device *gdev = to_gpio_device(dev);
 
-	scoped_guard(mutex, &gpio_devices_lock)
+	scoped_guard(rwsem_write, &gpio_devices_sem)
 		list_del(&gdev->list);
 
 	ida_free(&gpio_ida, gdev->id);

@@ -887,7 +887,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	gdev->ngpio = gc->ngpio;
 
-	scoped_guard(mutex, &gpio_devices_lock) {
+	scoped_guard(rwsem_write, &gpio_devices_sem) {
 		/*
 		 * TODO: this allocates a Linux GPIO number base in the global
 		 * GPIO numberspace for this chip. In the long run we want to

@@ -1017,7 +1017,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		goto err_print_message;
 	}
 err_remove_from_list:
-	scoped_guard(mutex, &gpio_devices_lock)
+	scoped_guard(rwsem_write, &gpio_devices_sem)
 		list_del(&gdev->list);
 err_free_label:
 	kfree_const(gdev->label);

@@ -1127,7 +1127,7 @@ struct gpio_device *gpio_device_find(void *data,
 	 */
 	might_sleep();
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (gdev->chip && match(gdev->chip, data))

@@ -4745,7 +4745,7 @@ static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos)
 
 	s->private = "";
 
-	guard(mutex)(&gpio_devices_lock);
+	guard(rwsem_read)(&gpio_devices_sem);
 
 	list_for_each_entry(gdev, &gpio_devices, list) {
 		if (index-- == 0)

@@ -4760,7 +4760,7 @@ static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos)
 	struct gpio_device *gdev = v;
 	void *ret = NULL;
 
-	scoped_guard(mutex, &gpio_devices_lock) {
+	scoped_guard(rwsem_read, &gpio_devices_sem) {
 		if (list_is_last(&gdev->list, &gpio_devices))
 			ret = NULL;
 		else

drivers/gpio/gpiolib.h +1 -1

index 0ce7451a6b24..97df54abf57a 100644
@@ -137,7 +137,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
 
 extern spinlock_t gpio_lock;
 extern struct list_head gpio_devices;
-extern struct mutex gpio_devices_lock;
+extern struct rw_semaphore gpio_devices_sem;
 
 void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);

Signature · 1 line

2.40.1

Bartosz Golaszewski

author

2023-12-21 18:43 UTC #2 lore raw

PATCH 2/2 gpiolib: pin GPIO devices in place during descriptor lookup

In reply to 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]>

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

drivers/gpio/gpiolib.c +23 ~~-17~~

index 1baeb6778ec6..8a15b8f6b50e 100644
@@ -4147,27 +4147,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);

Signature · 1 line

2.40.1

Bartosz Golaszewski

author

2023-12-21 19:03 UTC #3 lore raw

Re: [PATCH 2/2] gpiolib: pin GPIO devices in place during descriptor lookup

Quoted 19 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]>
---
 drivers/gpio/gpiolib.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)
drivers/gpio/gpiolib.c +0 -0
index 1baeb6778ec6..8a15b8f6b50e 100644

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

Quoted 6 lines

drivers/gpio/gpiolib.c +1 -2

@@ -4147,27 +4147,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) {

I am too sleep deprived to be coding. This doesn't make sense because if we are held at the write semaphore in gpiodev_release() then it's already too late as the device is being removed already and we'll still end up with a dangling pointer.

Should we hold off any reference putting for all GPIO devices when a descriptor lookup is in progress?

Bart

Quoted 47 lines

drivers/gpio/gpiolib.c +22 ~~-16~~

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

2.40.1