Re: [PATCH 2/3] leds: trigger: netdev: fix sysfs_update_group() vs led_trigger_set() races

[Andrew Lunn]

On Mon, Sep 14, 2026 at 03:23:51PM +0200, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> 
> The netdev LED trigger calls sysfs_update_group() to refresh link_speed
> attribute visibility on NETDEV_CHANGE events and device_name writes.  The
> link_speed attribute group was listed in netdev_led_trigger.groups, so it
> was also created and destroyed by the LED core via device_add_groups() /
> device_remove_groups() in led_trigger_set(), which runs under
> trigger_lock(W).  These two owners of the same group share no lock and are
> not serialized against each other.
> 
> This was observed as a "cannot create duplicate filename" sysfs splat
> during boot when the netdev trigger is re-armed while PHY link-up events
> are being delivered concurrently:
> 
>   sysfs: cannot create duplicate filename '...green:lan/link_10'
>   ...
>   led_trigger_set
>   led_trigger_write
> 
> Race 1: activate() vs NETDEV_CHANGE (confirmed with reproducer)
> 
>     CPU 0 (led_trigger_set)              CPU 1 (linkwatch workqueue)
>     -----------------------              ---------------------------
>     [holds trigger_lock(W)]
>     activate():
>       register_netdevice_notifier()
>       <- returns
>                                          netdev_trig_notify(NETDEV_CHANGE):
>                                            sysfs_update_group()
>                                              creates "link_10"
>     device_add_groups()
>       creates "link_10" <- EEXIST!
> 
> Race 2: device_remove_groups() vs NETDEV_CHANGE
> 
>     CPU 0 (led_trigger_set)              CPU 1 (linkwatch workqueue)
>     -----------------------              ---------------------------
>     [holds trigger_lock(W)]
>     device_remove_groups()
>       removes "link_10"
>                                          netdev_trig_notify(NETDEV_CHANGE):
>                                            sysfs_update_group()
>                                              creates "link_10" <- ORPHANED
>     deactivate()
>       kfree(trigger_data) <- UAF via orphaned sysfs files
> 
> Serializing the trigger's sysfs_update_group() against the core under
> trigger_lock is not viable: NETDEV_CHANGE is delivered from linkwatch with
> rtnl_mutex held, whereas led_trigger_set() holds trigger_lock(W) and then
> takes rtnl_mutex via activate() -> set_device_name().  Acquiring
> trigger_lock from the NETDEV_CHANGE handler would invert that order and
> deadlock.
> 
> Fix it by construction instead: drop the link_speed group from
> netdev_led_trigger.groups and let the trigger own its lifecycle.  Create
> it with sysfs_create_group() in activate() (before registering the
> notifier) and remove it with sysfs_remove_group() in deactivate() (after
> unregistering the notifier).  The LED core therefore never touches this
> group, so sysfs_update_group() can no longer collide with
> device_add_groups() / device_remove_groups() -- eliminating both races
> above without taking trigger_lock, and hence without the rtnl_mutex
> inversion.
> 
> The only remaining concurrency -- device_name_store() and
> netdev_trig_notify() both calling sysfs_update_group() on the now
> trigger-owned group -- stays serialized by trigger_data->lock.  Creating
> the group before notifier registration and removing it after notifier
> unregistration guarantees that no sysfs_update_group() can run while the
> group is being created or destroyed.
> 
> Fixes: 06cdca014eca ("leds: trigger: netdev: Display only supported link speed attribute")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>

Please reduce the verbosity of the comments in the code.

I know locking is hard, but we don't need a verbose commit message and
verbose code.

	Andrew