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

[sashiko-bot@kernel.org]

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()
--

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020124651.1987272-1-alexander.sverdlin@siemens.com?part=4