| Message ID | 20200506172158.218366-1-bgeffon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | userfaultfd: fix remap event with MREMAP_DONTUNMAP. | expand |
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 >
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
> >
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 > > >
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 >
> > - 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
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
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; }
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(-)