[PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Brian Geffon]

A user is not required to set a new address when using
MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
When doing so the remap event will use new_addr which may not
have been set and we didn't propagate it back other then
in the return value of remap_to.

Because ret is always the new address it's probably more
correct to use it rather than new_addr on the remap_event_complete
call, and it resolves this bug.

Signed-off-by: Brian Geffon <bgeffon@google.com>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mremap.c b/mm/mremap.c
index c881abeba0bf..6aa6ea605068 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
 	if (locked && new_len > old_len)
 		mm_populate(new_addr + old_len, new_len - old_len);
 	userfaultfd_unmap_complete(mm, &uf_unmap_early);
-	mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
+	mremap_userfaultfd_complete(&uf, addr, ret, old_len);
 	userfaultfd_unmap_complete(mm, &uf_unmap);
 	return ret;
 }
-- 
2.26.2.526.g744177e7f7-goog

Re: [PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Joel Fernandes]

On Wed, May 6, 2020 at 1:22 PM Brian Geffon <bgeffon@google.com> wrote:
>
> A user is not required to set a new address when using
> MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
> When doing so the remap event will use new_addr which may not
> have been set and we didn't propagate it back other then
> in the return value of remap_to.
>
> Because ret is always the new address it's probably more
> correct to use it rather than new_addr on the remap_event_complete
> call, and it resolves this bug.
>

Does it need Fixes: tag, and CC to stable? Going into a stable kernel
will mean the stable kernel merges into ChromeOS also gets it.

thanks,

 - Joel

> Signed-off-by: Brian Geffon <bgeffon@google.com>
> ---
>  mm/mremap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c881abeba0bf..6aa6ea605068 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>         if (locked && new_len > old_len)
>                 mm_populate(new_addr + old_len, new_len - old_len);
>         userfaultfd_unmap_complete(mm, &uf_unmap_early);
> -       mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> +       mremap_userfaultfd_complete(&uf, addr, ret, old_len);
>         userfaultfd_unmap_complete(mm, &uf_unmap);
>         return ret;
>  }
> --
> 2.26.2.526.g744177e7f7-goog
>

Re: [PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Brian Geffon]

It hasn't landed in a stable kernel yet, 5.7 is rc4 so I don't think
it needs to cc stable, right?

Andrew, I'd be happy to mail a new patch if necessary, otherwise here
is the fixes:

Fixes: e346b38 ("mm/mremap: add MREMAP_DONTUNMAP to mremap()")

Brian


On Wed, May 6, 2020 at 10:28 AM Joel Fernandes <joel@joelfernandes.org> wrote:
>
> On Wed, May 6, 2020 at 1:22 PM Brian Geffon <bgeffon@google.com> wrote:
> >
> > A user is not required to set a new address when using
> > MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
> > When doing so the remap event will use new_addr which may not
> > have been set and we didn't propagate it back other then
> > in the return value of remap_to.
> >
> > Because ret is always the new address it's probably more
> > correct to use it rather than new_addr on the remap_event_complete
> > call, and it resolves this bug.
> >
>
> Does it need Fixes: tag, and CC to stable? Going into a stable kernel
> will mean the stable kernel merges into ChromeOS also gets it.
>
> thanks,
>
>  - Joel
>
> > Signed-off-by: Brian Geffon <bgeffon@google.com>
> > ---
> >  mm/mremap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/mremap.c b/mm/mremap.c
> > index c881abeba0bf..6aa6ea605068 100644
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> >         if (locked && new_len > old_len)
> >                 mm_populate(new_addr + old_len, new_len - old_len);
> >         userfaultfd_unmap_complete(mm, &uf_unmap_early);
> > -       mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> > +       mremap_userfaultfd_complete(&uf, addr, ret, old_len);
> >         userfaultfd_unmap_complete(mm, &uf_unmap);
> >         return ret;
> >  }
> > --
> > 2.26.2.526.g744177e7f7-goog
> >

Re: [PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Joel Fernandes]

Hi Brian,

On Wed, May 6, 2020 at 1:32 PM Brian Geffon <bgeffon@google.com> wrote:
>
> It hasn't landed in a stable kernel yet, 5.7 is rc4 so I don't think
> it needs to cc stable, right?

I think the criteria is, if it has been merged into Linus's tree in a
kernel release, then CC'ing stable means any future stable releases of
the kernel will have the patch applied to the stable tree. The fix
patch itself will need to be merged into Linus tree at a future date
before being applied to the stable tree. But at least CC'ing stable
means it is tracked my stable bots and they'll do the right. This is
my understanding.

If the patch you are fixing was applied for v5.7 merge window, then I
don't think you need to CC stable.

> Andrew, I'd be happy to mail a new patch if necessary, otherwise here
> is the fixes:
>
> Fixes: e346b38 ("mm/mremap: add MREMAP_DONTUNMAP to mremap()")

