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

[Greg KH <gregkh@linuxfoundation.org>]

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