On Mon 2021-11-15 17:55:15, Greg Kroah-Hartman wrote:
> From: Johannes Berg <johannes.berg@xxxxxxxxx>
>
> [ Upstream commit 2a5a8fa8b23144d14567d6f8293dd6fbeecee393 ]
>
> Even with the previous commit 27af8e2c90fb
> ("leds: trigger: fix potential deadlock with libata")
> to this file, we still get lockdep unhappy, and Boqun
> explained the report here:
> https://lore.kernel.org/r/YNA+d1X4UkoQ7g8a@boqun-archlinux
>
> Effectively, this means that the read_lock_irqsave() isn't
> enough here because another CPU might be trying to do a
> write lock, and thus block the readers.
>
> This is all pretty messy, but it doesn't seem right that
> the LEDs framework imposes some locking requirements on
> users, in particular we'd have to make the spinlock in the
> iwlwifi driver always disable IRQs, even if we don't need
> that for any other reason, just to avoid this deadlock.
>
> Since writes to the led_cdevs list are rare (and are done
> by userspace), just switch the list to RCU. This costs a
> synchronize_rcu() at removal time so we can ensure things
> are correct, but that seems like a small price to pay for
> getting lock-free iterations and no deadlocks (nor any
> locking requirements imposed on users.)
>
> Signed-off-by: Johannes Berg <johannes.berg@xxxxxxxxx>
> Signed-off-by: Pavel Machek <pavel@xxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
Please drop. We discussed this with Johannes, and it was not marked
for stable on purpose. Bug is rather obscure and change did not have
enough testing.
Best regards,
Pavel
> ---
> drivers/leds/led-triggers.c | 41 +++++++++++++++++++------------------
> include/linux/leds.h | 2 +-
> 2 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 4e7b78a84149b..072491d3e17b0 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -157,7 +157,6 @@ EXPORT_SYMBOL_GPL(led_trigger_read);
> /* Caller must ensure led_cdev->trigger_lock held */
> int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> {
> - unsigned long flags;
> char *event = NULL;
> char *envp[2];
> const char *name;
> @@ -171,10 +170,13 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
>
> /* Remove any existing trigger */
> if (led_cdev->trigger) {
> - write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> - list_del(&led_cdev->trig_list);
> - write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock,
> - flags);
> + spin_lock(&led_cdev->trigger->leddev_list_lock);
> + list_del_rcu(&led_cdev->trig_list);
> + spin_unlock(&led_cdev->trigger->leddev_list_lock);
> +
> + /* ensure it's no longer visible on the led_cdevs list */
> + synchronize_rcu();
> +
> cancel_work_sync(&led_cdev->set_brightness_work);
> led_stop_software_blink(led_cdev);
> if (led_cdev->trigger->deactivate)
> @@ -186,9 +188,9 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
> led_set_brightness(led_cdev, LED_OFF);
> }
> if (trig) {
> - write_lock_irqsave(&trig->leddev_list_lock, flags);
> - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs);
> - write_unlock_irqrestore(&trig->leddev_list_lock, flags);
> + spin_lock(&trig->leddev_list_lock);
> + list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs);
> + spin_unlock(&trig->leddev_list_lock);
> led_cdev->trigger = trig;
>
> if (trig->activate)
> @@ -223,9 +225,10 @@ err_add_groups:
> trig->deactivate(led_cdev);
> err_activate:
>
> - write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags);
> - list_del(&led_cdev->trig_list);
> - write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags);
> + spin_lock(&led_cdev->trigger->leddev_list_lock);
> + list_del_rcu(&led_cdev->trig_list);
> + spin_unlock(&led_cdev->trigger->leddev_list_lock);
> + synchronize_rcu();
> led_cdev->trigger = NULL;
> led_cdev->trigger_data = NULL;
> led_set_brightness(led_cdev, LED_OFF);
> @@ -285,7 +288,7 @@ int led_trigger_register(struct led_trigger *trig)
> struct led_classdev *led_cdev;
> struct led_trigger *_trig;
>
> - rwlock_init(&trig->leddev_list_lock);
> + spin_lock_init(&trig->leddev_list_lock);
> INIT_LIST_HEAD(&trig->led_cdevs);
>
> down_write(&triggers_list_lock);
> @@ -378,15 +381,14 @@ void led_trigger_event(struct led_trigger *trig,
> enum led_brightness brightness)
> {
> struct led_classdev *led_cdev;
> - unsigned long flags;
>
> if (!trig)
> return;
>
> - read_lock_irqsave(&trig->leddev_list_lock, flags);
> - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list)
> + rcu_read_lock();
> + list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list)
> led_set_brightness(led_cdev, brightness);
> - read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> + rcu_read_unlock();
> }
> EXPORT_SYMBOL_GPL(led_trigger_event);
>
> @@ -397,20 +399,19 @@ static void led_trigger_blink_setup(struct led_trigger *trig,
> int invert)
> {
> struct led_classdev *led_cdev;
> - unsigned long flags;
>
> if (!trig)
> return;
>
> - read_lock_irqsave(&trig->leddev_list_lock, flags);
> - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) {
> + rcu_read_lock();
> + list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) {
> if (oneshot)
> led_blink_set_oneshot(led_cdev, delay_on, delay_off,
> invert);
> else
> led_blink_set(led_cdev, delay_on, delay_off);
> }
> - read_unlock_irqrestore(&trig->leddev_list_lock, flags);
> + rcu_read_unlock();
> }
>
> void led_trigger_blink(struct led_trigger *trig,
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 329fd914cf243..fa59326b0ad9f 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -354,7 +354,7 @@ struct led_trigger {
> struct led_hw_trigger_type *trigger_type;
>
> /* LEDs under control by this trigger (for simple triggers) */
> - rwlock_t leddev_list_lock;
> + spinlock_t leddev_list_lock;
> struct list_head led_cdevs;
>
> /* Link to next registered trigger */
--
http://www.livejournal.com/~pavelmachek
Attachment:
signature.asc
Description: Digital signature
