Re: [PATCH v3 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 1 potential issue(s) to consider:
- [Medium] leds: netdev: async work cancel allows zombie worker to overwrite hw offload
--

commit d51dca8c4abbb112c1f8e31383d3f2334d50c585
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

leds: trigger: netdev: Serialize mode/interval stores with trigger lock

This commit adds explicit mutex locking to serialize mode and interval stores.
It replaces the synchronous delayed work cancellation with an asynchronous
cancellation while holding the trigger lock.

> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index b27a453bbb859..7ad61c4be282e 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
[ ... ]
> @@ -424,6 +424,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  		return -EINVAL;
>  	}
>  
> +	/* async cancel: the worker takes this lock */
> +	mutex_lock(&trigger_data->lock);
> +
> +	mode = trigger_data->mode;
>  	assign_bit(bit, &mode, state);
>  
>  	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
> @@ -436,20 +440,26 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
>  	     test_bit(TRIGGER_NETDEV_LINK_25000, &mode) ||
>  	     test_bit(TRIGGER_NETDEV_LINK_40000, &mode) ||
>  	     test_bit(TRIGGER_NETDEV_LINK_50000, &mode) ||
> -	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode)))
> +	     test_bit(TRIGGER_NETDEV_LINK_100000, &mode))) {
> +		mutex_unlock(&trigger_data->lock);
>  		return -EINVAL;
> +	}
>  
> -	cancel_delayed_work_sync(&trigger_data->work);
> +	cancel_delayed_work(&trigger_data->work);

[Severity: Medium]
If the worker netdev_trig_work() is concurrently executing and blocked on
trigger_data->lock, will this asynchronous cancel fail to stop it?

Once netdev_led_attr_store() finishes configuring hardware offload and
releases the lock, could the worker unblock and continue to execute?

If it does, the worker doesn't appear to check trigger_data->hw_control
before falling back to software blinking and rescheduling itself:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_work() {
	...
	/* If we are not looking for RX/TX then return  */
	if (!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)) {
		mutex_unlock(&trigger_data->lock);
		return;
	}
	...
	/* no check for hw_control */
	...
		led_blink_set_oneshot(trigger_data->led_cdev, ...);
		trigger_data->last_activity = new_activity;
	}

	schedule_delayed_work(&trigger_data->work, ...);

	mutex_unlock(&trigger_data->lock);
}

Will this cause the zombie worker to permanently overwrite the hardware
offload state configured here and waste CPU by continuously rescheduling
itself?

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