[PATCH] gpiolib: synchronize updates of descriptor labels

[PATCH] gpiolib: synchronize updates of descriptor labels

[Bartosz Golaszewski]

desc_set_label() swaps desc->label with rcu_replace_pointer() but
without any synchronization of concurrent updates. The swap is a plain
read + write: two threads can observe the same old label and then call
call_scru() on the same SRCU head corrupting the state of the chain.

Add a spinlock to struct gpio_device - we're using a single spinlock for
all descriptors as using a per-descriptor lock would unnecessarily bloat
the size of the desc array with very little chance and no consequences of
contention - and use it to serialize the pointer update.

Cc: stable@vger.kernel.org
Fixes: 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU")
Closes: https://sashiko.dev/#/bug/linux-2f7ac8b6-5034-4ff1-9afa-5700ff44e3e9
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/gpio/gpiolib.c | 11 ++++++++---
 drivers/gpio/gpiolib.h |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fbdeda2f278713a47c2e3d2a6640f387f6d92227..574f1b161d22c41e5f9248fe59efe25d7fcf5032 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -154,9 +154,12 @@ static int desc_set_label(struct gpio_desc *desc, const char *label)
 		memcpy(new->str, label, len);
 	}
 
-	old = rcu_replace_pointer(desc->label, new, 1);
-	if (old)
-		call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
+	scoped_guard(spinlock, &desc->gdev->desc_lock) {
+		old = rcu_replace_pointer(desc->label, new,
+					  lockdep_is_held(&desc->gdev->desc_lock));
+		if (old)
+			call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
+	}
 
 	return 0;
 }
@@ -1201,6 +1204,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		goto err_free_ida;
 	rcu_assign_pointer(gdev->chip, gc);
 
+	spin_lock_init(&gdev->desc_lock);
+
 	ret = init_srcu_struct(&gdev->desc_srcu);
 	if (ret)
 		goto err_cleanup_gdev_srcu;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 650a702741dfb938fbdf11db65a46ac85b6af512..8dd8ea747f8a907664ca334b29b6602661c3767d 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -37,6 +37,7 @@ struct fwnode_handle;
  * @valid_mask: If not %NULL, holds bitmask of GPIOs which are valid to be
  * used from the chip.
  * @desc_srcu: ensures consistent state of GPIO descriptors exposed to users
+ * @desc_lock: synchronizes the updates to descriptor labels
  * @ngpio: the number of GPIO lines on this GPIO device, equal to the size
  * of the @descs array.
  * @can_sleep: indicate whether the GPIO chip driver's callbacks can sleep
@@ -71,6 +72,7 @@ struct gpio_device {
 	struct gpio_desc	*descs;
 	unsigned long		*valid_mask;
 	struct srcu_struct	desc_srcu;
+	spinlock_t		desc_lock;
 	unsigned int		base;
 	u16			ngpio;
 	bool			can_sleep;

---
base-commit: 8b7179210b22f8e48a82c5ceacb9d56a9c4cfec4
change-id: 20261105-gpiolib-set-desc-label-corruption-d024bd14a945

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH] gpiolib: synchronize updates of descriptor labels

[Andy Shevchenko]

On Thu, Nov 05, 2026 at 02:23:45PM +0100, Bartosz Golaszewski wrote:
> desc_set_label() swaps desc->label with rcu_replace_pointer() but
> without any synchronization of concurrent updates. The swap is a plain
> read + write: two threads can observe the same old label and then call

s/call//

> call_scru() on the same SRCU head corrupting the state of the chain.
> 
> Add a spinlock to struct gpio_device - we're using a single spinlock for
> all descriptors as using a per-descriptor lock would unnecessarily bloat
> the size of the desc array with very little chance and no consequences of
> contention - and use it to serialize the pointer update.

...

> -	old = rcu_replace_pointer(desc->label, new, 1);
> -	if (old)
> -		call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
> +	scoped_guard(spinlock, &desc->gdev->desc_lock) {
> +		old = rcu_replace_pointer(desc->label, new,
> +					  lockdep_is_held(&desc->gdev->desc_lock));

Can lockdep_is_held() return anything except LOCK_STATE_HELD here? Or is it
checked outside of this scoped_guard()?

> +		if (old)
> +			call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
> +	}

-- 
With Best Regards,
Andy Shevchenko