I'd also avoid top-posting and send inline replies:
https://web.archive.org/web/20080722025748/http://www.zip.com.au/~akpm/linux/patches/stuff/top-posting.txt
https://lkml.org/lkml/2019/12/4/267

Thanks!

 - Joel


>
> Brian
>
>
> On Wed, May 6, 2020 at 10:28 AM Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> > On Wed, May 6, 2020 at 1:22 PM Brian Geffon <bgeffon@google.com> wrote:
> > >
> > > A user is not required to set a new address when using
> > > MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
> > > When doing so the remap event will use new_addr which may not
> > > have been set and we didn't propagate it back other then
> > > in the return value of remap_to.
> > >
> > > Because ret is always the new address it's probably more
> > > correct to use it rather than new_addr on the remap_event_complete
> > > call, and it resolves this bug.
> > >
> >
> > Does it need Fixes: tag, and CC to stable? Going into a stable kernel
> > will mean the stable kernel merges into ChromeOS also gets it.
> >
> > thanks,
> >
> >  - Joel
> >
> > > Signed-off-by: Brian Geffon <bgeffon@google.com>
> > > ---
> > >  mm/mremap.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > index c881abeba0bf..6aa6ea605068 100644
> > > --- a/mm/mremap.c
> > > +++ b/mm/mremap.c
> > > @@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> > >         if (locked && new_len > old_len)
> > >                 mm_populate(new_addr + old_len, new_len - old_len);
> > >         userfaultfd_unmap_complete(mm, &uf_unmap_early);
> > > -       mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> > > +       mremap_userfaultfd_complete(&uf, addr, ret, old_len);
> > >         userfaultfd_unmap_complete(mm, &uf_unmap);
> > >         return ret;
> > >  }
> > > --
> > > 2.26.2.526.g744177e7f7-goog
> > >

Re: [PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Joel Fernandes]

On Wed, May 6, 2020 at 1:22 PM Brian Geffon <bgeffon@google.com> wrote:
>
> A user is not required to set a new address when using
> MREMAP_DONTUNMAP as it can be used without MREMAP_FIXED.
> When doing so the remap event will use new_addr which may not
> have been set and we didn't propagate it back other then
> in the return value of remap_to.
>
> Because ret is always the new address it's probably more
> correct to use it rather than new_addr on the remap_event_complete
> call, and it resolves this bug.
>
> Signed-off-by: Brian Geffon <bgeffon@google.com>
> ---
>  mm/mremap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index c881abeba0bf..6aa6ea605068 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -794,7 +794,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>         if (locked && new_len > old_len)
>                 mm_populate(new_addr + old_len, new_len - old_len);
>         userfaultfd_unmap_complete(mm, &uf_unmap_early);
> -       mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> +       mremap_userfaultfd_complete(&uf, addr, ret, old_len);

Not super familiar with this code, but thought I'd ask, does ret need
to be checked for -ENOMEM before calling mremap_userfaultfd_complete?
Sorry if I missed something.

Thanks,

 - Joel

>         userfaultfd_unmap_complete(mm, &uf_unmap);
>         return ret;
>  }
> --
> 2.26.2.526.g744177e7f7-goog
>

Re: [PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Brian Geffon]

> > -       mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> > +       mremap_userfaultfd_complete(&uf, addr, ret, old_len);
>
> Not super familiar with this code, but thought I'd ask, does ret
> to be checked for -ENOMEM before calling mremap_userfaultfd_complete?
> Sorry if I missed something.

No, mremap_userfaultfd_complete will do a check similar to
offset_in_page() by checking the page mask.
It does (to & ~PAGE_MASK) to check for a non-aligned "to" value, so we're good.

Additionally, earlier in the process then ctx will be null because we
will have never called mremap_userfaultfd_prep,
and mremap_userfaultfd_complete will check if there is a context
before proceeding.

Brian

Re: [PATCH] userfaultfd: fix remap event with MREMAP_DONTUNMAP.

[Joel Fernandes]

On Wed, May 6, 2020 at 9:11 PM Brian Geffon <bgeffon@google.com> wrote:
>
> > > -       mremap_userfaultfd_complete(&uf, addr, new_addr, old_len);
> > > +       mremap_userfaultfd_complete(&uf, addr, ret, old_len);
> >
> > Not super familiar with this code, but thought I'd ask, does ret
> > to be checked for -ENOMEM before calling mremap_userfaultfd_complete?
> > Sorry if I missed something.
>
> No, mremap_userfaultfd_complete will do a check similar to
> offset_in_page() by checking the page mask.
> It does (to & ~PAGE_MASK) to check for a non-aligned "to" value, so we're good.
>
> Additionally, earlier in the process then ctx will be null because we
> will have never called mremap_userfaultfd_prep,
> and mremap_userfaultfd_complete will check if there is a context
> before proceeding.

Makes sense.

thanks,

 - Joel