Re: [PATCH] counter: Fix refcount leak in counter_alloc() error path

[William Breathitt Gray]

On Mon, Apr 13, 2026 at 09:47:04AM +0200, Uwe Kleine-König wrote:
> On Sat, Apr 11, 2026 at 09:35:11PM +0800, Guangshuo Li wrote:
> > After device_initialize(), the lifetime of the embedded struct device
> > is expected to be managed through the device core reference counting.
> > 
> > In counter_alloc(), if dev_set_name() fails after device_initialize(),
> > the error path removes the chrdev, frees the ID, and frees the backing
> > allocation directly instead of releasing the device reference with
> > put_device(). This bypasses the normal device lifetime rules and may
> > leave the reference count of the embedded struct device unbalanced,
> > resulting in a refcount leak and potentially leading to a use-after-free.
> > 
> > Fix this by using put_device() in the dev_set_name() failure path and
> > let counter_device_release() handle the final cleanup.
> > 
> > Fixes: 4da08477ea1f ("counter: Set counter device name")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Guangshuo Li <lgs201920130244@xxxxxxxxx>
> > ---
> >  drivers/counter/counter-core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> > index 50bd30ba3d03..12dc18c78672 100644
> > --- a/drivers/counter/counter-core.c
> > +++ b/drivers/counter/counter-core.c
> > @@ -123,10 +123,10 @@ struct counter_device *counter_alloc(size_t sizeof_priv)
> >  	return counter;
> >  
> >  err_dev_set_name:
> > +	put_device(dev);
> > +	return NULL;
> >  
> > -	counter_chrdev_remove(counter);
> >  err_chrdev_add:
> > -
> >  	ida_free(&counter_ida, dev->id);
> >  err_ida_alloc:
> 
> This patch is technically correct. Looking in more detail however I
> wonder why 4da08477ea1f ("counter: Set counter device name") was created
> in the presence of
> 
> 	static const struct bus_type counter_bus_type = {
> 		...
> 		.dev_name = "counter",
> 	};
> 
> 	int device_add(struct device *dev)
> 	{
> 		...
> 		if (dev->bus && dev->bus->dev_name)
> 			error = dev_set_name(dev, "%s%u", dev->bus->dev_name, dev->id);
> 		...
> 	}
> 
> The only upside I can see is that the name is already set before
> device_add() is called. 

Looking at the current code, the only Counter subsystem specific call
occurring after device_initialize() but before device_add() is
counter_sysfs_add() which does perform a number devm_kcalloc() and other
such devres operations. I suspect the reason we added the dev_set_name()
call is to make those operations in counter_sysfs_add() clearer in the
devres_log events.

> Assuming the dev_set_name() call should be kept, I think that
> 
> diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
> index 50bd30ba3d03..69f042ce4418 100644
> --- a/drivers/counter/counter-core.c
> +++ b/drivers/counter/counter-core.c
> @@ -114,12 +114,12 @@ struct counter_device *counter_alloc(size_t sizeof_priv)
>  	if (err < 0)
>  		goto err_chrdev_add;
>  
> -	device_initialize(dev);
> -
>  	err = dev_set_name(dev, COUNTER_NAME "%d", dev->id);
>  	if (err)
>  		goto err_dev_set_name;
>  
> +	device_initialize(dev);
> +
>  	return counter;
>  
>  err_dev_set_name:
> 
> also fixes the issue.

We moved dev_set_name() after device_initialize() so that the device
core takes care of freeing the memory allocated for the name.[^1] I
think that's the behavior expected for dev_set_name(), so it's probably
best to keep it after device_initialize() so that there is no need to
manually free memory on failure.

William Breathitt Gray

[^1] https://lkml.iu.edu/hypermail/linux/kernel/2202.0/03859.html