[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

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

[Greg KH]

On Thu, Aug 28, 2025 at 01:59:48PM +0800, Zhangfei Gao wrote:
> 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.

Disabling mremap is not a permission issue, it's more of an invalid
call?  I don't know, what do other drivers do?

thanks,

greg k-h

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

[Zhangfei Gao]

Hi Greg

On Sat, 6 Sept 2025 at 20:03, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Aug 28, 2025 at 01:59:48PM +0800, Zhangfei Gao wrote:
> > 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.
>
> Disabling mremap is not a permission issue, it's more of an invalid

OK，it's fine.

> call?  I don't know, what do other drivers do?

Found one example in kernel/events/uprobes.c

commit c16e2fdd746c78f5b2ce3c2ab8a26a61b6ed09e5
Author: Oleg Nesterov <oleg@redhat.com>
Date:   Sun Sep 29 16:42:58 2024 +0200

    uprobes: deny mremap(xol_vma)

+static int xol_mremap(const struct vm_special_mapping *sm, struct
vm_area_struct *new_vma)
+{
+       return -EPERM;
+}


Thanks

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

[huangchenghai]

On Sat, 6 Sept 2025 at 20:03, Greg KH wrote:
> On Thu, Aug 28, 2025 at 01:59:48PM +0800, Zhangfei Gao wrote:
>> 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.
> Disabling mremap is not a permission issue, it's more of an invalid
> call?  I don't know, what do other drivers do?
>
> thanks,
>
> greg k-h
Hi Greg,

Thank you for your feedback.

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

Regards,
ChengHai

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

[huangchenghai]

On Thu, Aug 22, 2025 at 07:27 PM +0800, Greg KH wrote:
> 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
Okay，I will add it in v2.

Thanks,
ChengHai

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

[Greg KH]

On Sat, Sep 13, 2025 at 06:40:23PM +0800, huangchenghai wrote:
> 
> On Sat, 6 Sept 2025 at 20:03, Greg KH wrote:
> > On Thu, Aug 28, 2025 at 01:59:48PM +0800, Zhangfei Gao wrote:
> > > 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.
> > Disabling mremap is not a permission issue, it's more of an invalid
> > call?  I don't know, what do other drivers do?
> > 
> > thanks,
> > 
> > greg k-h
> Hi Greg,
> 
> Thank you for your feedback.
> 
> 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).

Great, can you please include that in the changelog text?

thanks,

greg k-h

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

[huangchenghai]

On Sat, Sep 13, 2025 at 7:06 PM, Greg KH wrote:
> On Sat, Sep 13, 2025 at 06:40:23PM +0800, huangchenghai wrote:
>> On Sat, 6 Sept 2025 at 20:03, Greg KH wrote:
>>> On Thu, Aug 28, 2025 at 01:59:48PM +0800, Zhangfei Gao wrote:
>>>> 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.
>>> Disabling mremap is not a permission issue, it's more of an invalid
>>> call?  I don't know, what do other drivers do?
>>>
>>> thanks,
>>>
>>> greg k-h
>> Hi Greg,
>>
>> Thank you for your feedback.
>>
>> 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).
> Great, can you please include that in the changelog text?
>
> thanks,
>
> greg k-h
Sure, I will add changelog in the v2 patch lately.

Thanks,
ChengHai