Re: [PATCH v2] fix mmap return value when vma is merged after call_mmap()

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Dec 04, 2020 at 03:11:54PM +0100, David Hildenbrand wrote:
> On 03.12.20 09:53, Liu Zixian wrote:
> > On success, mmap should return the begin address of newly mapped area,
> > but patch "mm: mmap: merge vma after call_mmap() if possible"
> > set vm_start of newly merged vma to return value addr.
> > Users of mmap will get wrong address if vma is merged after call_mmap().
> > We fix this by moving the assignment to addr before merging vma.
> > 
> > Fixes: d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible")
> > Signed-off-by: Liu Zixian <liuzixian4@huawei.com>
> > v2:
> > We want to do "addr = vma->vm_start;" unconditionally,
> > so move assignment to addr before if(unlikely) block.
> >  mm/mmap.c | 26 ++++++++++++--------------
> >  1 file changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index d91ecb00d38c..5c8b4485860d 100644
> > +++ b/mm/mmap.c
> > @@ -1808,6 +1808,17 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> >  		if (error)
> >  			goto unmap_and_free_vma;
> >  
> > +		/* Can addr have changed??
> > +		 *
> > +		 * Answer: Yes, several device drivers can do it in their
> > +		 *         f_op->mmap method. -DaveM
> > +		 * Bug: If addr is changed, prev, rb_link, rb_parent should
> > +		 *      be updated for vma_link()
> > +		 */
> 
> 
> Why do we tolerate device drivers doing such stuff at all?
> WARN_ON_ONCE() is just as good as BUG_ON() in many environments. If we
> support it, then no warning. If we don't support it, then why tolerate it?

The commit that introduced this seemed pretty clear it is to catch
possibly wrong drivers. I suppose the idea was to give a migration
time where things would "work" and drivers could be fixed. Since it
has now been 8 years it should be either dropped or turned into:

 /* Drivers are not permitted to change vm_start */
 if (WARN_ON(addr != vma->vm_start)) {
     err = EINVAL
     goto unmap_and_free_vma
 }


commit 2897b4d29d9fca82a57b09b8a216a5d604966e4b
Author: Joonsoo Kim <js1304@gmail.com>
Date:   Wed Dec 12 13:51:53 2012 -0800

    mm: WARN_ON_ONCE if f_op->mmap() change vma's start address
    
    During reviewing the source code, I found a comment which mention that
    after f_op->mmap(), vma's start address can be changed.  I didn't verify
    that it is really possible, because there are so many f_op->mmap()
    implementation.  But if there are some mmap() which change vma's start
    address, it is possible error situation, because we already prepare prev
    vma, rb_link and rb_parent and these are related to original address.
    
    So add WARN_ON_ONCE for finding that this situtation really happens.

Jason