Re: [PATCH] mm/mmap: Fix MAP_FIXED address return on VMA merge

[Liam Howlett]

* David Hildenbrand <david@xxxxxxxxxx> [221018 15:27]:
> On 18.10.22 21:17, Liam Howlett wrote:
> > mmap should return the start address of newly mapped area when
> > successful.  On a successful merge of a VMA, the return address was
> > changed and thus was violating that expectation from userspace.
> > 
> 
> Just wondering, do we have a simple user space reproducer / test?

Not today, no.

> 
> Do we want to add some more tests for such scenarios?

Yes, I will code something up for LTP.

> 
> > This is a restoration of functionality provided by 309d08d9b3a3
> > (mm/mmap.c: fix mmap return value when vma is merged after call_mmap()).
> > For completeness of fixing MAP_FIXED, implement the comments from the
> > previous discussion to never update the address and fail if the address
> > changes.  Leaving the error as a WARN_ON() to avoid crashing the kernel.
> > 
> > Cc: Liu Zixian <liuzixian4@xxxxxxxxxx>
> > Cc: David Hildenbrand <david@xxxxxxxxxx>
> > Cc: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> > Link: https://lore.kernel.org/all/Y06yk66SKxlrwwfb@lakrids/
> > Link: https://lore.kernel.org/all/20201203085350.22624-1-liuzixian4@xxxxxxxxxx/
> > Fixes: 4dd1b84140c1 (mm/mmap: use advanced maple tree API for mmap_region())
> > Reported-by: Mark Rutland <mark.rutland@xxxxxxx>
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> > ---
> >   mm/mmap.c | 15 +++++++--------
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 42cd2c260898..22010e13f1a1 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2625,14 +2625,14 @@ 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
> > +		/*
> > +		 * Expansion is handled above, merging is handled below.
> > +		 * Drivers should not alter the address of the VMA.
> >   		 */
> > -		WARN_ON_ONCE(addr != vma->vm_start);
> > -
> > -		addr = vma->vm_start;
> > +		if (WARN_ON((addr != vma->vm_start))) {
> > +			error = -EINVAL;
> > +			goto close_and_free_vma;
> > +		}
> 
> If this is something that user space can trigger, WARN_* is the wrong
> choice. But what I understand from the comment change is that this must not
> happen at that point unless there is a real issue.

The VMA start address could be changed in call_mmap() which is a driver
call.  I guess someone could write a driver to mmap by a users action?
I don't think it can be reached other ways.  In any case, I'm changing a
WARN_ON_ONCE() to a WARN_ON() and undoing the badness instead of
marching forwards.

> 
> Why not "if (WARN_ON_ONCE)" ?

I was thinking it was harder to ignore if it happen more frequently?
There isn't a driver that does this now, but I'm not picky over which
variant to call.

Thanks,
Liam