| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250311145407.7e05b5d9@erd003.prtnl>
Date: Tue, 11 Mar 2025 14:54:07 +0100
From: David Jander <david@...tonic.nl>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Kent Gibson <warthog618@...il.com>, Linus Walleij
<linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org, Bartosz Golaszewski
<bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH RFT] gpio: cdev: use raw notifier
On Tue, 11 Mar 2025 14:19:40 +0100
Bartosz Golaszewski <brgl@...ev.pl> wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> Atomic notifiers call rcu_synchronize() in
> atomic_notifier_chain_unregister() causing a considerable delay in some
> circumstances. Replace the atomic notifier with the raw variant and
> provide synchronization with a read-write spinlock.
>
> Fixes: fcc8b637c542 ("gpiolib: switch the line state notifier to atomic")
> Reported-by: David Jander <david@...tonic.nl>
> Closes: https://lore.kernel.org/all/20250311110034.53959031@erd003.prtnl/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> ---
> drivers/gpio/gpiolib-cdev.c | 15 +++++++++------
> drivers/gpio/gpiolib.c | 8 +++++---
> drivers/gpio/gpiolib.h | 5 ++++-
> 3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 40f76a90fd7d..107d75558b5a 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2729,8 +2729,9 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
> cdev->gdev = gpio_device_get(gdev);
>
> cdev->lineinfo_changed_nb.notifier_call = lineinfo_changed_notify;
> - ret = atomic_notifier_chain_register(&gdev->line_state_notifier,
> - &cdev->lineinfo_changed_nb);
> + scoped_guard(write_lock_irqsave, &gdev->line_state_lock)
> + ret = raw_notifier_chain_register(&gdev->line_state_notifier,
> + &cdev->lineinfo_changed_nb);
> if (ret)
> goto out_free_bitmap;
>
> @@ -2754,8 +2755,9 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
> blocking_notifier_chain_unregister(&gdev->device_notifier,
> &cdev->device_unregistered_nb);
> out_unregister_line_notifier:
> - atomic_notifier_chain_unregister(&gdev->line_state_notifier,
> - &cdev->lineinfo_changed_nb);
> + scoped_guard(write_lock_irqsave, &gdev->line_state_lock)
> + raw_notifier_chain_unregister(&gdev->line_state_notifier,
> + &cdev->lineinfo_changed_nb);
> out_free_bitmap:
> gpio_device_put(gdev);
> bitmap_free(cdev->watched_lines);
> @@ -2779,8 +2781,9 @@ static int gpio_chrdev_release(struct inode *inode, struct file *file)
>
> blocking_notifier_chain_unregister(&gdev->device_notifier,
> &cdev->device_unregistered_nb);
> - atomic_notifier_chain_unregister(&gdev->line_state_notifier,
> - &cdev->lineinfo_changed_nb);
> + scoped_guard(write_lock_irqsave, &gdev->line_state_lock)
> + raw_notifier_chain_unregister(&gdev->line_state_notifier,
> + &cdev->lineinfo_changed_nb);
> bitmap_free(cdev->watched_lines);
> gpio_device_put(gdev);
> kfree(cdev);
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e5eb3f0ee071..b8197502a5ac 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1075,7 +1075,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> }
> }
>
> - ATOMIC_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
> + rwlock_init(&gdev->line_state_lock);
> + RAW_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
> BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
>
> ret = init_srcu_struct(&gdev->srcu);
> @@ -4361,8 +4362,9 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_value_cansleep);
>
> void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action)
> {
> - atomic_notifier_call_chain(&desc->gdev->line_state_notifier,
> - action, desc);
> + guard(read_lock_irqsave)(&desc->gdev->line_state_lock);
> +
> + raw_notifier_call_chain(&desc->gdev->line_state_notifier, action, desc);
> }
>
> /**
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index a738e6c647d8..58f64056de77 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -16,6 +16,7 @@
> #include <linux/gpio/driver.h>
> #include <linux/module.h>
> #include <linux/notifier.h>
> +#include <linux/spinlock.h>
> #include <linux/srcu.h>
> #include <linux/workqueue.h>
>
> @@ -47,6 +48,7 @@
> * @list: links gpio_device:s together for traversal
> * @line_state_notifier: used to notify subscribers about lines being
> * requested, released or reconfigured
> + * @line_state_lock: RW-spinlock protecting the line state notifier
> * @line_state_wq: used to emit line state events from a separate thread in
> * process context
> * @device_notifier: used to notify character device wait queues about the GPIO
> @@ -75,7 +77,8 @@ struct gpio_device {
> const char *label;
> void *data;
> struct list_head list;
> - struct atomic_notifier_head line_state_notifier;
> + struct raw_notifier_head line_state_notifier;
> + rwlock_t line_state_lock;
> struct workqueue_struct *line_state_wq;
> struct blocking_notifier_head device_notifier;
> struct srcu_struct srcu;
>
> ---
> base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
> change-id: 20250311-gpiolib-line-state-raw-notifier-70c1ad3e99eb
>
> Best regards,
Tested-by: David Jander <david@...tonic.nl>
Thanks!
Seems to work correctly under some basic testing.
$ time gpiofind LPOUT0
gpiochip7 9
real 0m 0.02s
user 0m 0.00s
sys 0m 0.01s
$ time gpiodetect
gpiochip0 [GPIOA] (16 lines)
gpiochip1 [GPIOB] (16 lines)
gpiochip10 [GPIOK] (8 lines)
gpiochip11 [GPIOZ] (8 lines)
gpiochip12 [unknown] (22 lines)
gpiochip13 [mcp23s17.0] (16 lines)
gpiochip14 [0-0020] (16 lines)
gpiochip15 [0-0021] (16 lines)
gpiochip2 [GPIOC] (16 lines)
gpiochip3 [GPIOD] (16 lines)
gpiochip4 [GPIOE] (16 lines)
gpiochip5 [GPIOF] (16 lines)
gpiochip6 [GPIOG] (16 lines)
gpiochip7 [GPIOH] (16 lines)
gpiochip8 [GPIOI] (16 lines)
gpiochip9 [GPIOJ] (16 lines)
real 0m 0.03s
user 0m 0.00s
sys 0m 0.01s
Best regards,
--
David Jander
Powered by blists - more mailing lists