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

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

[Guangshuo Li]

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@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 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:
 
-- 
2.43.0

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2704 bytes --]

Hello,

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@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
>  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. 

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.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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@vger.kernel.org
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> > ---
> >  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