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

[huangchenghai <huangchenghai2@huawei.com>]

On Mon, Sep 16, 2025 at 11:15 PM +0800, Greg KH wrote:
> 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
We analyze it theoretically there may be a memory leak
issue here, if the cdev_device_add returns a failure,
the uacce_remove will not be executed, which results in the
uacce cdev memory not being released.
Therefore, we have decided to align with the design of other
drivers by making cdev a static member of uacce_device and
releasing the memory through uacce_device.

found one example in drivers/watchdog/watchdog_dev.h.
struct watchdog_core_data {
     struct device dev;
     struct cdev cdev;
     struct watchdog_device *wdd;
     struct mutex lock;
     ktime_t last_keepalive;
     ktime_t last_hw_keepalive;
     ktime_t open_deadline;
     ...
};

static int watchdog_cdev_register(struct watchdog_device *wdd)
{
     struct watchdog_core_data *wd_data;
     int err;
     ...
     cdev_init(&wd_data->cdev, &watchdog_fops);
     wd_data->cdev.owner = wdd->ops->owner;

     /* Add the device */
     err = cdev_device_add(&wd_data->cdev, &wd_data->dev);
     ...
}

static void watchdog_cdev_unregister(struct watchdog_device *wdd)
{
     struct watchdog_core_data *wd_data = wdd->wd_data;
     ...
     cdev_device_del(&wd_data->cdev, &wd_data->dev);
     if (wdd->id == 0) {
             misc_deregister(&watchdog_miscdev);
             old_wd_data = NULL;
     }
     ...
}

Thanks,

ChengHai