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

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

[Chenghai Huang]

This patch series addresses several issues in the uacce:
1.Memory leak fix when device registration fails.
2.Fix sysfs file creation conditions.
3.Add error reporting for unsupported mremap operations.
4.Ensuring safe queue release with proper state management.

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

Wenkai Lin (1):
  uacce: fix for cdev memory leak

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

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

-- 
2.33.0

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

[Chenghai Huang]

From: Wenkai Lin <linwenkai6@hisilicon.com>

If adding uacce cdev to the system fails, it could be due to two
reasons: either the device's devt exists when the failure occurs,
or the device_add operation fails. In the latter case, cdev_del
will be executed, but in the former case, it will not, leading to a
resource leak. Therefore, it is necessary to perform the cdev_del
action during abnormal exit.

Fixes: 015d239ac014 ("uacce: add uacce driver")
Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
---
 drivers/misc/uacce/uacce.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 42e7d2a2a90c..3604f722ed60 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,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;
 }
 EXPORT_SYMBOL_GPL(uacce_register);
 
-- 
2.33.0

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

[Chenghai Huang]

The uacce supports device isolation feature. If the driver
implements the isolate_err_threshold_read and
isolate_err_threshold_write callbacks, the uacce will create sysfs
files. Users can read and configure isolation policies through
sysfs. Currently, if either isolate_err_threshold_read or
isolate_err_threshold_write callback exists, sysfs files are
created.

However, accessing a non-existent callback may cause a system panic.
Therefore, sysfs files are only created when both
isolate_err_threshold_read and isolate_err_threshold_write are
present.

Fixes: e3e289fbc0b5 ("uacce: supports device isolation feature")
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
---
 drivers/misc/uacce/uacce.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 3604f722ed60..6a38809ca819 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -441,7 +441,7 @@ static umode_t uacce_dev_is_visible(struct kobject *kobj,
 		return 0;
 
 	if (attr == &dev_attr_isolate_strategy.attr &&
-	    (!uacce->ops->isolate_err_threshold_read &&
+	    (!uacce->ops->isolate_err_threshold_read ||
 	     !uacce->ops->isolate_err_threshold_write))
 		return 0;
 
-- 
2.33.0

[PATCH 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

Fixes: 015d239ac014 ("uacce: add uacce driver")
Signed-off-by: Yang Shen <shenyang39@huawei.com>
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
---
 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 6a38809ca819..531a24145ba4 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 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")
Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
Signed-off-by: Yang Shen <shenyang39@huawei.com>
---
 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 531a24145ba4..8a78edb545a1 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

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

[Greg KH]

On Fri, Aug 22, 2025 at 06:39:01PM +0800, Chenghai Huang wrote:
> From: Wenkai Lin <linwenkai6@hisilicon.com>
> 
> If adding uacce cdev to the system fails, it could be due to two
> reasons: either the device's devt exists when the failure occurs,
> or the device_add operation fails. In the latter case, cdev_del
> will be executed, but in the former case, it will not, leading to a
> resource leak. Therefore, it is necessary to perform the cdev_del
> action during abnormal exit.
> 
> Fixes: 015d239ac014 ("uacce: add uacce driver")
> Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
> Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
> ---
>  drivers/misc/uacce/uacce.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index 42e7d2a2a90c..3604f722ed60 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,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;
>  }
>  EXPORT_SYMBOL_GPL(uacce_register);
>  
> -- 
> 2.33.0
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Greg KH]

On Fri, Aug 22, 2025 at 06:39:03PM +0800, Chenghai Huang wrote:
> 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

Why is this needed?  If mremap is not set, what is the value returned?

And why is -EPERM the correct value to return here?  That's not what the
man pages say is valid :(

thanks,

greg k-h

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

[Zhangfei Gao]

On Fri, 22 Aug 2025 at 18:39, Chenghai Huang <huangchenghai2@huawei.com> wrote:
>
> From: Wenkai Lin <linwenkai6@hisilicon.com>
>
> If adding uacce cdev to the system fails, it could be due to two
> reasons: either the device's devt exists when the failure occurs,
> or the device_add operation fails. In the latter case, cdev_del
> will be executed, but in the former case, it will not, leading to a
> resource leak. Therefore, it is necessary to perform the cdev_del
> action during abnormal exit.
>
> Fixes: 015d239ac014 ("uacce: add uacce driver")
> Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
> Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
> ---
>  drivers/misc/uacce/uacce.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
> index 42e7d2a2a90c..3604f722ed60 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,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);

Can cdev_del be called multi-times? since cdev_device_add may call
cdev_device_add itself.
Does this introduce another issue?

Not understand the case: "either the device's devt exists when the
failure occurs,"
Looks it is cdev_device_add itself issue, or need add check
if (uacce->dev.devt)
 cdev_del(uacce->cdev);
how about other drivers' behavior.

Thanks

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

[Zhangfei Gao]

Hi, Greg

On Fri, 22 Aug 2025 at 19:46, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Aug 22, 2025 at 06:39:03PM +0800, Chenghai Huang wrote:
> > 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
>
> Why is this needed?  If mremap is not set, what is the value returned?

Did some debug locally.

By default, mremap is permitted.

With mremap, the original vma is released,
The vma_close is called and free resources, including q->qfr.

However, vma->vm_private_data (q) is copied to the new vma.
When the new vma is closed, vma_close will get q and q->qft=0.

So disable mremap here looks safer.

>
> And why is -EPERM the correct value to return here?  That's not what the
> man pages say is valid :(

if disable mremap, -1 is returned as MAP_FAILED.
The errno is decided by the return value, -EPERM (-1) or -EINVAL (-22).
man mremap only lists -EINVAL.

However, here the driver wants to disable mremap, looks -EPERM is more suitable.

What's your suggestion?

Thanks

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

[linwenkai (C)]

在 2025/8/25 16:20, Zhangfei Gao 写道:
> On Fri, 22 Aug 2025 at 18:39, Chenghai Huang <huangchenghai2@huawei.com> wrote:
>> From: Wenkai Lin <linwenkai6@hisilicon.com>
>>
>> If adding uacce cdev to the system fails, it could be due to two
>> reasons: either the device's devt exists when the failure occurs,
>> or the device_add operation fails. In the latter case, cdev_del
>> will be executed, but in the former case, it will not, leading to a
>> resource leak. Therefore, it is necessary to perform the cdev_del
>> action during abnormal exit.
>>
>> Fixes: 015d239ac014 ("uacce: add uacce driver")
>> Signed-off-by: Wenkai Lin <linwenkai6@hisilicon.com>
>> Signed-off-by: Chenghai Huang <huangchenghai2@huawei.com>
>> ---
>>   drivers/misc/uacce/uacce.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
>> index 42e7d2a2a90c..3604f722ed60 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,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);
> Can cdev_del be called multi-times? since cdev_device_add may call
> cdev_device_add itself.
> Does this introduce another issue?
>
> Not understand the case: "either the device's devt exists when the
> failure occurs,"
> Looks it is cdev_device_add itself issue, or need add check
> if (uacce->dev.devt)
>   cdev_del(uacce->cdev);
> how about other drivers' behavior.

When devt is non-zero, there is still no way to distinguish whether cdev_del

has been executed internally, which might be a  restriction of 
cdev_device_add.

Other drivers often use cdev_init, which does not require consideration

of memory leak, perhaps uacce should use this method too, the plan is to

modify it in v2.

> Thanks