[PATCH] gpio: cdev: fix waitqueue deadlock on chip deregistration

[PATCH] gpio: cdev: fix waitqueue deadlock on chip deregistration

[Bartosz Golaszewski]

The three calls to wait_event_interruptible_locked() sleep while holding
gdev->srcu. In gpiochip_remove(), synchronize_srcu() runs before
gcdev_unregister(). The latter fires the waitqueue wakeup events from
the notifier callbacks, so a reader blocked on an empty FIFO never leaves
its SRCU section and the removal will block forever in the former.

Since rcu_access_pointer() is a plain READ_ONCE(), we don't really need
to take the SRCU read lock. We don't access the chip structure after
checking it. Remove the lock and instead make the waitqueue aware of the
possible chip deregistration. Keep the initial gdev->chip checks as
removing them may change the error codes visible to user-space, for
instance in cases of a non-blocking read on an empty fifo.

Ideally, we'd factor out the common code into a dedicated helper but
there doesn't seem to exist a clean way of passing a kfifo as argument
so let's open-code the three call sites.

Cc: stable@vger.kernel.org
Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
I've had it on my TODO list for a while. I vaguely remember someone
reporting it to me but can't find the reference anymore. Please feel
free to add a Reported-by: and Closes:.

Paul: I've Cc'ed you as you may want to take a look at my SRCU logic
here.
---
 drivers/gpio/gpiolib-cdev.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f68e33cbd7d4cd5153e82ad77768cf52a90601d6..62215d43b8ac757bbd5344bbcf8631feb2443a6b 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1489,8 +1489,6 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 	ssize_t bytes_read = 0;
 	int ret;
 
-	guard(srcu)(&lr->gdev->srcu);
-
 	if (!rcu_access_pointer(lr->gdev->chip))
 		return -ENODEV;
 
@@ -1507,9 +1505,14 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
 					return -EAGAIN;
 
 				ret = wait_event_interruptible_locked(lr->wait,
-						!kfifo_is_empty(&lr->events));
+						!kfifo_is_empty(&lr->events) ||
+						!rcu_access_pointer(lr->gdev->chip));
 				if (ret)
 					return ret;
+
+				/* Woken up by chip removal. */
+				if (kfifo_is_empty(&lr->events))
+					return -ENODEV;
 			}
 
 			if (kfifo_out(&lr->events, &le, 1) != 1) {
@@ -1801,8 +1804,6 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 	ssize_t ge_size;
 	int ret;
 
-	guard(srcu)(&le->gdev->srcu);
-
 	if (!rcu_access_pointer(le->gdev->chip))
 		return -ENODEV;
 
@@ -1832,9 +1833,14 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 					return -EAGAIN;
 
 				ret = wait_event_interruptible_locked(le->wait,
-						!kfifo_is_empty(&le->events));
+						!kfifo_is_empty(&le->events) ||
+						!rcu_access_pointer(le->gdev->chip));
 				if (ret)
 					return ret;
+
+				/* Woken up by chip removal. */
+				if (kfifo_is_empty(&le->events))
+					return -ENODEV;
 			}
 
 			if (kfifo_out(&le->events, &ge, 1) != 1) {
@@ -2579,8 +2585,6 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 	int ret;
 	size_t event_size;
 
-	guard(srcu)(&cdev->gdev->srcu);
-
 	if (!rcu_access_pointer(cdev->gdev->chip))
 		return -ENODEV;
 
@@ -2600,9 +2604,14 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
 					return -EAGAIN;
 
 				ret = wait_event_interruptible_locked(cdev->wait,
-						!kfifo_is_empty(&cdev->events));
+						!kfifo_is_empty(&cdev->events) ||
+						!rcu_access_pointer(cdev->gdev->chip));
 				if (ret)
 					return ret;
+
+				/* Woken up by chip removal. */
+				if (kfifo_is_empty(&cdev->events))
+					return -ENODEV;
 			}
 #ifdef CONFIG_GPIO_CDEV_V1
 			/* must be after kfifo check so watch_abi_version is set */

---
base-commit: 8b7179210b22f8e48a82c5ceacb9d56a9c4cfec4
change-id: 20261105-gpio-cdev-srcu-deadlock-1377b86567a3

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

Re: [PATCH] gpio: cdev: fix waitqueue deadlock on chip deregistration

[Kent Gibson]

On Thu, Nov 05, 2026 at 03:47:59PM +0100, Bartosz Golaszewski wrote:
> The three calls to wait_event_interruptible_locked() sleep while holding
> gdev->srcu. In gpiochip_remove(), synchronize_srcu() runs before
> gcdev_unregister(). The latter fires the waitqueue wakeup events from
> the notifier callbacks, so a reader blocked on an empty FIFO never leaves
> its SRCU section and the removal will block forever in the former.
> 
> Since rcu_access_pointer() is a plain READ_ONCE(), we don't really need
> to take the SRCU read lock. We don't access the chip structure after
> checking it. Remove the lock and instead make the waitqueue aware of the
> possible chip deregistration. Keep the initial gdev->chip checks as
> removing them may change the error codes visible to user-space, for
> instance in cases of a non-blocking read on an empty fifo.
> 
> Ideally, we'd factor out the common code into a dedicated helper but
> there doesn't seem to exist a clean way of passing a kfifo as argument
> so let's open-code the three call sites.
> 
> Cc: stable@vger.kernel.org
> Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
> I've had it on my TODO list for a while. I vaguely remember someone
> reporting it to me but can't find the reference anymore. Please feel
> free to add a Reported-by: and Closes:.
> 

Isn't this the same as patch 4/7 in Adrian's series[1]?

Cheers,
Kent.

[1] https://lore.kernel.org/linux-gpio/20261103-gpiolib-chip-removal-v5-4-aambrozewicz@nvidia.com/

Re: [PATCH] gpio: cdev: fix waitqueue deadlock on chip deregistration

[Bartosz Golaszewski]

On Thu, Nov 05, 2026 at 10:57:11PM +0800, Kent Gibson wrote:
> On Thu, Nov 05, 2026 at 03:47:59PM +0100, Bartosz Golaszewski wrote:
> > The three calls to wait_event_interruptible_locked() sleep while holding
> > gdev->srcu. In gpiochip_remove(), synchronize_srcu() runs before
> > gcdev_unregister(). The latter fires the waitqueue wakeup events from
> > the notifier callbacks, so a reader blocked on an empty FIFO never leaves
> > its SRCU section and the removal will block forever in the former.
> > 
> > Since rcu_access_pointer() is a plain READ_ONCE(), we don't really need
> > to take the SRCU read lock. We don't access the chip structure after
> > checking it. Remove the lock and instead make the waitqueue aware of the
> > possible chip deregistration. Keep the initial gdev->chip checks as
> > removing them may change the error codes visible to user-space, for
> > instance in cases of a non-blocking read on an empty fifo.
> > 
> > Ideally, we'd factor out the common code into a dedicated helper but
> > there doesn't seem to exist a clean way of passing a kfifo as argument
> > so let's open-code the three call sites.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > ---
> > I've had it on my TODO list for a while. I vaguely remember someone
> > reporting it to me but can't find the reference anymore. Please feel
> > free to add a Reported-by: and Closes:.
> > 
> 
> Isn't this the same as patch 4/7 in Adrian's series[1]?
>

Dang it, yes indeed. Which may explain why I had it written down for
some reason and thought I was supposed to fix it.

Nevermind this patch.

Bart