Re: [PATCH 5.14 232/849] leds: trigger: use RCU to protect the led_cdevs list

[Pavel Machek]

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