[PATCH v2 0/6] gpiolib: fix chip removal with lines still requested from user space

[PATCH v2 0/6] gpiolib: fix chip removal with lines still requested from user space

[Adrian Ambrozewicz]

Removing a GPIO chip while user space keeps a line request open, which
is what happens when a USB or otherwise hot-pluggable GPIO controller
goes away, leaves a few things behind on current kernels:

 - the consumer label of each such line leaks (patch 2), and freeing it
   just before the GPIO device is released needs gpiodev_release() to
   wait for pending label frees (patch 1, which can also be hit on its
   own with line hogs);
 - the character device frees the line's IRQ a second time on close,
   after gpiochip_remove() already did, which warns when the IRQ number
   was reused and was reported as a use-after-free with virtio-gpio
   (patch 3), and its IRQs are freed in an order that still lets a
   reused IRQ number or a concurrent close() get in the way (patch 6);
 - a process blocked in read() on the request keeps gpiochip_remove()
   waiting in synchronize_srcu() until it is signalled, and with it
   device_shutdown() on power-off (patch 4);
 - the IRQ name the character device allocated leaks (patch 5).

All five were reproduced with gpio-sim on v7.3-rc5 under QEMU (x86_64,
4 vCPUs) and are still present in gpio/for-next. The loop: create a
gpio-sim chip, request a line through the character device and hold
it, set "live" to 0 while it is held, then optionally set the chip up
again and request the same line, and close the first request.

                      v7.3-rc5 (30 cycles)        with the series (100)
 v2 edges, reuse      30 WARN, 17-30 labels       0 WARN, 0 leaks
 v2 edges             0 WARN, 19-29 labels        0
 v2 sw debounce       30 WARN, 13-30 labels       0
 v1 line event        30 WARN, 14-29 labels       0
 v2 no edges          15-30 labels                0
 v2 edges, consumer   30 WARN, 15-30 labels,      0
                      17-24 IRQ names
 blocked read()       removal waits for the       removal completes,
 (v2, v1, info watch) reader to be killed         read() gets -ENODEV

The counts above are from both a KASAN and a non-KASAN build of v1;
KASAN did not report anything in these runs, the slab use-after-free
from the report needs the descriptor free to race with the close.
kmemleak does not see every leaked object, hence the ranges. The
desc_srcu warning fixed by patch 1 takes srcutree.convert_to_big=1 to
show up within a few dozen cycles of a hogged line (cycle 57 here);
with the series, 1000 cycles pass. That warning was also seen on real
hardware, on arm64 KCSAN kernels with v7.3-rc5 unbinding a USB GPIO
expander whose driver is not yet upstream, built both as an out-of-tree
module and into the kernel.

Patch 3 on its own leaves two windows, which gpio-sim does not hit: if
the chip disposes of its IRQ mappings in gpiochip_irqchip_remove() and
another device gets one of the numbers before the character device's
notifier runs, the notifier's free_irq() warns; and a close() racing
with gpiochip_remove() is not serialized against gpiod_free_irqs().
Patch 6 calls the notifier right after the chip pointer is cleared and
before gpiod_free_irqs(), and makes the release paths free their IRQs
before unregistering from it. With a test-only chip on the gpiolib
hierarchical irqchip that hands an IRQ number out again during the
removal, or sleeps in irq_bus_lock() to widen the race, 30 out of 30
cycles warn in each case with patches 1-5 and none with patch 6.

Patch 6 is kept apart from patch 3 because it changes the order of
gpiochip_remove() for in-kernel users too: interrupts they leave
requested are now freed after the chip pointer is cleared rather than
before, so the irqchip callbacks that free_irq() calls into run with
the gpiod_*() consumer calls on that chip already failing; the driver
data is still set at that point. Patch 3 is enough for the reported
use-after-free and is the smaller backport. Herve's series [1] moved the
character device's cleanup earlier in gpiochip_remove() in 2024 and was
set aside for a fix covering in-kernel users as well, which became
gpiochip_free_remaining_irqs(); patch 6 keeps that and only lets the
character device go first.

The revocable conversion posted for gpiolib [3] keeps all of these as
they are; patches 2, 4 and 6 would need rebasing over it, patch 6
because its notifier call goes right after the revocable_revoke() that
the conversion puts in gpiochip_remove().

On arm64 (QEMU virt, 4 vCPUs, KASAN), 5 cycles per case, the removal
loop gives the same picture: on v7.3-rc5 the v2 edge, debounce, v1 and
consumer cases warn in every cycle, all cases but the blocked readers
leak labels, and the blocked readers hold up the removal; with the
series none of that happens. With the test chip, the close race warns
in 10 out of 10 cycles on v7.3-rc5 for v2 and v1 alike, and in one v2
cycle KASAN reports the use-after-free of [2], free_irq() from
edge_detector_stop() on a freed irq_desc; with the series there is
neither. (The reused IRQ number only leads to a warning once patch 3
frees IRQs from the notifier, so not on v7.3-rc5.) An x86_64 KCSAN
build, default KCSAN options, shows the same warnings and stalls on
v7.3-rc5 and none with the series; KCSAN itself reported no data race
in gpiolib, also with kcsan.skip_watch=100 for the test chip and the
edge, consumer and blocked-reader cases (one report in 32 boots, in
osq_unlock() while a disk was probed during boot). The test kernels
with patch 6 also had the direction fix mentioned below applied, which
touches none of these paths.

For v2, the x86_64 removal loop (10 cycles per case, 2 for the blocked
readers) and the test chip (30 cycles) were run again on a debug and kmemleak build of this series
with the two fixes below, and on KASAN and KCSAN builds that also had
two fixes for existing code in gpiolib.c on top: no warnings, leaks or
stalls. The arm64 runs were not repeated.

User-visible change: with patch 4, a read() that is blocked on a line
request, on a v1 line event or on a chip file with watched lines when
the chip is removed returns the events still queued, if any, and then
fails with -ENODEV. On v7.3-rc5 it stays blocked until it is signalled
or an event arrives, holding up the removal. A read() started after the
removal already failed with -ENODEV, and poll() already reported
EPOLLHUP | EPOLLERR; neither changes.

For stable: patches 3, 5 and 6 fix commit ec8b6f55b981, which is in
6.12 and later. Patch 4 applies to 6.12.y as is; the SRCU read section
it changes replaced the gdev->sem rwsem in 6.9, and 5.15.y, 6.1.y and
6.6.y have that rwsem from the commit it fixes and need a different
change. There the read handlers hold gdev->sem for reading across the
whole read through call_read_locked(), wait included, and
gpiochip_remove() takes it for writing, so the backport has to drop the
rwsem around wait_event_interruptible_locked(), retake it and check
gdev->chip again, with !gdev->chip added to the wait condition. On
6.6.y the device notifier, called with the rwsem held for writing, then
wakes the reader, which gets -ENODEV once the removal drops the rwsem.
5.15.y and 6.1.y have no device notifier: the removal no longer waits,
but a blocked reader is only woken by an event or a signal, as before.

Two fixes found along the way have nothing to do with chip removal and
are sent on their own: one for the line info keeping a direction that
the driver refused (gpiolib-cdev, now at v2), one for the event clock
flags of a released line (gpiolib, v1 unchanged). This series does not
depend on them, nor they on it; all three apply to v7.3-rc5,
gpio/for-next and gpio/for-current in any order. A fix for a debounce
period race in gpiod_free_commit() that Sashiko found on the event
clock fix builds on patch 2, so it will follow once this series is in.

