Re: [PATCH v5 4/4] leds: trigger: netdev: Serialize mode/interval stores with trigger lock

["Sverdlin, Alexander" <alexander.sverdlin@siemens.com>]

Hi all,

On Tue, 2026-10-20 at 12:59 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] leds: trigger: netdev: unlocked hw_control check in interval_store()
> - [Medium] leds: trigger: netdev: missing carrier_link_up check in netdev_trig_sw_blink()

ACK, both findings will be fixed in v6.

> --
> 
> commit fa618832fa8e967d013f87c080cded7d0b155827
> Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> leds: trigger: netdev: Serialize mode/interval stores with trigger lock
> 
> This commit serializes sysfs mode and interval attribute stores in the
> netdev LED trigger using trigger_data->lock, switches to asynchronous
> work cancellation to avoid deadlocking with netdev_trig_work(), and adds
> a netdev_trig_sw_blink() helper to gate software activity polling.
> 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index bf9a5ec245a43..6859783422f5d 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -81,6 +81,16 @@ struct led_netdev_data {
> >  
> >  static const struct attribute_group netdev_trig_link_speed_attrs_group;
> >  
> > +/* True while the software activity worker should keep polling and blinking. */
> > +static bool netdev_trig_sw_blink(struct led_netdev_data *trigger_data)
> > +{
> > +	return trigger_data->net_dev && !trigger_data->hw_control &&
> > +	       (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
> > +		test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode) ||
> > +		test_bit(TRIGGER_NETDEV_TX_ERR, &trigger_data->mode) ||
> > +		test_bit(TRIGGER_NETDEV_RX_ERR, &trigger_data->mode));
> > +}
> 
> [Severity: Medium]
> Should netdev_trig_sw_blink() also check trigger_data->carrier_link_up?
> 
> In set_baseline_state(), trigger_data->work is only scheduled when
> trigger_data->carrier_link_up is true, and it is not cancelled when
> carrier_link_up is false:
> 
> drivers/leds/trigger/ledtrig-netdev.c:set_baseline_state() {
> 	if (!trigger_data->carrier_link_up) {
> 		led_set_brightness(led_cdev, LED_OFF);
> 	} else {
> 		...
> 		if (netdev_trig_sw_blink(trigger_data))
> 			schedule_delayed_work(&trigger_data->work, 0);
> 	}
> }
> 
> Both netdev_trig_notify() and set_device_name() call
> cancel_delayed_work_sync(&trigger_data->work) before acquiring
> trigger_data->lock:
> 
> drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_notify() {
> 	cancel_delayed_work_sync(&trigger_data->work);
> 
> 	mutex_lock(&trigger_data->lock);
> 
> 	trigger_data->carrier_link_up = false;
> 	...
> 	set_baseline_state(trigger_data);
> 
> 	mutex_unlock(&trigger_data->lock);
> }
> 
> If a concurrent netdev_led_attr_store() or interval_store() acquires
> trigger_data->lock in the window between cancel_delayed_work_sync() and
> mutex_lock(&trigger_data->lock) while carrier_link_up is still true,
> set_baseline_state() will re-arm trigger_data->work.
> 
> When netdev_trig_notify() or set_device_name() then acquires
> trigger_data->lock, it sets carrier_link_up = false and sets the LED to
> LED_OFF without cancelling the newly scheduled work. When netdev_trig_work()
> runs, netdev_trig_sw_blink() returns true despite carrier_link_up being
> false, allowing the worker to keep polling, blinking via
> led_blink_set_oneshot(), and rescheduling itself while the link is down
> (and leaving the LED on when any link or duplex mode bit sets invert = 1).
> 
> [ ... ]
> 
> > @@ -503,10 +520,14 @@ static ssize_t interval_store(struct device *dev,
> >  
> >  	/* impose some basic bounds on the timer interval */
> >  	if (value >= 5 && value <= 10000) {
> > -		cancel_delayed_work_sync(&trigger_data->work);
> > +		mutex_lock(&trigger_data->lock);
> > +
> > +		cancel_delayed_work(&trigger_data->work);
> >  
> >  		atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> >  		set_baseline_state(trigger_data);	/* resets timer */
> > +
> > +		mutex_unlock(&trigger_data->lock);
> >  	}
> >  
> >  	return size;
> 
> [Severity: Medium]
> Can interval_store() race with concurrent trigger_data->hw_control updates?
> 
> At the start of interval_store(), trigger_data->hw_control is checked
> before acquiring trigger_data->lock:
> 
> drivers/leds/trigger/ledtrig-netdev.c:interval_store() {
> 	if (trigger_data->hw_control)
> 		return -EINVAL;
> 	...
> 	if (value >= 5 && value <= 10000) {
> 		mutex_lock(&trigger_data->lock);
> }
> 
> If trigger_data->hw_control is false during that unlocked check, a
> concurrent netdev_led_attr_store() or netdev_trig_notify() (on NETDEV_UP)
> can acquire trigger_data->lock first, see the default interval in
> can_hw_control(), and set trigger_data->hw_control = true.
> 
> When interval_store() then acquires trigger_data->lock, it does not
> re-check trigger_data->hw_control before updating trigger_data->interval
> and calling set_baseline_state(). Doesn't this allow trigger_data->interval
> to be changed to a non-default value while hw_control is active, causing
> subsequent can_hw_control() checks to unexpectedly drop out of hardware
> offload?
> 
> [ ... ]
> 
> > 
> > base-commit: 3d229ce89862b8e82d71a9e11d6f840ca272a6d6

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com