On Fri, Dec 04, 2020 at 11:10:28AM -0400, Jason Gunthorpe wrote:
> 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@xxxxxxxxxx>
> > > 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
> }
This commit makes no sense. I know it's eight years old, so maybe the
device driver which did this has long been removed from the tree, but
davem's comment was (iirc) related to a device driver for a graphics
card that would 256MB-align the user address. Another possibility is
that userspace always asks for a 256MB-aligned address these days.
I don't understand why prev/rb_link/rb_parent would need to be changed
in this case. It's going to be inserted at the exact same location in
the rbtree, just at a slightly shifted address.
