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

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

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@vger.kernel.org
> > Closes: https://lore.kernel.org/all/20260914140045.B8F4C1F000FF@smtp.kernel.org/
> > Fixes: 0316cc5629d1 ("leds: trigger: netdev: init mode if hw control already active")
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> > 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@siemens.com/
[2] https://lore.kernel.org/all/13e58137-2104-4ba4-9d41-e30ba5dfb367@lunn.ch/

> > +
> >   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