[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2025091746-starship-nearest-7c10@gregkh>
Date: Wed, 17 Sep 2025 12:18:56 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: huangchenghai <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 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@...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
> 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
Powered by blists - more mailing lists