[PATCH v5 0/4] uacce: driver fixes for memory leaks and state management

[PATCH v5 0/4] uacce: driver fixes for memory leaks and state management

[Chenghai Huang]

This patch series addresses several issues in the uacce:
1.Fix cdev handling in register and remove paths.
2.Fix sysfs file creation conditions.
3.Add error reporting for unsupported mremap operations.
4.Ensuring safe queue release with proper state management.

---
Changes in v5:
- There is no memory leak issue when cdev_device_add fails, but it is necessary
  to check a flag to avoid calling cdev_device_del during abnormal exit.
- Link to v4: https://lore.kernel.org/all/20251022021149.1771168-1-huangchenghai2@huawei.com/

Changes in v4:
- Revert the interception of sysfs creation for isolate_strategy.
- Link to v3: https://lore.kernel.org/all/20251021135003.786588-1-huangchenghai2@huawei.com/

Changes in v3:
- Move the checks for the 'isolate_strategy_show' and
  'isolate_strategy_store' functions to their respective call sites.
- Use kobject_put to release the cdev memory instead of modifying
  cdev to be a static structure member.
- Link to v2: https://lore.kernel.org/all/20250916144811.1799687-1-huangchenghai2@huawei.com/

Changes in v2:
- Use cdev_init to allocate cdev memory to ensure that memory leaks
  are avoided.
- Supplement the reason for intercepting the remapping operation.
- Add "cc: stable@vger.kernel.org" to paths with fixed.
- Link to v1: https://lore.kernel.org/all/20250822103904.3776304-1-huangchenghai2@huawei.com/

---
Chenghai Huang (2):
  uacce: fix isolate sysfs check condition
  uacce: ensure safe queue release with state management

Wenkai Lin (1):
  uacce: fix cdev handling in register and remove paths

Yang Shen (1):
  uacce: implement mremap in uacce_vm_ops to return -EPERM

 drivers/misc/uacce/uacce.c | 50 +++++++++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 9 deletions(-)

-- 
2.33.0

[PATCH v5 1/4] uacce: fix cdev handling in register and remove paths

[Chenghai Huang]

From: Wenkai Lin <linwenkai6@hisilicon.com>

This patch addresses a potential issue in the uacce driver where the
cdev was not properly managed during error handling and cleanup paths.

Changes made:
1. In uacce_register(), store the return value of cdev_device_add()
   and clear the cdev owner as a flag if registration fails.
2. In uacce_remove(), add additional check for cdev owner before
   calling cdev_device_del() to prevent potential issues.

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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 42e7d2a2a90c..688050c35d88 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -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,11 @@ 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)
+		uacce->cdev->owner = NULL;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(uacce_register);
 
@@ -568,7 +574,7 @@ void uacce_remove(struct uacce_device *uacce)
 		unmap_mapping_range(q->mapping, 0, 0, 1);
 	}
 
