[PATCH 0/3] gpio: fix SRCU bugs

[PATCH 0/3] gpio: fix SRCU bugs

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Here are three fixes to some bugs in recent SRCU changes. The first one fixes
an actual race condition. The other two just make lockdep happy.

Bartosz Golaszewski (3):
  gpio: take the SRCU read lock in gpiod_hog()
  gpio: cdev: use correct pointer accessors with SRCU
  gpio: use rcu_dereference_protected() to make lockdep happy

 drivers/gpio/gpiolib-cdev.c | 25 ++++++++++++-------------
 drivers/gpio/gpiolib.c      | 23 +++++++++++++++--------
 drivers/gpio/gpiolib.h      |  4 +++-
 3 files changed, 30 insertions(+), 22 deletions(-)

-- 
2.40.1

[PATCH 1/3] gpio: take the SRCU read lock in gpiod_hog()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

gpiod_hog() may be called without the gpio_device SRCU read lock taken
so we need to do it here as well. It's alright if someone else is
already holding the lock as SRCU read critical sections can be nested.

Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 82811d9a4477..c1391a9a0af6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4492,24 +4492,27 @@ EXPORT_SYMBOL_GPL(gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 	      unsigned long lflags, enum gpiod_flags dflags)
 {
-	struct gpio_chip *gc;
+	struct gpio_device *gdev = desc->gdev;
 	struct gpio_desc *local_desc;
 	int hwnum;
 	int ret;
 
+	CLASS(gpio_chip_guard, guard)(desc);
+	if (!guard.gc)
+		return -ENODEV;
+
 	if (test_and_set_bit(FLAG_IS_HOGGED, &desc->flags))
 		return 0;
 
-	gc = gpiod_to_chip(desc);
 	hwnum = gpio_chip_hwgpio(desc);
 
-	local_desc = gpiochip_request_own_desc(gc, hwnum, name,
+	local_desc = gpiochip_request_own_desc(guard.gc, hwnum, name,
 					       lflags, dflags);
 	if (IS_ERR(local_desc)) {
 		clear_bit(FLAG_IS_HOGGED, &desc->flags);
 		ret = PTR_ERR(local_desc);
 		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed, %d\n",
-		       name, gc->label, hwnum, ret);
+		       name, gdev->label, hwnum, ret);
 		return ret;
 	}
 
-- 
2.40.1

Re: [PATCH 1/3] gpio: take the SRCU read lock in gpiod_hog()

[Linus Walleij]

On Tue, Feb 13, 2024 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> gpiod_hog() may be called without the gpio_device SRCU read lock taken
> so we need to do it here as well. It's alright if someone else is
> already holding the lock as SRCU read critical sections can be nested.
>
> Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Looking at the CLASS() stuff I see this is definitely the way to go
with the code now that we face massive scaling. Nice work.

Yours,
Linus Walleij

[PATCH 2/3] gpio: cdev: use correct pointer accessors with SRCU

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We never dereference the chip pointer in character device code so we can
use the lighter rcu_access_pointer() helper. This also makes lockep
happier as it no longer complains about suspicious rcu_dereference()
usage.

Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 9323b357df43..85037fa4925e 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -206,7 +206,7 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
 
 	guard(srcu)(&lh->gdev->srcu);
 
