Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

leds: trigger: use RCU to protect the led_cdevs list

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@intel.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>

authored by

Johannes Berg and committed by
Pavel Machek
2a5a8fa8 811b5440

+22 -21
+21 -20
drivers/leds/led-triggers.c
··· 157 157 /* Caller must ensure led_cdev->trigger_lock held */ 158 158 int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig) 159 159 { 160 - unsigned long flags; 161 160 char *event = NULL; 162 161 char *envp[2]; 163 162 const char *name; ··· 170 171 171 172 /* Remove any existing trigger */ 172 173 if (led_cdev->trigger) { 173 - write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags); 174 - list_del(&led_cdev->trig_list); 175 - write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, 176 - flags); 174 + spin_lock(&led_cdev->trigger->leddev_list_lock); 175 + list_del_rcu(&led_cdev->trig_list); 176 + spin_unlock(&led_cdev->trigger->leddev_list_lock); 177 + 178 + /* ensure it's no longer visible on the led_cdevs list */ 179 + synchronize_rcu(); 180 + 177 181 cancel_work_sync(&led_cdev->set_brightness_work); 178 182 led_stop_software_blink(led_cdev); 179 183 if (led_cdev->trigger->deactivate) ··· 188 186 led_set_brightness(led_cdev, LED_OFF); 189 187 } 190 188 if (trig) { 191 - write_lock_irqsave(&trig->leddev_list_lock, flags); 192 - list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); 193 - write_unlock_irqrestore(&trig->leddev_list_lock, flags); 189 + spin_lock(&trig->leddev_list_lock); 190 + list_add_tail_rcu(&led_cdev->trig_list, &trig->led_cdevs); 191 + spin_unlock(&trig->leddev_list_lock); 194 192 led_cdev->trigger = trig; 195 193 196 194 if (trig->activate) ··· 225 223 trig->deactivate(led_cdev); 226 224 err_activate: 227 225 228 - write_lock_irqsave(&led_cdev->trigger->leddev_list_lock, flags); 229 - list_del(&led_cdev->trig_list); 230 - write_unlock_irqrestore(&led_cdev->trigger->leddev_list_lock, flags); 226 + spin_lock(&led_cdev->trigger->leddev_list_lock); 227 + list_del_rcu(&led_cdev->trig_list); 228 + spin_unlock(&led_cdev->trigger->leddev_list_lock); 229 + synchronize_rcu(); 231 230 led_cdev->trigger = NULL; 232 231 led_cdev->trigger_data = NULL; 233 232 led_set_brightness(led_cdev, LED_OFF); ··· 288 285 struct led_classdev *led_cdev; 289 286 struct led_trigger *_trig; 290 287 291 - rwlock_init(&trig->leddev_list_lock); 288 + spin_lock_init(&trig->leddev_list_lock); 292 289 INIT_LIST_HEAD(&trig->led_cdevs); 293 290 294 291 down_write(&triggers_list_lock); ··· 381 378 enum led_brightness brightness) 382 379 { 383 380 struct led_classdev *led_cdev; 384 - unsigned long flags; 385 381 386 382 if (!trig) 387 383 return; 388 384 389 - read_lock_irqsave(&trig->leddev_list_lock, flags); 390 - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) 385 + rcu_read_lock(); 386 + list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) 391 387 led_set_brightness(led_cdev, brightness); 392 - read_unlock_irqrestore(&trig->leddev_list_lock, flags); 388 + rcu_read_unlock(); 393 389 } 394 390 EXPORT_SYMBOL_GPL(led_trigger_event); 395 391 ··· 399 397 int invert) 400 398 { 401 399 struct led_classdev *led_cdev; 402 - unsigned long flags; 403 400 404 401 if (!trig) 405 402 return; 406 403 407 - read_lock_irqsave(&trig->leddev_list_lock, flags); 408 - list_for_each_entry(led_cdev, &trig->led_cdevs, trig_list) { 404 + rcu_read_lock(); 405 + list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list) { 409 406 if (oneshot) 410 407 led_blink_set_oneshot(led_cdev, delay_on, delay_off, 411 408 invert); 412 409 else 413 410 led_blink_set(led_cdev, delay_on, delay_off); 414 411 } 415 - read_unlock_irqrestore(&trig->leddev_list_lock, flags); 412 + rcu_read_unlock(); 416 413 } 417 414 418 415 void led_trigger_blink(struct led_trigger *trig,
+1 -1
include/linux/leds.h
··· 360 360 struct led_hw_trigger_type *trigger_type; 361 361 362 362 /* LEDs under control by this trigger (for simple triggers) */ 363 - rwlock_t leddev_list_lock; 363 + spinlock_t leddev_list_lock; 364 364 struct list_head led_cdevs; 365 365 366 366 /* Link to next registered trigger */