Direction fix: https://lore.kernel.org/r/20261016-gpiolib-cdev-direction-v2-1-aambrozewicz@nvidia.com
Event clock fix: https://lore.kernel.org/r/20261012-gpiolib-event-clock-v1-1-aambrozewicz@nvidia.com

For transparency: an LLM coding assistant helped with the analysis, the
reproducers and test setup, and drafting the patches and messages,
hence the Assisted-by tags.

Cc'd beyond the GPIO maintainers and list: Paul for the SRCU side,
since patch 1 replaces synchronize_srcu() with srcu_barrier() before
cleanup_srcu_struct(), patch 4 moves sleeping waits out of the
gdev->srcu read section, and patch 6 relies on synchronize_srcu() on
gdev->srcu to rule out new IRQ requests through the character device;
Herve for [1], which patch 6 revisits; Jia Jia for the virtio-gpio
use-after-free report [2] that patch 3 addresses; Kent as the author of
the v2 character device; Andy, who acked the commit that patch 2 fixes.

Changes in v2:
- Patches 1, 2, 3 and 5: backtraces cut down to the warning and the
  frames that matter (Andy).
- Patch 1: Paul's Reviewed-by.
- Patch 2: gpiod_free_commit() returns early for a line that is not
  requested on a live chip and clears the label once, before the chip
  check, so now before ->free() (Andy).
- Patch 3: the IRQ check moved into free_line_irq(), with a local for
  the IRQ name; container_of() on one line; loop variable declared in
  the for (Andy).
- Patch 4: the three chip checks of the read() paths factored into
  gpiodev_check_chip() (Andy).
- Patch 5: follows the free_line_irq() shape from patch 3 (Andy).
- Patch 6: mutex_destroy() for config_mutex and irq_lock; the missing
  ->to_irq() case folded into the irq <= 0 check (Andy).
- Assisted-by tags; see the note on tools above.
- Link to v1: https://lore.kernel.org/r/20261012-gpiolib-chip-removal-v1-0-aambrozewicz@nvidia.com

[1] https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
[2] https://lore.kernel.org/all/20260817070429.157993-1-physicalmtea@gmail.com/
[3] https://lore.kernel.org/all/20261011121246.162759-1-tzungbi@kernel.org/

Adrian Ambrozewicz (6):
  gpiolib: wait for pending label frees before cleaning up desc_srcu
  gpiolib: free the label of lines released after their chip was removed
  gpiolib: cdev: do not free line IRQs again after the chip is removed
  gpiolib: cdev: don't sleep in the SRCU read section in read()
  gpiolib: cdev: don't leak the IRQ name when the chip is removed
  gpiolib: let the character device free its IRQs first on chip removal

 drivers/gpio/gpiolib-cdev.c | 146 +++++++++++++++++++++++++++---------
 drivers/gpio/gpiolib.c      |  39 ++++++++--
 2 files changed, 140 insertions(+), 45 deletions(-)


base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.43.0

[PATCH v2 1/6] gpiolib: wait for pending label frees before cleaning up desc_srcu

[Adrian Ambrozewicz]

desc_set_label() frees the old label with call_srcu() on gdev->desc_srcu.
gpiodev_release() calls synchronize_srcu() and then cleanup_srcu_struct()
on it, meaning to run the pending frees first, but synchronize_srcu()
only waits for a grace period, not for callbacks queued before it to be
invoked. Once the srcu_struct uses per-CPU callback lists, a label freed
on one CPU, for instance by gpiochip_free_hogs() at the start of
gpiochip_remove(), can still be waiting for its invocation timer there
when synchronize_srcu() returns on another, and cleanup_srcu_struct()
warns:

  WARNING: kernel/rcu/srcutree.c:706 at cleanup_srcu_struct
  Call Trace:
   gpiodev_release
   device_release
   kobject_put
   devres_release_all
   device_release_driver_internal
   platform_device_unregister
   gpio_sim_device_deactivate

cleanup_srcu_struct() then flushes the callbacks itself, so the label is
still freed, but the warning is what the comment above that check says
srcu_barrier() is for. Use it.

A gpio-sim chip with one hogged line, set live and back in a loop, warns
within 4 to 60 cycles in three runs on a 4-CPU guest booted with
srcutree.convert_to_big=1; with this change, three runs of 1000 cycles
pass. With the default srcutree parameters the srcu_struct only goes
per-CPU under contention and the loop did not hit it in 3000 cycles; the
warning was first seen that way, once, on a KCSAN arm64 guest unbinding
a USB-attached GPIO controller whose firmware node hogs a line, and
since then on real hardware with v7.3-rc5 arm64 KCSAN kernels, when
unbinding a USB GPIO expander whose driver is not yet upstream, built
both as an out-of-tree module and into the kernel.

Fixes: a86d27693066 ("gpiolib: fix the speed of descriptor label setting with SRCU")
Cc: stable@vger.kernel.org
Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
Assisted-by: LLM
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
Changes in v2:
- Backtrace cut down to the warning and the frames that matter (Andy).
- Paul's Reviewed-by.
- Assisted-by tag.

 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 772c6f04f489..1984f3416437 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -895,7 +895,7 @@ static void gpiodev_release(struct device *dev)
 	struct gpio_device *gdev = to_gpio_device(dev);
 
 	/* Call pending kfree()s for descriptor labels. */
-	synchronize_srcu(&gdev->desc_srcu);
+	srcu_barrier(&gdev->desc_srcu);
 	cleanup_srcu_struct(&gdev->desc_srcu);
 
 	ida_free(&gpio_ida, gdev->id);
-- 
2.43.0

[PATCH v2 2/6] gpiolib: free the label of lines released after their chip was removed

[Adrian Ambrozewicz]

gpiod_free_commit() only drops the consumer label while the chip is
still there:

	if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
		...
		desc_set_label(desc, NULL);

A line that is still requested when gpiochip_remove() runs, for instance
by a character device line request that user space keeps open, is
released only afterwards, when gdev->chip is already NULL. Its label is
then never freed, as gpiodev_release() frees the descriptor array but
not the labels, and kmemleak reports one object per such line:

  unreferenced object (size 32):
    comm "gpio-hold"
    backtrace:
      desc_set_label
      gpiod_request_commit
      gpiod_request
      linereq_create
      gpio_ioctl

Commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU")
cleared the label unconditionally; it moved under the chip check with
the removal of gpio_lock. Return early only for a line that is not
requested on a live chip, and clear the label before the chip check, so
that it is also freed when the chip is gone. The ->free() callback, the
flags update and the line state notification still need the chip and
stay under the check; with the chip gone, no character device file can
receive the notification any more.

The label is now often freed just before the last reference to the GPIO
device goes away, which depends on gpiodev_release() waiting for pending
label frees (previous patch).

Reproduced with gpio-sim: request a line through the character device,
set the chip's configfs "live" attribute to 0 while the request is open,
then close it. Without this change kmemleak reports between 15 and 30
such objects after 30 cycles; with it, none after 100.

Fixes: 35b545332b80 ("gpio: remove gpio_lock")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
Changes in v2:
- Return early for a line that is not requested on a live chip and clear
  the label once, before the chip check, so now before ->free() (Andy).
