[PATCH 0/3] leds: trigger: netdev: fix sysfs_update_group() races

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>

The netdev LED trigger refreshes the link_speed attribute group with
sysfs_update_group() from several contexts (the NETDEV_CHANGE notifier,
device_name writes and, indirectly, trigger (de)activation) that share no
common lock.  On a board that emits PHY link events while the trigger is
being (re)armed during boot this is observed as a hard sysfs failure:

  sysfs: cannot create duplicate filename '...green:lan/link_10'
  ...
  led_trigger_set
  led_trigger_write

Patches 1 and 2 close this really observed race.  Patch 1 serializes
device_name_store()'s sysfs_update_group() (and its is_visible callback's
read of ->supported_link_modes) against the notifier under
trigger_data->lock.  Patch 2 removes the remaining collision by construction:
the link_speed group is dropped from netdev_led_trigger.groups so the LED
core's device_add_groups() / device_remove_groups() no longer touch it, and
the trigger owns its lifecycle via sysfs_create_group() in activate() and
sysfs_remove_group() in deactivate().

The window between activate() and device_add_groups() in led_trigger_set()
is narrow, so to reproduce it reliably I artificially widened it with the
debug patch below:

  --- a/drivers/leds/led-triggers.c
  +++ b/drivers/leds/led-triggers.c
  @@ -12,6 +12,7 @@
   #include <linux/list.h>
   #include <linux/spinlock.h>
   #include <linux/device.h>
  +#include <linux/delay.h>
   #include <linux/timer.h>
   #include <linux/rwsem.h>
   #include <linux/leds.h>
  @@ -223,6 +224,8 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
                   if (ret)
                           goto err_activate;

  +               msleep(5000);
  +
                   ret = device_add_groups(led_cdev->dev, trig->groups);
                   if (ret) {
                           dev_err(led_cdev->dev, "Failed to add trigger attributes\n");

With that msleep() in place, writing the trigger while a NETDEV_CHANGE is
delivered from the linkwatch workqueue reproduces the duplicate-filename
splat every time; with patches 1 and 2 applied it no longer triggers.

Patch 3 addresses a separate race that I deduced by inspection but did not
try to reproduce in practice: netdev_led_attr_store() and interval_store()
mutate the shared trigger_data state (a non-atomic read-modify-write of
->mode, ->hw_control and set_baseline_state()) without holding
trigger_data->lock, so they can race with each other and with the notifier.
It is fixed by taking trigger_data->lock around those updates.

The netdev LED trigger refreshes the link_speed attribute group with
sysfs_update_group() from several contexts (the NETDEV_CHANGE notifier,
device_name writes and, indirectly, trigger (de)activation) that share no
common lock.  On a board that emits PHY link events while the trigger is
being (re)armed during boot this is observed as a hard sysfs failure:

  sysfs: cannot create duplicate filename '...green:lan/link_10'
  ...
  led_trigger_set
  led_trigger_write

Patches 1 and 2 close this really observed race.  Patch 1 serializes
device_name_store()'s sysfs_update_group() (and its is_visible callback's
read of ->supported_link_modes) against the notifier under
trigger_data->lock.  Patch 2 removes the remaining collision by construction:
the link_speed group is dropped from netdev_led_trigger.groups so the LED
core's device_add_groups() / device_remove_groups() no longer touch it, and
the trigger owns its lifecycle via sysfs_create_group() in activate() and
sysfs_remove_group() in deactivate().

The window between activate() and device_add_groups() in led_trigger_set()
is narrow, so to reproduce it reliably I artificially widened it with the
debug patch below:

  --- a/drivers/leds/led-triggers.c
  +++ b/drivers/leds/led-triggers.c
  @@ -12,6 +12,7 @@
   #include <linux/list.h>
   #include <linux/spinlock.h>
   #include <linux/device.h>
  +#include <linux/delay.h>
   #include <linux/timer.h>
   #include <linux/rwsem.h>
   #include <linux/leds.h>
  @@ -223,6 +224,8 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
                   if (ret)
                           goto err_activate;

  +               msleep(5000);
  +
                   ret = device_add_groups(led_cdev->dev, trig->groups);
                   if (ret) {
                           dev_err(led_cdev->dev, "Failed to add trigger attributes\n");

With that msleep() in place, writing the trigger while a NETDEV_CHANGE is
delivered from the linkwatch workqueue reproduces the duplicate-filename
splat every time; with patches 1 and 2 applied it no longer triggers.

Patch 3 addresses a separate race that I deduced by inspection but did not
try to reproduce in practice: netdev_led_attr_store() and interval_store()
mutate the shared trigger_data state (a non-atomic read-modify-write of
->mode, ->hw_control and set_baseline_state()) without holding
trigger_data->lock, so they can race with each other and with the notifier.
It is fixed by taking trigger_data->lock around those updates.

The whole series is LOCKDEP-verified.

Alexander Sverdlin (3):
  leds: trigger: netdev: hold lock across sysfs_update_group() in
    device_name_store()
  leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set()
    races
  leds: trigger: netdev: serialize mode/interval stores with trigger
    lock

 drivers/leds/trigger/ledtrig-netdev.c | 67 +++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 5 deletions(-)

-- 
2.55.0