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

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

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

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

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

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

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

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