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

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

Hi all,

On Tue, 2026-09-29 at 12:19 +0000, sashiko-bot@kernel.org wrote:
> 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 is right here, this requires re-work.

pw-bot: cr

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com