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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025091620-theft-glue-5e7f@gregkh>
Date: Tue, 16 Sep 2025 17:14:20 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Chenghai Huang <huangchenghai2@...wei.com>
Cc: zhangfei.gao@...aro.org, wangzhou1@...ilicon.com,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linuxarm@...neuler.org, fanghao11@...wei.com, shenyang39@...wei.com,
	liulongfang@...wei.com, qianweili@...wei.com
Subject: Re: [PATCH v2 1/4] uacce: fix for cdev memory leak

On Tue, Sep 16, 2025 at 10:48:08PM +0800, Chenghai Huang wrote:
> From: Wenkai Lin <linwenkai6@...ilicon.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@...r.kernel.org
> Signed-off-by: Wenkai Lin <linwenkai6@...ilicon.com>
> Signed-off-by: Chenghai Huang <huangchenghai2@...wei.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