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

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Sep 17, 2025 at 05:56:16PM +0800, huangchenghai wrote:
> 
> 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.

Then properly clean up if that happens.

> 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.

But again, this is wrong to do.

> found one example in drivers/watchdog/watchdog_dev.h.
> struct watchdog_core_data {
>     struct device dev;
>     struct cdev cdev;

This is also wrong and needs to be fixed.  Please send a patch to
resolve it as well, as it should not be copied as a valid example.

thanks,

greg k-h