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

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

[Liu Zixian]

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
--- a/mm/mmap.c
+++ 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()
+		 */
+		WARN_ON_ONCE(addr != vma->vm_start);
+
+		addr = vma->vm_start;
+
 		/* If vm_flags changed after call_mmap(), we should try merge vma again
 		 * as we may succeed this time.
 		 */
@@ -1822,25 +1833,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 				fput(vma->vm_file);
 				vm_area_free(vma);
 				vma = merge;
-				/* Update vm_flags and possible addr to pick up the change. We don't
-				 * warn here if addr changed as the vma is not linked by vma_link().
-				 */
-				addr = vma->vm_start;
+				/* Update vm_flags to pick up the change. */
 				vm_flags = vma->vm_flags;
 				goto unmap_writable;
 			}
 		}
 
-		/* 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()
-		 */
-		WARN_ON_ONCE(addr != vma->vm_start);
-
-		addr = vma->vm_start;
 		vm_flags = vma->vm_flags;
 	} else if (vm_flags & VM_SHARED) {
 		error = shmem_zero_setup(vma);
-- 
2.23.0

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

[Andrew Morton]

On Thu, 3 Dec 2020 16:53:50 +0800 Liu Zixian <liuzixian4@huawei.com> 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.

Let's cc David and Jason, as they commented on v1.

You cc'ed stable@vger.kernel.org on the email, but there's no
cc:stable@vger.kernel.org in the changelog tags.  There should be.

Has this bug actually been observed at runtime, or is it a theoretical
from-code-inspection thing?

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

[Liu Zixian]

> Has this bug actually been observed at runtime, or is it a theoretical
> from-code-inspection thing?

We have a driver which changes vm_flags, and this bug is found by our testcases.

Thanks to everyone for your review and email instructions, I will do better next time.

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

[David Hildenbrand]

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
> --- a/mm/mmap.c
> +++ 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?

Anyhow, unrelated to your patch

Reviewed-by: David Hildenbrand <david@redhat.com>



-- 
Thanks,

David / dhildenb

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

[Jason Gunthorpe]

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

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

[Jason Gunthorpe]

On Thu, Dec 03, 2020 at 04:53:50PM +0800, 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(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

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

[Matthew Wilcox]

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@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
>  }

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.

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

[Jason Gunthorpe]

On Fri, Dec 04, 2020 at 03:25:35PM +0000, Matthew Wilcox wrote:

> 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.

Presumably the latter, otherwise people would be complaining about the
WARN_ON.

With some grep I could only find this:

static int mc68x328fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
#ifndef MMU
        /* this is uClinux (no MMU) specific code */

        vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
        vma->vm_start = videomemory;

        return 0;
#else
        return -EINVAL;
#endif
}

So it does seem gone

> 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.

If the driver adjust the address, and it doesn't collide with another
vma, and it doesn't change the tree position, then it could work

But if the driver radically changes the vm_start all bets are off and
you end up with an unsorted rb_tree at worst.

Banning drivers from adjusting the vm_start/end makes sense to me, at
least. How could a driver do this correctly anyhow?

Jason

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

[Matthew Wilcox]

On Fri, Dec 04, 2020 at 12:04:51PM -0400, Jason Gunthorpe wrote:
> On Fri, Dec 04, 2020 at 03:25:35PM +0000, Matthew Wilcox wrote:
> 
> > 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.
> 
> Presumably the latter, otherwise people would be complaining about the
> WARN_ON.
> 
> With some grep I could only find this:
> 
> static int mc68x328fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> {
> #ifndef MMU
>         /* this is uClinux (no MMU) specific code */
> 
>         vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
>         vma->vm_start = videomemory;
> 
>         return 0;
> #else
>         return -EINVAL;
> #endif
> }
> 
> So it does seem gone

I found some buggy, uncompilable code in binder that used to modify it:

-#ifdef CONFIG_CPU_CACHE_VIPT
-       if (cache_is_vipt_aliasing()) {
-               while (CACHE_COLOUR(
-                       pr_info("%s: %d %lx-%lx maps %pK bad alignment\n",
-                               __func__,
-                               alloc->pid, vma->vm_start, vma->vm_end,
-                               alloc->buffer);
-                       vma->vm_start += PAGE_SIZE;
-               }
-       }
-#endif

pretty sure that even if the syntax error were fixed, it would have
done something awful and wrong.

It seems like it used to be quite popular to do it for nommu-fb-mmap,
but it fell out of favour.  If there was a sparc fb driver that did it,
it got deleted before 2.6.12-rc2, so I don't really care.

So yes, let's turn this into a BUG_ON.