-	if (uacce->cdev)
+	if (uacce->cdev && uacce->cdev->owner)
 		cdev_device_del(uacce->cdev, &uacce->dev);
 	xa_erase(&uacce_xa, uacce->dev_id);
 	/*
-- 
2.33.0

Re: [PATCH v5 1/4] uacce: fix cdev handling in register and remove paths

[Zhangfei Gao]

On Tue, 11 Nov 2025 at 17:35, Chenghai Huang <huangchenghai2@huawei.com> wrote:
>
> From: Wenkai Lin <linwenkai6@hisilicon.com>
>
> This patch addresses a potential issue in the uacce driver where the
> cdev was not properly managed during error handling and cleanup paths.
Can we clarify that it was caused by cdev_device_add?

>
> Changes made:
> 1. In uacce_register(), store the return value of cdev_device_add()
>    and clear the cdev owner as a flag if registration fails.
> 2. In uacce_remove(), add additional check for cdev owner before
>    calling cdev_device_del() to prevent potential issues.
>
> 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 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index 42e7d2a2a90c..688050c35d88 100644
> --- a/drivers/misc/uacce/uacce.c
> +++ b/drivers/misc/uacce/uacce.c
> @@ -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,11 @@ 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)
> +               uacce->cdev->owner = NULL;
If uacce is build in, THIS_MODULE = 0.


how about this, handle cdev_device_add fail for no device_add case.

+       if (ret) {
+               if (!device_is_registered(&uacce->dev)) {
+                       cdev_del(uacce->cdev);
+                       uacce->cdev = NULL;
+               }
+       }



> +
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(uacce_register);
>
> @@ -568,7 +574,7 @@ void uacce_remove(struct uacce_device *uacce)
>                 unmap_mapping_range(q->mapping, 0, 0, 1);
>         }
>
> -       if (uacce->cdev)
> +       if (uacce->cdev && uacce->cdev->owner)
>                 cdev_device_del(uacce->cdev, &uacce->dev);
is there an potential issue, uacce->cdev is not freed?

Thanks

Re: [PATCH v5 1/4] uacce: fix cdev handling in register and remove paths

[Zhangfei Gao]

.

On Wed, 12 Nov 2025 at 17:46, Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
> On Tue, 11 Nov 2025 at 17:35, Chenghai Huang <huangchenghai2@huawei.com> wrote:
> >
> > From: Wenkai Lin <linwenkai6@hisilicon.com>
> >
> > This patch addresses a potential issue in the uacce driver where the
> > cdev was not properly managed during error handling and cleanup paths.
> Can we clarify that it was caused by cdev_device_add?
>
> >
> > Changes made:
> > 1. In uacce_register(), store the return value of cdev_device_add()
> >    and clear the cdev owner as a flag if registration fails.
> > 2. In uacce_remove(), add additional check for cdev owner before
> >    calling cdev_device_del() to prevent potential issues.
> >
> > 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 | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> > index 42e7d2a2a90c..688050c35d88 100644
> > --- a/drivers/misc/uacce/uacce.c
> > +++ b/drivers/misc/uacce/uacce.c
> > @@ -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,11 @@ 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)
> > +               uacce->cdev->owner = NULL;
> If uacce is build in, THIS_MODULE = 0.
>
>
> how about this, handle cdev_device_add fail for no device_add case.
>
> +       if (ret) {
> +               if (!device_is_registered(&uacce->dev)) {
> +                       cdev_del(uacce->cdev);
> +                       uacce->cdev = NULL;
> +               }
> +       }
>
>
>
> > +
> > +       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(uacce_register);
> >
> > @@ -568,7 +574,7 @@ void uacce_remove(struct uacce_device *uacce)
> >                 unmap_mapping_range(q->mapping, 0, 0, 1);
> >         }
> >
> > -       if (uacce->cdev)
> > +       if (uacce->cdev && uacce->cdev->owner)
> >                 cdev_device_del(uacce->cdev, &uacce->dev);
> is there an potential issue, uacce->cdev is not freed?

And cdev_device_add(uacce->cdev, &uacce->dev) will not add refcount to
&uacce->dev
as usual when failing beforehand, so uacce_remove:
put_device(&uacce->dev) will have
refcount_t: underflow; use-after-free, need check as well.

@@ -558,8 +558,12 @@ int uacce_register(struct uacce_device *uacce)
        uacce->cdev->owner = THIS_MODULE;

        ret = cdev_device_add(uacce->cdev, &uacce->dev);
-       if (ret)
-               uacce->cdev->owner = NULL;
+       if (ret) {
+               if (!device_is_registered(&uacce->dev)) {
+                       kobject_put(&uacce->cdev->kobj);
+                       uacce->cdev = NULL;
+               }
+       }
        return ret;
 }

@@ -610,7 +614,8 @@ void uacce_remove(struct uacce_device *uacce)
        uacce->ops = NULL;
        uacce->parent = NULL;
        mutex_unlock(&uacce->mutex);
-       put_device(&uacce->dev);
+       if (device_is_registered(&uacce->dev))
+               put_device(&uacce->dev);
 }

Just one possible solution, FYI.

Thanks

[PATCH v5 2/4] uacce: fix isolate sysfs check condition

[Chenghai Huang]

uacce supports the device isolation feature. If the driver
implements the isolate_err_threshold_read and
isolate_err_threshold_write callback functions, uacce will create
sysfs files now. Users can read and configure the isolation policy
through sysfs. Currently, sysfs files are created as long as either
isolate_err_threshold_read or isolate_err_threshold_write callback
functions are present.

However, accessing a non-existent callback function may cause the
system to crash. Therefore, intercept the creation of sysfs if
neither read nor write exists; create sysfs if either is supported,
but intercept unsupported operations at the call site.

Fixes: e3e289fbc0b5 ("uacce: supports device isolation feature")
Cc: stable@vger.kernel.org
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/misc/uacce/uacce.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 688050c35d88..8a0cb38d9a3d 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -382,6 +382,9 @@ static ssize_t isolate_strategy_show(struct device *dev, struct device_attribute
 	struct uacce_device *uacce = to_uacce_device(dev);
 	u32 val;
 
+	if (!uacce->ops->isolate_err_threshold_read)
+		return -ENOENT;
+
 	val = uacce->ops->isolate_err_threshold_read(uacce);
 
 	return sysfs_emit(buf, "%u\n", val);
@@ -394,6 +397,9 @@ static ssize_t isolate_strategy_store(struct device *dev, struct device_attribut
 	unsigned long val;
 	int ret;
 
+	if (!uacce->ops->isolate_err_threshold_write)
+		return -ENOENT;
+
 	if (kstrtoul(buf, 0, &val) < 0)
 		return -EINVAL;
 
-- 
2.33.0

[PATCH v5 3/4] uacce: implement mremap in uacce_vm_ops to return -EPERM

[Chenghai Huang]

From: Yang Shen <shenyang39@huawei.com>

The current uacce_vm_ops does not support the mremap operation of
vm_operations_struct. Implement .mremap to return -EPERM to remind
users.

The reason we need to explicitly disable mremap is that when the
driver does not implement .mremap, it uses the default mremap
method. This could lead to a risk scenario:

An application might first mmap address p1, then mremap to p2,
followed by munmap(p1), and finally munmap(p2). Since the default
mremap copies the original vma's vm_private_data (i.e., q) to the
new vma, both munmap operations would trigger vma_close, causing
q->qfr to be freed twice(qfr will be set to null here, so repeated
release is ok).

Fixes: 015d239ac014 ("uacce: add uacce driver")
Cc: stable@vger.kernel.org
Signed-off-by: Yang Shen <shenyang39@huawei.com>
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/misc/uacce/uacce.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 8a0cb38d9a3d..f2296fc9fa4e 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -214,8 +214,14 @@ static void uacce_vma_close(struct vm_area_struct *vma)
 	}
 }
 
+static int uacce_vma_mremap(struct vm_area_struct *area)
+{
+	return -EPERM;
+}
+
 static const struct vm_operations_struct uacce_vm_ops = {
 	.close = uacce_vma_close,
+	.mremap = uacce_vma_mremap,
 };
 
 static int uacce_fops_mmap(struct file *filep, struct vm_area_struct *vma)
-- 
2.33.0

[PATCH v5 4/4] uacce: ensure safe queue release with state management

[Chenghai Huang]

Directly calling `put_queue` carries risks since it cannot
guarantee that resources of `uacce_queue` have been fully released
beforehand. So adding a `stop_queue` operation for the
UACCE_CMD_PUT_Q command and leaving the `put_queue` operation to
the final resource release ensures safety.

Queue states are defined as follows:
- UACCE_Q_ZOMBIE: Initial state
- UACCE_Q_INIT: After opening `uacce`
- UACCE_Q_STARTED: After `start` is issued via `ioctl`

When executing `poweroff -f` in virt while accelerator are still
working, `uacce_fops_release` and `uacce_remove` may execute
concurrently. This can cause `uacce_put_queue` within
`uacce_fops_release` to access a NULL `ops` pointer. Therefore, add
state checks to prevent accessing freed pointers.

Fixes: 015d239ac014 ("uacce: add uacce driver")
Cc: stable@vger.kernel.org
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
Signed-off-by: Yang Shen <shenyang39@huawei.com>
Acked-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/misc/uacce/uacce.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index f2296fc9fa4e..e581b977576d 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -40,20 +40,34 @@ static int uacce_start_queue(struct uacce_queue *q)
 	return 0;
 }
 
-static int uacce_put_queue(struct uacce_queue *q)
+static int uacce_stop_queue(struct uacce_queue *q)
 {
 	struct uacce_device *uacce = q->uacce;
 
-	if ((q->state == UACCE_Q_STARTED) && uacce->ops->stop_queue)
+	if (q->state != UACCE_Q_STARTED)
+		return 0;
+
+	if (uacce->ops->stop_queue)
 		uacce->ops->stop_queue(q);
 
-	if ((q->state == UACCE_Q_INIT || q->state == UACCE_Q_STARTED) &&
-	     uacce->ops->put_queue)
+	q->state = UACCE_Q_INIT;
+
+	return 0;
+}
+
+static void uacce_put_queue(struct uacce_queue *q)
+{
+	struct uacce_device *uacce = q->uacce;
+
+	uacce_stop_queue(q);
+
+	if (q->state != UACCE_Q_INIT)
+		return;
+
+	if (uacce->ops->put_queue)
 		uacce->ops->put_queue(q);
 
 	q->state = UACCE_Q_ZOMBIE;
-
-	return 0;
 }
 
 static long uacce_fops_unl_ioctl(struct file *filep,
@@ -80,7 +94,7 @@ static long uacce_fops_unl_ioctl(struct file *filep,
 		ret = uacce_start_queue(q);
 		break;
 	case UACCE_CMD_PUT_Q:
-		ret = uacce_put_queue(q);
+		ret = uacce_stop_queue(q);
 		break;
 	default:
 		if (uacce->ops->ioctl)
-- 
2.33.0