Re: [PATCH v4 1/4] leds: trigger: netdev: Put netdev on activate error path

[Lee Jones]

On Tue, 20 Oct 2026, Sverdlin, Alexander wrote:

> Hi Lee!
> 
> On Thu, 2026-10-15 at 15:52 +0100, Lee Jones wrote:
> > > When hw control is active, set_device_name() takes a reference on the
> > > netdev via dev_get_by_name(). The register_netdevice_notifier() failure
> > > path frees trigger_data without dropping it, leaking the netdev.
> > > 
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Closes: https://lore.kernel.org/all/20260914140045.B8F4C1F000FF@xxxxxxxxxxxxxxx/
> > > Fixes: 0316cc5629d1 ("leds: trigger: netdev: init mode if hw control already active")
> > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> > > ---
> > > Changelog:
> > > v3:
> > > - introduced unset_device_name() as a counterpart of set_device_name()
> > > 
> > >   drivers/leds/trigger/ledtrig-netdev.c | 13 +++++++++++--
> > >   1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > > index 5b0132484594c..e2a6c95a0dc97 100644
> > > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > > @@ -324,6 +324,13 @@ static int set_device_name(struct led_netdev_data *trigger_data,
> > >   	return 0;
> > >   }
> > > 
> > > +static void unset_device_name(struct led_netdev_data *trigger_data)
> > > +{
> > > +	dev_put(trigger_data->net_dev);
> > > +	trigger_data->net_dev = NULL;
> > > +	trigger_data->device_name[0] = 0;
> > > +}
> > 
> > Given that both callers immediately 'kfree(trigger_data)' after calling
> > this, why bother clearing these at all?  Wouldn't calling 'dev_put()'
> > directly in 'netdev_trig_activate()' be simpler than introducing a
> > helper here?
> 
> I agree on all points!
> The v2 was indeed simpler [1].
> unset_device_name() was proposed by Andrew [2], but maybe I went to far
> clearing things which are not required to be cleared actually.
> 
> Shall we agree on v2 of this patch as it was initially published, or shall
> I just drop the redundant zeroing from unset_device_name() and leave it
> just calling single dev_put() for now?
> 
> [1] https://lore.kernel.org/all/20260921093249.3481413-2-alexander.sverdlin@xxxxxxxxxxx/
> [2] https://lore.kernel.org/all/13e58137-2104-4ba4-9d41-e30ba5dfb367@xxxxxxx/

How many call-sites are there, and how many lines would
unset_device_name() actually save?

I think you are free to use your common sense.  Don't free / zero
anything that you don't need to, and if it makes sense to move the
clean-up to unset_device_name() do it, if not, don't.

=:-)

> > >   static ssize_t device_name_store(struct device *dev,
> > >   				 struct device_attribute *attr, const char *buf,
> > >   				 size_t size)
> > > @@ -776,8 +783,10 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
> > >   	led_set_trigger_data(led_cdev, trigger_data);
> > > 
> > >   	rc = register_netdevice_notifier(&trigger_data->notifier);
> > > -	if (rc)
> > > +	if (rc) {
> > > +		unset_device_name(trigger_data);
> > >   		kfree(trigger_data);
> > > +	}
> > > 
> > >   	return rc;
> > >   }
> > > @@ -790,7 +799,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
> > > 
> > >   	cancel_delayed_work_sync(&trigger_data->work);
> > > 
> > > -	dev_put(trigger_data->net_dev);
> > > +	unset_device_name(trigger_data);
> > > 
> > >   	kfree(trigger_data);
> > >   }
> 
> -- 
> Alexander Sverdlin
> Siemens AG
> www.siemens.com

-- 
Lee Jones