[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <868ceb0e-f4ba-4495-a1e1-0e387049281a@huawei.com>
Date: Sat, 13 Sep 2025 18:40:23 +0800
From: huangchenghai <huangchenghai2@...wei.com>
To: Greg KH <gregkh@...uxfoundation.org>, Zhangfei Gao
<zhangfei.gao@...aro.org>
CC: <wangzhou1@...ilicon.com>, <linux-kernel@...r.kernel.org>,
<linuxarm@...wei.com>, <linux-crypto@...r.kernel.org>,
<fanghao11@...wei.com>, <shenyang39@...wei.com>, <qianweili@...wei.com>,
<linwenkai6@...ilicon.com>, <liulongfang@...wei.com>
Subject: Re: [PATCH 3/4] uacce: implement mremap in uacce_vm_ops to return
-EPERM
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@...uxfoundation.org> wrote:
>>> On Fri, Aug 22, 2025 at 06:39:03PM +0800, Chenghai Huang wrote:
>>>> From: Yang Shen <shenyang39@...wei.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
Powered by blists - more mailing lists