Re: [PATCH v5 06/12] leds: trigger: netdev: Implement offloaded() callback

Hi all,

On Fri, 2026-08-21 at 17:27 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Unprotected lockless read of shared state in `netdev_trig_offloaded()`

This just mimics how the existing "offloaded" attribute does. See
offloaded_show().

This is just an informational hint. It doesn't need to be accurate, so
it's not a bug.

> 
> Pre-existing issues:
> - [High] Resource leak of `net_device` in `netdev_trig_activate()` error path

Fixing it is far beyond the scope of the series.

Thanks,
Rong

> --
> 
> --- Patch [6]: [PATCH v5 06/12] leds: trigger: netdev: Implement offloaded() callback ---
> commit d8144f7505ed647e0e3c94f8c01b2ef2b0814005
> Author: Rong Zhang <[email protected]>
> 
> leds: trigger: netdev: Implement offloaded() callback
> 
> This commit implements the offloaded callback for the netdev trigger,
> providing its hardware control state to the LED core. It also updates the
> sysfs ABI documentation to clarify the relationship between the custom and
> generic offload attributes.
> 
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index 64c078e997f28..a26109ca4b1c9 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> 
> [ ... ]
> 
> > @@ -754,10 +754,18 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
> >  	kfree(trigger_data);
> >  }
> >  
> > +static bool netdev_trig_offloaded(struct led_classdev *led_cdev)
> > +{
> > +	struct led_netdev_data *trigger_data = led_get_trigger_data(led_cdev);
> > +
> > +	return trigger_data->hw_control;
> 
> [Severity: Medium]
> Is it possible for hw_control to be read while it is being updated?
> 
> When userspace reads the trigger_may_offload sysfs attribute, it calls
> netdev_trig_offloaded() holding only the led_cdev->trigger_lock, which is a
> read lock.
> 
> Concurrently, a network event can invoke netdev_trig_notify() which
> acquires trigger_data->lock and mutates trigger_data->hw_control.
> 
> Does this code need to hold trigger_data->lock or use the READ_ONCE() macro
> to prevent a data race when reading trigger_data->hw_control here?
> 
> > +}
> > +
> >  static struct led_trigger netdev_led_trigger = {
> >  	.name = "netdev",
> >  	.activate = netdev_trig_activate,
> >  	.deactivate = netdev_trig_deactivate,
> > +	.offloaded = netdev_trig_offloaded,
> >  	.groups = netdev_trig_groups,
> >  };
> 
> [Severity: High]
> This is a pre-existing issue, but does netdev_trig_activate() leak a
> net_device reference in its error path?
> 
> If hardware control is supported, set_device_name() is called, which
> acquires a net device reference via dev_get_by_name() and saves it to
> trigger_data->net_dev.
> 
> If the subsequent call to register_netdevice_notifier() fails, the error
> path executes:
> 
> drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
>     ...
> 	rc = register_netdevice_notifier(&trigger_data->notifier);
> 	if (rc)
> 		kfree(trigger_data);
> 
> 	return rc;
> }
> 
> Since trigger_data is freed without calling dev_put(trigger_data->net_dev),
> does this leave a permanent reference count leak on the net_device?