- kmemleak report cut down (Andy).
- Assisted-by tag.

 drivers/gpio/gpiolib.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1984f3416437..31ccdd373765 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2666,7 +2666,12 @@ void gpiod_free_commit(struct gpio_desc *desc)
 
 	flags = READ_ONCE(desc->flags);
 
-	if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
+	if (guard.gc && !test_bit(GPIOD_FLAG_REQUESTED, &flags))
+		return;
+
+	desc_set_label(desc, NULL);
+
+	if (guard.gc) {
 		if (guard.gc->free)
 			guard.gc->free(guard.gc, gpiod_hwgpio(desc));
 
@@ -2683,7 +2688,6 @@ void gpiod_free_commit(struct gpio_desc *desc)
 #ifdef CONFIG_OF_DYNAMIC
 		WRITE_ONCE(desc->hog, NULL);
 #endif
-		desc_set_label(desc, NULL);
 		WRITE_ONCE(desc->flags, flags);
 #ifdef CONFIG_GPIO_CDEV
 		WRITE_ONCE(desc->debounce_period_us, 0);
-- 
2.43.0

[PATCH v2 3/6] gpiolib: cdev: do not free line IRQs again after the chip is removed

[Adrian Ambrozewicz]

Since commit ec8b6f55b981 ("gpio: free irqs that are still requested
when the chip is being removed"), gpiochip_remove() calls free_irq() for
every action still installed on the lines of the chip. The character
device does not know about this: a line request or v1 line event that
user space keeps open across the removal still holds the IRQ number and
calls free_irq() on it again when it is released.

What that second free_irq() does depends on what happened to the IRQ
number in the meantime. If the chip disposed of its mapping, the
descriptor is gone and the call returns early, or, when it races with
the RCU-deferred free of the descriptor, it touches freed memory, as
reported with virtio-gpio. If the number was handed out again, for
instance because the same chip was set up again, the call finds a live
descriptor without our action and warns:

  Trying to free already-free IRQ 28
  WARNING: kernel/irq/manage.c:1881 at free_irq
  Call Trace:
   linereq_free
   linereq_release
   __fput

Stop edge detection from the device notifier that gpiochip_remove()
already calls into, rather than when the request is released, and only
free the IRQ there if it still has an action: chips that never mark
their lines as used for IRQs, which gpiochip_lock_as_irq() leaves to
the driver, keep theirs past gpiod_free_irqs(). The release path then
finds no IRQ left to free. The notifier takes the request's
config_mutex, which is otherwise only taken by the ioctls, so that it
does not race with a concurrent reconfiguration of the lines.

Reproduced with gpio-sim: request a line with edge detection, set the
chip's configfs "live" attribute to 0 while the request is open, set it
up again and request the same line, then close the first request. Each
of 30 cycles warns as above, for v2 requests with and without software
debouncing and for v1 line events; with this change, 100 cycles of each
pass without a warning.

Fixes: ec8b6f55b981 ("gpio: free irqs that are still requested when the chip is being removed")
Reported-by: Jia Jia <physicalmtea@gmail.com>
Closes: https://lore.kernel.org/all/20260817070429.157993-1-physicalmtea@gmail.com/
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
Changes in v2:
- The IRQ check moved into free_line_irq(), which gets a local for the
  IRQ name, and dropped from the callers (Andy).
- container_of() on one line, loop variable declared in the for (Andy).
- Backtrace cut down (Andy).
- Assisted-by tag.

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

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 5d53bfcdf726..3792ccee4da0 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -544,17 +544,6 @@ struct linereq {
 	 GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE | \
 	 GPIO_V2_LINE_EDGE_FLAGS)
 
-static int linereq_unregistered_notify(struct notifier_block *nb,
-				       unsigned long action, void *data)
-{
-	struct linereq *lr = container_of(nb, struct linereq,
-					  device_unregistered_nb);
-
-	wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
-
-	return NOTIFY_OK;
-}
-
 static void linereq_put_event(struct linereq *lr,
 			      struct gpio_v2_line_event *le)
 {
@@ -609,6 +598,21 @@ static inline void free_irq_label(const char *label)
 	kfree(label);
 }
 
+/*
+ * gpiochip_remove() frees the interrupts still requested on the lines of the
+ * chip being removed, so once the chip is going away the IRQ may no longer be
+ * ours to free.
+ */
+static void free_line_irq(unsigned int irq, void *dev_id)
+{
+	const char *name = NULL;
+
+	if (irq && irq_has_action(irq))
+		name = free_irq(irq, dev_id);
+
+	free_irq_label(name);
+}
+
 #ifdef CONFIG_HTE
 
 static enum hte_return process_hw_ts_thread(void *p)
@@ -974,10 +978,8 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
 
 static void edge_detector_stop(struct line *line)
 {
-	if (line->irq) {
-		free_irq_label(free_irq(line->irq, line));
-		line->irq = 0;
-	}
+	free_line_irq(line->irq, line);
+	line->irq = 0;
 
 #ifdef CONFIG_HTE
 	if (READ_ONCE(line->edflags) & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE)
@@ -992,6 +994,25 @@ static void edge_detector_stop(struct line *line)
 	/* do not change line->level - see comment in debounced_value() */
 }
 
+static int linereq_unregistered_notify(struct notifier_block *nb,
+				       unsigned long action, void *data)
+{
+	struct linereq *lr = container_of(nb, struct linereq, device_unregistered_nb);
+
+	/*
+	 * Stop edge detection now rather than when the request is released:
+	 * by then the IRQ numbers of the removed chip may be in use again.
+	 */
+	scoped_guard(mutex, &lr->config_mutex) {
+		for (unsigned int i = 0; i < lr->num_lines; i++)
+			edge_detector_stop(&lr->lines[i]);
+	}
+
+	wake_up_poll(&lr->wait, EPOLLIN | EPOLLERR);
+
+	return NOTIFY_OK;
+}
+
 static int edge_detector_fifo_init(struct linereq *req)
 {
 	if (kfifo_initialized(&req->events))
@@ -1792,6 +1813,9 @@ static int lineevent_unregistered_notify(struct notifier_block *nb,
 	struct lineevent_state *le = container_of(nb, struct lineevent_state,
 						  device_unregistered_nb);
 
+	free_line_irq(le->irq, le);
+	le->irq = 0;
+
 	wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
 
 	return NOTIFY_OK;
@@ -1871,8 +1895,7 @@ static void lineevent_free(struct lineevent_state *le)
 	if (le->device_unregistered_nb.notifier_call)
 		blocking_notifier_chain_unregister(&le->gdev->device_notifier,
 						   &le->device_unregistered_nb);
-	if (le->irq)
-		free_irq_label(free_irq(le->irq, le));
+	free_line_irq(le->irq, le);
 	if (le->desc)
 		gpiod_free(le->desc);
 	kfree(le->label);
-- 
2.43.0

[PATCH v2 4/6] gpiolib: cdev: don't sleep in the SRCU read section in read()

[Adrian Ambrozewicz]

linereq_read(), lineevent_read() and lineinfo_watch_read() enter the
gdev->srcu read section to check that the chip is still there and stay
in it while they sleep waiting for an event. gpiochip_remove() clears
the chip pointer and then calls synchronize_srcu() on gdev->srcu, which
cannot return while a reader sleeps there. The wake-up meant for that
reader is sent by the device notifier, which runs later in
gpiochip_remove() and would not end the wait anyway, as the wait
condition only looks at the event FIFO. Unless an event arrives, a
process blocked in read() on a line request, a v1 line event or a chip
file with watched lines holds up the removal of the chip until it is
signalled:

  task:init state:D stack:0 pid:1 tgid:1 ppid:0
  Call Trace:
   __synchronize_srcu
   gpiochip_remove
   release_nodes
   devres_release_all
   device_unbind_cleanup
   device_release_driver_internal
   bus_remove_device
   device_del
   platform_device_unregister
   gpio_sim_device_deactivate
   gpio_sim_device_config_live_store
   configfs_write_iter
   vfs_write

and since the device lock is held meanwhile, so does device_shutdown()
on power-off.

Check the chip pointer in a scoped SRCU section only. Nothing after the
check dereferences the chip, the request and its FIFO belong to the
file. Also end the wait once the chip is gone, which the notifiers'
wake-ups now see, and return -ENODEV if there is no event to report.

This dates back to the RW semaphore that the SRCU section replaced,
which these functions also held while sleeping and gpiochip_remove()
took for writing.

Reproduced with gpio-sim: one thread blocked in read() on a line
request while another sets the chip's configfs "live" attribute to 0.
The write does not return until the reader is killed; with this change
it returns at once and read() fails with -ENODEV.

Fixes: bdbbae241a04 ("gpiolib: protect the GPIO device against being dropped while in use by user-space")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
Changes in v2:
- The three chip checks factored into gpiodev_check_chip() (Andy).
- Assisted-by tag.

 drivers/gpio/gpiolib-cdev.c | 50 ++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 3792ccee4da0..6f84aaa95033 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1512,6 +1512,20 @@ static __poll_t linereq_poll(struct file *file,
 	return events;
 }
 
+/*
+ * For the read() paths, which must not stay in the SRCU read section while
+ * sleeping: that would stall gpiochip_remove() until an event arrives.
+ */
+static int gpiodev_check_chip(struct gpio_device *gdev)
+{
+	guard(srcu)(&gdev->srcu);
+
+	if (!rcu_access_pointer(gdev->chip))
+		return -ENODEV;
+
+	return 0;
+}
+
 static ssize_t linereq_read(struct file *file, char __user *buf,
 			    size_t count, loff_t *f_ps)
 {
@@ -1520,10 +1534,9 @@ 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;
+	ret = gpiodev_check_chip(lr->gdev);
+	if (ret)
+		return ret;
 
 	if (count < sizeof(le))
 		return -EINVAL;
@@ -1538,9 +1551,12 @@ 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;
+				if (kfifo_is_empty(&lr->events))
+					return -ENODEV;
 			}
 
 			if (kfifo_out(&lr->events, &le, 1) != 1) {
@@ -1835,10 +1851,9 @@ 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;
+	ret = gpiodev_check_chip(le->gdev);
+	if (ret)
+		return ret;
 
 	/*
 	 * When compatible system call is being used the struct gpioevent_data,
@@ -1866,9 +1881,12 @@ 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;
+				if (kfifo_is_empty(&le->events))
+					return -ENODEV;
 			}
 
 			if (kfifo_out(&le->events, &ge, 1) != 1) {
@@ -2612,10 +2630,9 @@ 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;
+	ret = gpiodev_check_chip(cdev->gdev);
+	if (ret)
+		return ret;
 
 #ifndef CONFIG_GPIO_CDEV_V1
 	event_size = sizeof(struct gpio_v2_line_info_changed);
@@ -2633,9 +2650,12 @@ 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;
+				if (kfifo_is_empty(&cdev->events))
+					return -ENODEV;
 			}
 #ifdef CONFIG_GPIO_CDEV_V1
 			/* must be after kfifo check so watch_abi_version is set */
-- 
2.43.0

[PATCH v2 5/6] gpiolib: cdev: don't leak the IRQ name when the chip is removed

[Adrian Ambrozewicz]

The character device requests line IRQs with a copy of the consumer
label as the name and frees that copy with the name free_irq() returns.
Since commit ec8b6f55b981 ("gpio: free irqs that are still requested
when the chip is being removed"), gpiochip_remove() may free the IRQ
first and drops the name it gets back. The character device then does
not get it from its own free_irq() call, which since the previous patch
it does not even make, so the copy leaks:

  unreferenced object (size 8):
    comm "gpio-hold"
    backtrace:
      kstrdup
      kstrdup_and_replace
      edge_detector_setup
      linereq_create
      gpio_ioctl

Keep the name with the line, or with the v1 line event, and free it
there whoever freed the IRQ.

Reproduced with gpio-sim and a line requested with edge detection and
a consumer label, the chip being removed while the request is open:
kmemleak reports between 17 and 23 such objects after 30 cycles; with
this change, none after 100.

Fixes: ec8b6f55b981 ("gpio: free irqs that are still requested when the chip is being removed")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
Changes in v2:
- Rebased on the free_line_irq() shape from patch 3, so this only
  changes where the name comes from (Andy).
- kmemleak report cut down (Andy).
- Assisted-by tag.

 drivers/gpio/gpiolib-cdev.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 6f84aaa95033..788a36475e5d 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -399,6 +399,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @desc: the GPIO descriptor for this line.
  * @req: the corresponding line request
  * @irq: the interrupt triggered in response to events on this GPIO
+ * @irq_label: the name @irq was requested with
  * @edflags: the edge flags, GPIO_V2_LINE_FLAG_EDGE_RISING and/or
  * GPIO_V2_LINE_FLAG_EDGE_FALLING, indicating the edge detection applied
  * @timestamp_ns: cache for the timestamp storing it between hardirq and
@@ -422,6 +423,7 @@ struct line {
 	 */
 	struct linereq *req;
 	unsigned int irq;
+	char *irq_label;
 	/*
 	 * The flags for the active edge detector configuration.
 	 *
@@ -601,16 +603,18 @@ static inline void free_irq_label(const char *label)
 /*
  * gpiochip_remove() frees the interrupts still requested on the lines of the
  * chip being removed, so once the chip is going away the IRQ may no longer be
- * ours to free.
+ * ours to free. The label is kept by the caller as free_irq() is then not the
+ * one returning it.
  */
-static void free_line_irq(unsigned int irq, void *dev_id)
+static void free_line_irq(unsigned int irq, void *dev_id, char **label)
 {
-	const char *name = NULL;
+	const char *name = *label;
 
 	if (irq && irq_has_action(irq))
-		name = free_irq(irq, dev_id);
+		free_irq(irq, dev_id);
 
 	free_irq_label(name);
+	*label = NULL;
 }
 
 #ifdef CONFIG_HTE
@@ -936,6 +940,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
 				return ret;
 			}
 			line->irq = irq;
+			line->irq_label = label;
 		} else {
 			ret = hte_edge_setup(line, GPIO_V2_LINE_FLAG_EDGE_BOTH);
 			if (ret)
@@ -978,7 +983,7 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
 
 static void edge_detector_stop(struct line *line)
 {
-	free_line_irq(line->irq, line);
+	free_line_irq(line->irq, line, &line->irq_label);
 	line->irq = 0;
 
 #ifdef CONFIG_HTE
@@ -1078,6 +1083,7 @@ static int edge_detector_setup(struct line *line,
 	}
 
 	line->irq = irq;
+	line->irq_label = label;
 	return 0;
 }
 
@@ -1781,6 +1787,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
  * @desc: the GPIO descriptor held by this event
  * @eflags: the event flags this line was requested with
  * @irq: the interrupt that trigger in response to events on this GPIO
+ * @irq_label: the name @irq was requested with
  * @wait: wait queue that handles blocking reads of events
  * @device_unregistered_nb: notifier block for receiving gdev unregister events
  * @events: KFIFO for the GPIO events
@@ -1794,6 +1801,7 @@ struct lineevent_state {
 	struct gpio_desc *desc;
 	u32 eflags;
 	int irq;
+	char *irq_label;
 	wait_queue_head_t wait;
 	struct notifier_block device_unregistered_nb;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
@@ -1829,7 +1837,7 @@ static int lineevent_unregistered_notify(struct notifier_block *nb,
 	struct lineevent_state *le = container_of(nb, struct lineevent_state,
 						  device_unregistered_nb);
 
-	free_line_irq(le->irq, le);
+	free_line_irq(le->irq, le, &le->irq_label);
 	le->irq = 0;
 
 	wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
@@ -1913,7 +1921,7 @@ static void lineevent_free(struct lineevent_state *le)
 	if (le->device_unregistered_nb.notifier_call)
 		blocking_notifier_chain_unregister(&le->gdev->device_notifier,
 						   &le->device_unregistered_nb);
-	free_line_irq(le->irq, le);
+	free_line_irq(le->irq, le, &le->irq_label);
 	if (le->desc)
 		gpiod_free(le->desc);
 	kfree(le->label);
@@ -2152,6 +2160,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	}
 
 	le->irq = irq;
+	le->irq_label = label;
 
 	FD_PREPARE(fdf, O_RDONLY | O_CLOEXEC,
 		   anon_inode_getfile("gpio-event", &lineevent_fileops, le,
-- 
2.43.0

[PATCH v2 6/6] gpiolib: let the character device free its IRQs first on chip removal

[Adrian Ambrozewicz]

gpiochip_remove() force-frees every IRQ still requested on the chip's
lines with gpiochip_free_remaining_irqs() before anything else, then
clears the chip pointer, removes the irqchip, disposing of the IRQ
mappings, and only at the very end, from gpiolib_cdev_unregister(),
calls the device notifier on which line requests and v1 line events
stop edge detection and free their own IRQs. That leaves two windows in
which the character device frees an IRQ it no longer owns:

- The notifier runs after the mappings are gone. If an IRQ number freed
  by gpiochip_irqchip_remove() is allocated to another device in the
  meantime, irq_has_action() finds that device's action and free_irq()
  warns "Trying to free already-free IRQ". IRQs requested through the
  character device after gpiochip_free_remaining_irqs() but before the
  chip pointer is cleared are still installed when their mappings are
  disposed of.

- Releasing a request concurrently with gpiochip_remove() races
  gpiod_free_irqs(), which can free the action between the release
  path's irq_has_action() check and its free_irq(), and then
  gpiochip_irqchip_remove(), which can dispose of the descriptor that
  free_irq() is still using.

Call the device notifier from gpiochip_remove() right after the chip
pointer is cleared and the SRCU grace period has elapsed, when no ioctl
can request new IRQs any more, and only free the remaining IRQs after
it. Have the release paths free their IRQs before unregistering from the
notifier, under the lock that the notifier callback also takes, so that
the notifier does not return while a release is still freeing an IRQ.
For v1 line events that needs a new mutex; line requests use their
config_mutex, which now has to be initialized before the first error
path. As the chip pointer is NULL by the time gpiod_free_irqs() runs,
have it call gc->to_irq() directly rather than gpiod_to_irq().

Every IRQ requested through the character device is thus freed exactly
once, by the notifier or by the release path, and before
gpiod_free_irqs() and the removal of the irqchip.

Moving the character device's part of the removal earlier was proposed
by Herve Codina before gpiochip_free_remaining_irqs() existed; with that
in place, only the order of the two is left to fix.

Reproduced with a test-only GPIO chip that uses the gpiolib hierarchical
irqchip, unbound from its driver with lines requested through the
character device. For the first window, the chip hands the IRQ number of
line 0 to a dummy handler while gpiochip_irqchip_remove() disposes of
the mappings, and the requests are closed after the unbind. For the
second, it sleeps in its irq_bus_lock() callback once removal has
started, and the request is closed while gpiochip_remove() is in
free_irq(). Each of 30 cycles warns "Trying to free already-free IRQ" in
both cases, for v2 line requests and v1 line events alike; with this
change none does.

Link: https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
Fixes: ec8b6f55b981 ("gpio: free irqs that are still requested when the chip is being removed")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
---
Changes in v2:
- mutex_destroy() for config_mutex and irq_lock (Andy).
- The missing ->to_irq() case folded into the irq <= 0 check (Andy).
- Assisted-by tag.

 drivers/gpio/gpiolib-cdev.c | 36 ++++++++++++++++++++++++++++--------
 drivers/gpio/gpiolib.c      | 29 ++++++++++++++++++++++++-----
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 788a36475e5d..b4b3c9ac54c2 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1588,19 +1588,29 @@ static void linereq_free(struct linereq *lr)
 {
 	unsigned int i;
 
+	/*
+	 * Stop edge detection while still registered for the notification, so
+	 * that gpiochip_remove() does not get past it until the IRQs are freed.
+	 */
+	scoped_guard(mutex, &lr->config_mutex) {
+		for (i = 0; i < lr->num_lines; i++) {
+			if (lr->lines[i].desc)
+				edge_detector_stop(&lr->lines[i]);
+		}
+	}
+
 	if (lr->device_unregistered_nb.notifier_call)
 		blocking_notifier_chain_unregister(&lr->gdev->device_notifier,
 						   &lr->device_unregistered_nb);
 
 	for (i = 0; i < lr->num_lines; i++) {
-		if (lr->lines[i].desc) {
-			edge_detector_stop(&lr->lines[i]);
+		if (lr->lines[i].desc)
 			gpiod_free(lr->lines[i].desc);
-		}
 	}
 	kfifo_free(&lr->events);
 	kfree(lr->label);
 	gpio_device_put(lr->gdev);
+	mutex_destroy(&lr->config_mutex);
 	kvfree(lr);
 }
 
@@ -1671,6 +1681,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 	if (!lr)
 		return -ENOMEM;
 	lr->num_lines = ulr.num_lines;
+	mutex_init(&lr->config_mutex);
 
 	lr->gdev = gpio_device_get(gdev);
 
@@ -1688,7 +1699,6 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
 			return -ENOMEM;
 	}
 
-	mutex_init(&lr->config_mutex);
 	init_waitqueue_head(&lr->wait);
 	INIT_KFIFO(lr->events);
 	lr->event_buffer_size = ulr.event_buffer_size;
@@ -1788,6 +1798,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
  * @eflags: the event flags this line was requested with
  * @irq: the interrupt that trigger in response to events on this GPIO
  * @irq_label: the name @irq was requested with
+ * @irq_lock: serializes freeing @irq on release and on chip removal
  * @wait: wait queue that handles blocking reads of events
  * @device_unregistered_nb: notifier block for receiving gdev unregister events
  * @events: KFIFO for the GPIO events
@@ -1802,6 +1813,8 @@ struct lineevent_state {
 	u32 eflags;
 	int irq;
 	char *irq_label;
+	/* release vs. chip removal notifier */
+	struct mutex irq_lock;
 	wait_queue_head_t wait;
 	struct notifier_block device_unregistered_nb;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
@@ -1837,8 +1850,10 @@ static int lineevent_unregistered_notify(struct notifier_block *nb,
 	struct lineevent_state *le = container_of(nb, struct lineevent_state,
 						  device_unregistered_nb);
 
-	free_line_irq(le->irq, le, &le->irq_label);
-	le->irq = 0;
+	scoped_guard(mutex, &le->irq_lock) {
+		free_line_irq(le->irq, le, &le->irq_label);
+		le->irq = 0;
+	}
 
 	wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
 
@@ -1918,14 +1933,19 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
 
 static void lineevent_free(struct lineevent_state *le)
 {
+	/* As in linereq_free(), free the IRQ before unregistering. */
+	scoped_guard(mutex, &le->irq_lock) {
+		free_line_irq(le->irq, le, &le->irq_label);
+		le->irq = 0;
+	}
 	if (le->device_unregistered_nb.notifier_call)
 		blocking_notifier_chain_unregister(&le->gdev->device_notifier,
 						   &le->device_unregistered_nb);
-	free_line_irq(le->irq, le, &le->irq_label);
 	if (le->desc)
 		gpiod_free(le->desc);
 	kfree(le->label);
 	gpio_device_put(le->gdev);
+	mutex_destroy(&le->irq_lock);
 	kfree(le);
 }
 
@@ -2097,6 +2117,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 	le = kzalloc_obj(*le);
 	if (!le)
 		return -ENOMEM;
+	mutex_init(&le->irq_lock);
 	le->gdev = gpio_device_get(gdev);
 
 	if (eventreq.consumer_label[0] != '\0') {
@@ -2832,5 +2853,4 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
 {
 	destroy_workqueue(gdev->line_state_wq);
 	cdev_device_del(&gdev->chrdev, &gdev->dev);
-	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
 }
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 31ccdd373765..7dd730228b05 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -851,11 +851,23 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 
-static void gpiod_free_irqs(struct gpio_desc *desc)
+static void gpiod_free_irqs(struct gpio_chip *gc, struct gpio_desc *desc)
 {
-	int irq = gpiod_to_irq(desc);
-	struct irq_desc *irqd = irq_to_desc(irq);
+	struct irq_desc *irqd;
 	void *cookie;
+	int irq;
+
+	/* Called with gdev->chip already NULL, so gpiod_to_irq() can't be used. */
+	if (gc->to_irq)
+		irq = gc->to_irq(gc, gpiod_hwgpio(desc));
+	else
+		irq = 0;
+	if (irq <= 0)
+		return;
+
+	irqd = irq_to_desc(irq);
+	if (!irqd)
+		return;
 
 	for (;;) {
 		/*
@@ -887,7 +899,7 @@ static void gpiochip_free_remaining_irqs(struct gpio_chip *gc)
 	struct gpio_desc *desc;
 
 	for_each_gpio_desc_with_flag(gc, desc, GPIOD_FLAG_USED_AS_IRQ)
-		gpiod_free_irqs(desc);
+		gpiod_free_irqs(gc, desc);
 }
 
 static void gpiodev_release(struct device *dev)
@@ -1422,7 +1434,6 @@ void gpiochip_remove(struct gpio_chip *gc)
 	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
 	gpiochip_sysfs_unregister(gc);
 	gpiochip_free_hogs(gc);
-	gpiochip_free_remaining_irqs(gc);
 
 	scoped_guard(mutex, &gpio_devices_lock)
 		list_del_rcu(&gdev->list);
@@ -1431,6 +1442,14 @@ void gpiochip_remove(struct gpio_chip *gc)
 	/* Numb the device, cancelling all outstanding operations */
 	rcu_assign_pointer(gdev->chip, NULL);
 	synchronize_srcu(&gdev->srcu);
+
+	/*
+	 * No new interrupts can be requested through the character device now.
+	 * Have it free the ones it holds before forcibly freeing the rest, and
+	 * both before the irqchip and its mappings go away.
+	 */
+	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
+	gpiochip_free_remaining_irqs(gc);
 	gpio_device_teardown_shared(gdev);
 	gpiochip_irqchip_remove(gc);
 	acpi_gpiochip_remove(gc);
-- 
2.43.0

Re: [PATCH v2 6/6] gpiolib: let the character device free its IRQs first on chip removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpio: missing cdev notifier and IRQ cleanup in probe error path
--

commit 97979b5ca9a75aef3162272761d1c98137dc48bb
Author: Adrian Ambrozewicz <aambrozewicz@nvidia.com>

gpiolib: let the character device free its IRQs first on chip removal

This patch moves the device notifier call from gpiolib_cdev_unregister()
to gpiochip_remove() to ensure the character device frees its irqs before
the irqchip and mappings are removed. It also serializes irq freeing in
the release paths using locks to prevent races during chip removal.

Link: https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/

> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 788a36475e5d4..b4b3c9ac54c27 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c

[ ... ]

> @@ -2832,5 +2853,4 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
>  {
>  	destroy_workqueue(gdev->line_state_wq);
>  	cdev_device_del(&gdev->chrdev, &gdev->dev);
> -	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
>  }

[Severity: High]
With the notifier call removed from here and moved exclusively to
gpiochip_remove(), does this break the error teardown path in
gpiochip_setup_dev()?

If a failure occurs during initialization, the error path unwinds by calling
gcdev_unregister(), which leads here:

drivers/gpio/gpiolib.c:gpiochip_setup_dev() {
    ...
    ret = gcdev_register(gc, gpio_devt);
    if (ret)
        return ret;

    ret = gpiochip_sysfs_register(gc);
    if (ret)
        goto err_remove_device;
    ...
err_remove_device:
    gcdev_unregister(gdev);
    return ret;
}

Since the notifier is no longer called in gpiolib_cdev_unregister(),
consumers aren't notified to stop active edge detectors and free their irqs.

Additionally, because gpiochip_remove() is not called on this error path,
gpiochip_free_remaining_irqs() is also skipped.

If userspace raced to open the cdev and request an irq during the brief
window it was exposed, could this leave the irq active? The error path
then proceeds to call gpiochip_irqchip_remove(), destroying the irq
domain. When the userspace application later closes the file, it would
attempt to free the irq against the destroyed domain, leading to a
use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-gpiolib-chip-removal-v2-0-aambrozewicz@nvidia.com?part=6

Re: [PATCH v2 6/6] gpiolib: let the character device free its IRQs first on chip removal

[Adrian Ambrozewicz]

On Fri, Oct 16, 2026 at 09:45:44AM +0000, sashiko-bot@kernel.org wrote:
> With the notifier call removed from here and moved exclusively to
> gpiochip_remove(), does this break the error teardown path in
> gpiochip_setup_dev()?

No, an IRQ requested in that window is left behind without this patch
too. The notifier handlers in gpiolib-cdev.c only wake up waiters
(linereq_unregistered_notify(), lineevent_unregistered_notify() and
gpio_device_unregistered_notify() just call wake_up_poll()), so the call
removed here never freed anything. On that path the woken waiters also
find nothing to report, as gdev->chip is still set.

The error path of gpiochip_add_data_with_key() after the character
device is visible is an existing gap: unlike gpiochip_remove(), it
neither clears gdev->chip nor frees the remaining IRQs before removing
the irqchip. That is separate from this series; I'll send a fix for it
on its own.

Re: [PATCH v2 2/6] gpiolib: free the label of lines released after their chip was removed

[Andy Shevchenko]

On Fri, Oct 16, 2026 at 12:23:34PM +0300, Adrian Ambrozewicz wrote:
> gpiod_free_commit() only drops the consumer label while the chip is
> still there:
> 
> 	if (guard.gc && test_bit(GPIOD_FLAG_REQUESTED, &flags)) {
> 		...
> 		desc_set_label(desc, NULL);
> 
> A line that is still requested when gpiochip_remove() runs, for instance
> by a character device line request that user space keeps open, is
> released only afterwards, when gdev->chip is already NULL. Its label is
> then never freed, as gpiodev_release() frees the descriptor array but
> not the labels, and kmemleak reports one object per such line:
> 
>   unreferenced object (size 32):
>     comm "gpio-hold"
>     backtrace:

Be consistent, in other patch it's called Call Trace:. Is it really different
wording in the Oopses you got?

>       desc_set_label
>       gpiod_request_commit
>       gpiod_request
>       linereq_create
>       gpio_ioctl
> 
> Commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU")
> cleared the label unconditionally; it moved under the chip check with
> the removal of gpio_lock. Return early only for a line that is not
> requested on a live chip, and clear the label before the chip check, so
> that it is also freed when the chip is gone. The ->free() callback, the
> flags update and the line state notification still need the chip and
> stay under the check; with the chip gone, no character device file can
> receive the notification any more.
> 
> The label is now often freed just before the last reference to the GPIO
> device goes away, which depends on gpiodev_release() waiting for pending
> label frees (previous patch).
> 
> Reproduced with gpio-sim: request a line through the character device,
> set the chip's configfs "live" attribute to 0 while the request is open,
> then close it. Without this change kmemleak reports between 15 and 30
> such objects after 30 cycles; with it, none after 100.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] gpiolib: let the character device free its IRQs first on chip removal

[Kent Gibson]

On Fri, Oct 16, 2026 at 12:23:38PM +0300, Adrian Ambrozewicz wrote:
> gpiochip_remove() force-frees every IRQ still requested on the chip's
> lines with gpiochip_free_remaining_irqs() before anything else, then
> clears the chip pointer, removes the irqchip, disposing of the IRQ
> mappings, and only at the very end, from gpiolib_cdev_unregister(),
> calls the device notifier on which line requests and v1 line events
> stop edge detection and free their own IRQs. That leaves two windows in
> which the character device frees an IRQ it no longer owns:
> 
> - The notifier runs after the mappings are gone. If an IRQ number freed
>   by gpiochip_irqchip_remove() is allocated to another device in the
>   meantime, irq_has_action() finds that device's action and free_irq()
>   warns "Trying to free already-free IRQ". IRQs requested through the
>   character device after gpiochip_free_remaining_irqs() but before the
>   chip pointer is cleared are still installed when their mappings are
>   disposed of.
> 
> - Releasing a request concurrently with gpiochip_remove() races
>   gpiod_free_irqs(), which can free the action between the release
>   path's irq_has_action() check and its free_irq(), and then
>   gpiochip_irqchip_remove(), which can dispose of the descriptor that
>   free_irq() is still using.
> 
> Call the device notifier from gpiochip_remove() right after the chip
> pointer is cleared and the SRCU grace period has elapsed, when no ioctl
> can request new IRQs any more, and only free the remaining IRQs after
> it. Have the release paths free their IRQs before unregistering from the
> notifier, under the lock that the notifier callback also takes, so that
> the notifier does not return while a release is still freeing an IRQ.
> For v1 line events that needs a new mutex; line requests use their
> config_mutex, which now has to be initialized before the first error
> path. As the chip pointer is NULL by the time gpiod_free_irqs() runs,
> have it call gc->to_irq() directly rather than gpiod_to_irq().
> 
> Every IRQ requested through the character device is thus freed exactly
> once, by the notifier or by the release path, and before
> gpiod_free_irqs() and the removal of the irqchip.
> 
> Moving the character device's part of the removal earlier was proposed
> by Herve Codina before gpiochip_free_remaining_irqs() existed; with that
> in place, only the order of the two is left to fix.
> 
> Reproduced with a test-only GPIO chip that uses the gpiolib hierarchical
> irqchip, unbound from its driver with lines requested through the
> character device. For the first window, the chip hands the IRQ number of
> line 0 to a dummy handler while gpiochip_irqchip_remove() disposes of
> the mappings, and the requests are closed after the unbind. For the
> second, it sleeps in its irq_bus_lock() callback once removal has
> started, and the request is closed while gpiochip_remove() is in
> free_irq(). Each of 30 cycles warns "Trying to free already-free IRQ" in
> both cases, for v2 line requests and v1 line events alike; with this
> change none does.
> 
> Link: https://lore.kernel.org/all/20240227113426.253232-1-herve.codina@bootlin.com/
> Fixes: ec8b6f55b981 ("gpio: free irqs that are still requested when the chip is being removed")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Adrian Ambrozewicz <aambrozewicz@nvidia.com>
> ---
> Changes in v2:
> - mutex_destroy() for config_mutex and irq_lock (Andy).
> - The missing ->to_irq() case folded into the irq <= 0 check (Andy).
> - Assisted-by tag.
> 
>  drivers/gpio/gpiolib-cdev.c | 36 ++++++++++++++++++++++++++++--------
>  drivers/gpio/gpiolib.c      | 29 ++++++++++++++++++++++++-----
>  2 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 788a36475e5d..b4b3c9ac54c2 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1588,19 +1588,29 @@ static void linereq_free(struct linereq *lr)
>  {
>  	unsigned int i;
>  
> +	/*
> +	 * Stop edge detection while still registered for the notification, so
> +	 * that gpiochip_remove() does not get past it until the IRQs are freed.
> +	 */
> +	scoped_guard(mutex, &lr->config_mutex) {
> +		for (i = 0; i < lr->num_lines; i++) {
> +			if (lr->lines[i].desc)
> +				edge_detector_stop(&lr->lines[i]);
> +		}
> +	}
> +
>  	if (lr->device_unregistered_nb.notifier_call)
>  		blocking_notifier_chain_unregister(&lr->gdev->device_notifier,
>  						   &lr->device_unregistered_nb);
>  
>  	for (i = 0; i < lr->num_lines; i++) {
> -		if (lr->lines[i].desc) {
> -			edge_detector_stop(&lr->lines[i]);
> +		if (lr->lines[i].desc)
>  			gpiod_free(lr->lines[i].desc);
> -		}
>  	}
>  	kfifo_free(&lr->events);
>  	kfree(lr->label);
>  	gpio_device_put(lr->gdev);
> +	mutex_destroy(&lr->config_mutex);
>  	kvfree(lr);
>  }
>  
> @@ -1671,6 +1681,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>  	if (!lr)
>  		return -ENOMEM;
>  	lr->num_lines = ulr.num_lines;
> +	mutex_init(&lr->config_mutex);
>  
>  	lr->gdev = gpio_device_get(gdev);
>  
> @@ -1688,7 +1699,6 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>  			return -ENOMEM;
>  	}
>  
> -	mutex_init(&lr->config_mutex);
>  	init_waitqueue_head(&lr->wait);
>  	INIT_KFIFO(lr->events);
>  	lr->event_buffer_size = ulr.event_buffer_size;
> @@ -1788,6 +1798,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>   * @eflags: the event flags this line was requested with
>   * @irq: the interrupt that trigger in response to events on this GPIO
>   * @irq_label: the name @irq was requested with
> + * @irq_lock: serializes freeing @irq on release and on chip removal
>   * @wait: wait queue that handles blocking reads of events
>   * @device_unregistered_nb: notifier block for receiving gdev unregister events
>   * @events: KFIFO for the GPIO events
> @@ -1802,6 +1813,8 @@ struct lineevent_state {
>  	u32 eflags;
>  	int irq;
>  	char *irq_label;
> +	/* release vs. chip removal notifier */
> +	struct mutex irq_lock;
>  	wait_queue_head_t wait;
>  	struct notifier_block device_unregistered_nb;
>  	DECLARE_KFIFO(events, struct gpioevent_data, 16);
> @@ -1837,8 +1850,10 @@ static int lineevent_unregistered_notify(struct notifier_block *nb,
>  	struct lineevent_state *le = container_of(nb, struct lineevent_state,
>  						  device_unregistered_nb);
>  
> -	free_line_irq(le->irq, le, &le->irq_label);
> -	le->irq = 0;
> +	scoped_guard(mutex, &le->irq_lock) {
> +		free_line_irq(le->irq, le, &le->irq_label);
> +		le->irq = 0;
> +	}
>  
>  	wake_up_poll(&le->wait, EPOLLIN | EPOLLERR);
>  
> @@ -1918,14 +1933,19 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
>  
>  static void lineevent_free(struct lineevent_state *le)
>  {
> +	/* As in linereq_free(), free the IRQ before unregistering. */

First part of the comment can rapidly become stale, and the last part
doesn't add anything the code doesn't already say.

> +	scoped_guard(mutex, &le->irq_lock) {
> +		free_line_irq(le->irq, le, &le->irq_label);
> +		le->irq = 0;
> +	}
>  	if (le->device_unregistered_nb.notifier_call)
>  		blocking_notifier_chain_unregister(&le->gdev->device_notifier,
>  						   &le->device_unregistered_nb);
> -	free_line_irq(le->irq, le, &le->irq_label);
>  	if (le->desc)
>  		gpiod_free(le->desc);
>  	kfree(le->label);
>  	gpio_device_put(le->gdev);
> +	mutex_destroy(&le->irq_lock);
>  	kfree(le);
>  }
>  
> @@ -2097,6 +2117,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>  	le = kzalloc_obj(*le);
>  	if (!le)
>  		return -ENOMEM;
> +	mutex_init(&le->irq_lock);
>  	le->gdev = gpio_device_get(gdev);
>  
>  	if (eventreq.consumer_label[0] != '\0') {
> @@ -2832,5 +2853,4 @@ void gpiolib_cdev_unregister(struct gpio_device *gdev)
>  {
>  	destroy_workqueue(gdev->line_state_wq);
>  	cdev_device_del(&gdev->chrdev, &gdev->dev);
> -	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
>  }
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 31ccdd373765..7dd730228b05 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -851,11 +851,23 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc,
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
>  
> -static void gpiod_free_irqs(struct gpio_desc *desc)
> +static void gpiod_free_irqs(struct gpio_chip *gc, struct gpio_desc *desc)
>  {
> -	int irq = gpiod_to_irq(desc);
> -	struct irq_desc *irqd = irq_to_desc(irq);
> +	struct irq_desc *irqd;
>  	void *cookie;
> +	int irq;
> +
> +	/* Called with gdev->chip already NULL, so gpiod_to_irq() can't be used. */
> +	if (gc->to_irq)
> +		irq = gc->to_irq(gc, gpiod_hwgpio(desc));
> +	else
> +		irq = 0;
> +	if (irq <= 0)
> +		return;

Perhaps I'm channelling Andy here, but how about:

	if (!gc->to_irq)
        return;
	irq = gc->to_irq(gc, gpiod_hwgpio(desc));
	if (irq <= 0)
		return;

> +
> +	irqd = irq_to_desc(irq);
> +	if (!irqd)
> +		return;
>  
>  	for (;;) {
>  		/*
> @@ -887,7 +899,7 @@ static void gpiochip_free_remaining_irqs(struct gpio_chip *gc)
>  	struct gpio_desc *desc;
>  
>  	for_each_gpio_desc_with_flag(gc, desc, GPIOD_FLAG_USED_AS_IRQ)
> -		gpiod_free_irqs(desc);
> +		gpiod_free_irqs(gc, desc);
>  }
>  
>  static void gpiodev_release(struct device *dev)
> @@ -1422,7 +1434,6 @@ void gpiochip_remove(struct gpio_chip *gc)
>  	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
>  	gpiochip_sysfs_unregister(gc);
>  	gpiochip_free_hogs(gc);
> -	gpiochip_free_remaining_irqs(gc);
>  
>  	scoped_guard(mutex, &gpio_devices_lock)
>  		list_del_rcu(&gdev->list);
> @@ -1431,6 +1442,14 @@ void gpiochip_remove(struct gpio_chip *gc)
>  	/* Numb the device, cancelling all outstanding operations */
>  	rcu_assign_pointer(gdev->chip, NULL);
>  	synchronize_srcu(&gdev->srcu);
> +
> +	/*
> +	 * No new interrupts can be requested through the character device now.
> +	 * Have it free the ones it holds before forcibly freeing the rest, and
> +	 * both before the irqchip and its mappings go away.
> +	 */
> +	blocking_notifier_call_chain(&gdev->device_notifier, 0, NULL);
> +	gpiochip_free_remaining_irqs(gc);
>  	gpio_device_teardown_shared(gdev);
>  	gpiochip_irqchip_remove(gc);
>  	acpi_gpiochip_remove(gc);
> -- 
> 2.43.0
> 

That's all I've got from a first pass of the whole series - looks great.

Cheers,
Kent.

Re: [PATCH v2 5/6] gpiolib: cdev: don't leak the IRQ name when the chip is removed

[Andy Shevchenko]

On Fri, Oct 16, 2026 at 12:23:37PM +0300, Adrian Ambrozewicz wrote:
> The character device requests line IRQs with a copy of the consumer
> label as the name and frees that copy with the name free_irq() returns.
> Since commit ec8b6f55b981 ("gpio: free irqs that are still requested
> when the chip is being removed"), gpiochip_remove() may free the IRQ
> first and drops the name it gets back. The character device then does
> not get it from its own free_irq() call, which since the previous patch
> it does not even make, so the copy leaks:
> 
>   unreferenced object (size 8):
>     comm "gpio-hold"
>     backtrace:
>       kstrdup
>       kstrdup_and_replace
>       edge_detector_setup
>       linereq_create
>       gpio_ioctl
> 
> Keep the name with the line, or with the v1 line event, and free it
> there whoever freed the IRQ.
> 
> Reproduced with gpio-sim and a line requested with edge detection and
> a consumer label, the chip being removed while the request is open:
> kmemleak reports between 17 and 23 such objects after 30 cycles; with
> this change, none after 100.

...

> struct line {

>  	struct linereq *req;
>  	unsigned int irq;
> +	char *irq_label;

Have you checked this with `pahole`? Is it good layout?

...

> struct lineevent_state {

>  	struct gpio_desc *desc;
>  	u32 eflags;
>  	int irq;
> +	char *irq_label;
>  	wait_queue_head_t wait;
>  	struct notifier_block device_unregistered_nb;
>  	DECLARE_KFIFO(events, struct gpioevent_data, 16);

Ditto.

-- 
With Best Regards,
Andy Shevchenko