Re: [PATCH v2 1/4] uacce: fix for cdev memory leak

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Sep 16, 2025 at 10:48:08PM +0800, Chenghai Huang wrote:
> From: Wenkai Lin <linwenkai6@hisilicon.com>
> 
> If cdev_device_add failed, it is hard to determine
> whether cdev_del has been executed, which lead to a
> memory leak issue, so we use cdev_init to avoid it.

I do not understand, what is wrong with the current code?  It checks if
add fails:

> 
> Fixes: 015d239ac014 ("uacce: add uacce driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
> Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
> ---
>  drivers/misc/uacce/uacce.c | 13 ++++---------
>  include/linux/uacce.h      |  2 +-
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index 42e7d2a2a90c..12370469f646 100644
> --- a/drivers/misc/uacce/uacce.c
> +++ b/drivers/misc/uacce/uacce.c
> @@ -522,14 +522,10 @@ int uacce_register(struct uacce_device *uacce)
>  	if (!uacce)
>  		return -ENODEV;
>  
> -	uacce->cdev = cdev_alloc();
> -	if (!uacce->cdev)
> -		return -ENOMEM;

This is the check.


> -
> -	uacce->cdev->ops = &uacce_fops;
> -	uacce->cdev->owner = THIS_MODULE;
> +	cdev_init(&uacce->cdev, &uacce_fops);
> +	uacce->cdev.owner = THIS_MODULE;
>  
> -	return cdev_device_add(uacce->cdev, &uacce->dev);
> +	return cdev_device_add(&uacce->cdev, &uacce->dev);

And so is this.  So what is wrong here?


>  }
>  EXPORT_SYMBOL_GPL(uacce_register);
>  
> @@ -568,8 +564,7 @@ void uacce_remove(struct uacce_device *uacce)
>  		unmap_mapping_range(q->mapping, 0, 0, 1);
>  	}
>  
> -	if (uacce->cdev)
> -		cdev_device_del(uacce->cdev, &uacce->dev);
> +	cdev_device_del(&uacce->cdev, &uacce->dev);
>  	xa_erase(&uacce_xa, uacce->dev_id);
>  	/*
>  	 * uacce exists as long as there are open fds, but ops will be freed
> diff --git a/include/linux/uacce.h b/include/linux/uacce.h
> index e290c0269944..98b896192a44 100644
> --- a/include/linux/uacce.h
> +++ b/include/linux/uacce.h
> @@ -126,7 +126,7 @@ struct uacce_device {
>  	bool is_vf;
>  	u32 flags;
>  	u32 dev_id;
> -	struct cdev *cdev;
> +	struct cdev cdev;
>  	struct device dev;

You can not do this, you now have 2 different reference counts
controlling the lifespan of this one structure.  That is just going to
cause so many more bugs...

How was this tested?  What is currently failing that requires this
change?

thanks,

greg k-h