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

26 Sep 2025

在 2025/9/26 16:47, huangchenghai 写道:
...
On Wed, Sep 17, 2025 at 06:18 PM +0800, Greg KH wrote:
...
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
Very sorry for the delayed response.
In v1, our first thought was that if cdev_device_add returns a
failure, we could release the resources allocated by cdev_alloc
using cdev_del. For this, we attempted the following modification:
@@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(uacce_alloc);
  */
 int uacce_register(struct uacce_device *uacce)
 {
+    int ret;
+
     if (!uacce)
         return -ENODEV;
@@ -529,7 +531,14 @@ int uacce_register(struct uacce_device *uacce)
     uacce->cdev->ops = &uacce_fops;
     uacce->cdev->owner = THIS_MODULE;
-    return cdev_device_add(uacce->cdev, &uacce->dev);
+    ret = cdev_device_add(uacce->cdev, &uacce->dev);
+    if (ret) {
+        cdev_del(uacce->cdev);
+        uacce->cdev = NULL;
+        return ret;
+    }
+
+    return 0;
 }
However, after further analysis, we found that cdev_device_add does
not increment the reference count when it fails. Therefore, in this
case, cdev_del is not necessary. This means that the resources
allocated by cdev_alloc will not cause a memory leak in the failure
path.
Thus, I believe this patch modification is unnecessary. In the
upcoming v3 version, I will remove this modification.
Thank you for your patient guidance!
Best regards,
Chenghai
...
After further analysis, it was found that during the cdev_alloc
process, the reference count of the cdev is initialized to 1.

If kobject_put is not actively executed, the release function of
the cdev cannot be called, and the memory will never be released,
leading to a leak, so kobject_put is still necessary here?

Here is the new solution:
@@ -519,6 +519,8 @@ EXPORT_SYMBOL_GPL(uacce_alloc);
   */
  int uacce_register(struct uacce_device *uacce)
  {
+    int ret;
+
      if (!uacce)
          return -ENODEV;

@@ -529,7 +531,14 @@ int uacce_register(struct uacce_device *uacce)
      uacce->cdev->ops = &uacce_fops;
      uacce->cdev->owner = THIS_MODULE;

-    return cdev_device_add(uacce->cdev, &uacce->dev);
+    ret = cdev_device_add(uacce->cdev, &uacce->dev);
+    if (ret) {
+        kobject_put(&uacce->cdev->kobject);
+        uacce->cdev = NULL;
+        return ret;
+    }
+
+    return 0;
  }

Thanks,
WenKai

    

linwenkai (C)

tags

participants (1)