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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- 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 --]