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

[sashiko-bot@kernel.org]

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