-	if (!rcu_dereference(lh->gdev->chip))
+	if (!rcu_access_pointer(lh->gdev->chip))
 		return -ENODEV;
 
 	switch (cmd) {
@@ -1521,7 +1521,7 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
 
 	guard(srcu)(&lr->gdev->srcu);
 
-	if (!rcu_dereference(lr->gdev->chip))
+	if (!rcu_access_pointer(lr->gdev->chip))
 		return -ENODEV;
 
 	switch (cmd) {
@@ -1552,7 +1552,7 @@ static __poll_t linereq_poll(struct file *file,
 
 	guard(srcu)(&lr->gdev->srcu);
 
-	if (!rcu_dereference(lr->gdev->chip))
+	if (!rcu_access_pointer(lr->gdev->chip))
 		return EPOLLHUP | EPOLLERR;
 
 	poll_wait(file, &lr->wait, wait);
@@ -1574,7 +1574,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 
 	guard(srcu)(&lr->gdev->srcu);
 
-	if (!rcu_dereference(lr->gdev->chip))
+	if (!rcu_access_pointer(lr->gdev->chip))
 		return -ENODEV;
 
 	if (count < sizeof(le))
@@ -1875,7 +1875,7 @@ static __poll_t lineevent_poll(struct file *file,
 
 	guard(srcu)(&le->gdev->srcu);
 
-	if (!rcu_dereference(le->gdev->chip))
+	if (!rcu_access_pointer(le->gdev->chip))
 		return EPOLLHUP | EPOLLERR;
 
 	poll_wait(file, &le->wait, wait);
@@ -1913,7 +1913,7 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 
 	guard(srcu)(&le->gdev->srcu);
 
-	if (!rcu_dereference(le->gdev->chip))
+	if (!rcu_access_pointer(le->gdev->chip))
 		return -ENODEV;
 
 	/*
@@ -1996,7 +1996,7 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
 
 	guard(srcu)(&le->gdev->srcu);
 
-	if (!rcu_dereference(le->gdev->chip))
+	if (!rcu_access_pointer(le->gdev->chip))
 		return -ENODEV;
 
 	/*
@@ -2510,7 +2510,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	guard(srcu)(&gdev->srcu);
 
 	/* We fail any subsequent ioctl():s when the chip is gone */
-	if (!rcu_dereference(gdev->chip))
+	if (!rcu_access_pointer(gdev->chip))
 		return -ENODEV;
 
 	/* Fill in the struct and pass to userspace */
@@ -2595,7 +2595,7 @@ static __poll_t lineinfo_watch_poll(struct file *file,
 
 	guard(srcu)(&cdev->gdev->srcu);
 
-	if (!rcu_dereference(cdev->gdev->chip))
+	if (!rcu_access_pointer(cdev->gdev->chip))
 		return EPOLLHUP | EPOLLERR;
 
 	poll_wait(file, &cdev->wait, pollt);
@@ -2618,7 +2618,7 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 
 	guard(srcu)(&cdev->gdev->srcu);
 
-	if (!rcu_dereference(cdev->gdev->chip))
+	if (!rcu_access_pointer(cdev->gdev->chip))
 		return -ENODEV;
 
 #ifndef CONFIG_GPIO_CDEV_V1
@@ -2696,7 +2696,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
 	guard(srcu)(&gdev->srcu);
 
 	/* Fail on open if the backing gpiochip is gone */
-	if (!rcu_dereference(gdev->chip))
+	if (!rcu_access_pointer(gdev->chip))
 		return -ENODEV;
 
 	cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
@@ -2796,8 +2796,7 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
 
 	guard(srcu)(&gdev->srcu);
 
-	gc = rcu_dereference(gdev->chip);
-	if (!gc)
+	if (!rcu_access_pointer(gdev->chip))
 		return -ENODEV;
 
 	chip_dbg(gc, "added GPIO chardev (%d:%d)\n", MAJOR(devt), gdev->id);
-- 
2.40.1

Re: [PATCH 2/3] gpio: cdev: use correct pointer accessors with SRCU

[Linus Walleij]

On Tue, Feb 13, 2024 at 10:31 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We never dereference the chip pointer in character device code so we can
> use the lighter rcu_access_pointer() helper. This also makes lockep
> happier as it no longer complains about suspicious rcu_dereference()
> usage.
>
> Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I had to check what rcu_access_pointer() does and it's clearly the
right thing to do so:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 3/3] gpio: use rcu_dereference_protected() to make lockdep happy

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Lockdep with CONFIG_PROVE_RCU enabled reports false positives about
suspicious rcu_dereference() usage. Let's silence it by using
rcu_dereference_protected().

Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpiolib.c | 12 ++++++++----
 drivers/gpio/gpiolib.h |  4 +++-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index c1391a9a0af6..d7503376b918 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -109,7 +109,8 @@ const char *gpiod_get_label(struct gpio_desc *desc)
 		return "interrupt";
 
 	return test_bit(FLAG_REQUESTED, &flags) ?
-			rcu_dereference(desc->label) : NULL;
+			rcu_dereference_protected(desc->label,
+					lockdep_is_held(&desc->srcu)) : NULL;
 }
 
 static int desc_set_label(struct gpio_desc *desc, const char *label)
@@ -2978,7 +2979,8 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
 
 	guard(srcu)(&gdev->srcu);
 
-	gc = rcu_dereference(gdev->chip);
+	gc = rcu_dereference_protected(gdev->chip,
+				       lockdep_is_held(&gdev->srcu));
 	if (!gc)
 		return -ENODEV;
 
@@ -3012,7 +3014,8 @@ static bool gpio_device_chip_cmp(struct gpio_device *gdev, struct gpio_chip *gc)
 {
 	guard(srcu)(&gdev->srcu);
 
-	return gc == rcu_dereference(gdev->chip);
+	return gc == rcu_dereference_protected(gdev->chip,
+					       lockdep_is_held(&gdev->srcu));
 }
 
 int gpiod_get_array_value_complex(bool raw, bool can_sleep,
@@ -3593,7 +3596,8 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 	gdev = desc->gdev;
 	/* FIXME Cannot use gpio_chip_guard due to const desc. */
 	guard(srcu)(&gdev->srcu);
-	gc = rcu_dereference(gdev->chip);
+	gc = rcu_dereference_protected(gdev->chip,
+				       lockdep_is_held(&gdev->srcu));
 	if (!gc)
 		return -ENODEV;
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 07443d26cbca..a857848b8955 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/gpio/consumer.h> /* for enum gpiod_flags */
 #include <linux/gpio/driver.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
 #include <linux/srcu.h>
@@ -202,7 +203,8 @@ DEFINE_CLASS(gpio_chip_guard,
 
 		_guard.gdev = desc->gdev;
 		_guard.idx = srcu_read_lock(&_guard.gdev->srcu);
-		_guard.gc = rcu_dereference(_guard.gdev->chip);
+		_guard.gc = rcu_dereference_protected(_guard.gdev->chip,
+					lockdep_is_held(&_guard.gdev->srcu));
 
 		_guard;
 	     }),
-- 
2.40.1

Re: [PATCH 3/3] gpio: use rcu_dereference_protected() to make lockdep happy

[Paul E. McKenney]

On Tue, Feb 13, 2024 at 10:31:08AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Lockdep with CONFIG_PROVE_RCU enabled reports false positives about
> suspicious rcu_dereference() usage. Let's silence it by using
> rcu_dereference_protected().
> 
> Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/gpio/gpiolib.c | 12 ++++++++----
>  drivers/gpio/gpiolib.h |  4 +++-
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index c1391a9a0af6..d7503376b918 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -109,7 +109,8 @@ const char *gpiod_get_label(struct gpio_desc *desc)
>  		return "interrupt";
>  
>  	return test_bit(FLAG_REQUESTED, &flags) ?
> -			rcu_dereference(desc->label) : NULL;
> +			rcu_dereference_protected(desc->label,
> +					lockdep_is_held(&desc->srcu)) : NULL;

Why not this instead?

> +			srcu_dereference(desc->label, &desc->srcu) : NULL;

And similarly for the rest of the changes.

							Thanx, Paul

>  }
>  
>  static int desc_set_label(struct gpio_desc *desc, const char *label)
> @@ -2978,7 +2979,8 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
>  
>  	guard(srcu)(&gdev->srcu);
>  
> -	gc = rcu_dereference(gdev->chip);
> +	gc = rcu_dereference_protected(gdev->chip,
> +				       lockdep_is_held(&gdev->srcu));
>  	if (!gc)
>  		return -ENODEV;
>  
> @@ -3012,7 +3014,8 @@ static bool gpio_device_chip_cmp(struct gpio_device *gdev, struct gpio_chip *gc)
>  {
>  	guard(srcu)(&gdev->srcu);
>  
> -	return gc == rcu_dereference(gdev->chip);
> +	return gc == rcu_dereference_protected(gdev->chip,
> +					       lockdep_is_held(&gdev->srcu));
>  }
>  
>  int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> @@ -3593,7 +3596,8 @@ int gpiod_to_irq(const struct gpio_desc *desc)
>  	gdev = desc->gdev;
>  	/* FIXME Cannot use gpio_chip_guard due to const desc. */
>  	guard(srcu)(&gdev->srcu);
> -	gc = rcu_dereference(gdev->chip);
> +	gc = rcu_dereference_protected(gdev->chip,
> +				       lockdep_is_held(&gdev->srcu));
>  	if (!gc)
>  		return -ENODEV;
>  
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 07443d26cbca..a857848b8955 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -14,6 +14,7 @@
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h> /* for enum gpiod_flags */
>  #include <linux/gpio/driver.h>
> +#include <linux/lockdep.h>
>  #include <linux/module.h>
>  #include <linux/notifier.h>
>  #include <linux/srcu.h>
> @@ -202,7 +203,8 @@ DEFINE_CLASS(gpio_chip_guard,
>  
>  		_guard.gdev = desc->gdev;
>  		_guard.idx = srcu_read_lock(&_guard.gdev->srcu);
> -		_guard.gc = rcu_dereference(_guard.gdev->chip);
> +		_guard.gc = rcu_dereference_protected(_guard.gdev->chip,
> +					lockdep_is_held(&_guard.gdev->srcu));
>  
>  		_guard;
>  	     }),
> -- 
> 2.40.1
> 

Re: [PATCH 3/3] gpio: use rcu_dereference_protected() to make lockdep happy

[Bartosz Golaszewski]

On Tue, Feb 13, 2024 at 1:03 PM Paul E. McKenney <paulmck@kernel.org> wrote:
>
> On Tue, Feb 13, 2024 at 10:31:08AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Lockdep with CONFIG_PROVE_RCU enabled reports false positives about
> > suspicious rcu_dereference() usage. Let's silence it by using
> > rcu_dereference_protected().
> >
> > Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/gpio/gpiolib.c | 12 ++++++++----
> >  drivers/gpio/gpiolib.h |  4 +++-
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index c1391a9a0af6..d7503376b918 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -109,7 +109,8 @@ const char *gpiod_get_label(struct gpio_desc *desc)
> >               return "interrupt";
> >
> >       return test_bit(FLAG_REQUESTED, &flags) ?
> > -                     rcu_dereference(desc->label) : NULL;
> > +                     rcu_dereference_protected(desc->label,
> > +                                     lockdep_is_held(&desc->srcu)) : NULL;
>
> Why not this instead?
>
> > +                     srcu_dereference(desc->label, &desc->srcu) : NULL;
>
> And similarly for the rest of the changes.
>

Ah, I missed this one. Thanks, I'll use it.

For patch 1/3 in this series - should I stick with
rcu_access_pointer() or is it better to use srcu_dereference here as
well?

Bart

>                                                         Thanx, Paul
>
> >  }
> >
> >  static int desc_set_label(struct gpio_desc *desc, const char *label)
> > @@ -2978,7 +2979,8 @@ static int gpiod_get_raw_value_commit(const struct gpio_desc *desc)
> >
> >       guard(srcu)(&gdev->srcu);
> >
> > -     gc = rcu_dereference(gdev->chip);
> > +     gc = rcu_dereference_protected(gdev->chip,
> > +                                    lockdep_is_held(&gdev->srcu));
> >       if (!gc)
> >               return -ENODEV;
> >
> > @@ -3012,7 +3014,8 @@ static bool gpio_device_chip_cmp(struct gpio_device *gdev, struct gpio_chip *gc)
> >  {
> >       guard(srcu)(&gdev->srcu);
> >
> > -     return gc == rcu_dereference(gdev->chip);
> > +     return gc == rcu_dereference_protected(gdev->chip,
> > +                                            lockdep_is_held(&gdev->srcu));
> >  }
> >
> >  int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> > @@ -3593,7 +3596,8 @@ int gpiod_to_irq(const struct gpio_desc *desc)
> >       gdev = desc->gdev;
> >       /* FIXME Cannot use gpio_chip_guard due to const desc. */
> >       guard(srcu)(&gdev->srcu);
> > -     gc = rcu_dereference(gdev->chip);
> > +     gc = rcu_dereference_protected(gdev->chip,
> > +                                    lockdep_is_held(&gdev->srcu));
> >       if (!gc)
> >               return -ENODEV;
> >
> > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> > index 07443d26cbca..a857848b8955 100644
> > --- a/drivers/gpio/gpiolib.h
> > +++ b/drivers/gpio/gpiolib.h
> > @@ -14,6 +14,7 @@
> >  #include <linux/err.h>
> >  #include <linux/gpio/consumer.h> /* for enum gpiod_flags */
> >  #include <linux/gpio/driver.h>
> > +#include <linux/lockdep.h>
> >  #include <linux/module.h>
> >  #include <linux/notifier.h>
> >  #include <linux/srcu.h>
> > @@ -202,7 +203,8 @@ DEFINE_CLASS(gpio_chip_guard,
> >
> >               _guard.gdev = desc->gdev;
> >               _guard.idx = srcu_read_lock(&_guard.gdev->srcu);
> > -             _guard.gc = rcu_dereference(_guard.gdev->chip);
> > +             _guard.gc = rcu_dereference_protected(_guard.gdev->chip,
> > +                                     lockdep_is_held(&_guard.gdev->srcu));
> >
> >               _guard;
> >            }),
> > --
> > 2.40.1
> >