userfaultfd: fix remap event with MREMAP_DONTUNMAP.

PATCH In review Brian Geffon 2 participants 7 messages May 6, 2020 – May 7, 2020

Subsystems (2)

Memory management Memory mapping

lore

Share & tools

lore

Brian Geffon

author

2020-05-06 17:22 UTC #0 lore raw

PATCH 1/1 userfaultfd: fix remap event with MREMAP_DONTUNMAP.

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 <[email protected]>

---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mremap.c +1 -1

index c881abeba0bf..6aa6ea605068 100644
@@ -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;
 }

Signature · 1 line

2.26.2.526.g744177e7f7-goog

Joel Fernandes

2020-05-06 17:28 UTC #1 lore raw

Quoted 11 lines · 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.

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

Quoted 21 lines

Signed-off-by Brian Geffon <[email protected]>

---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/mremap.c +1 -2

index c881abeba0bf..6aa6ea605068 100644
@@ -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

Brian Geffon

author

2020-05-06 17:32 UTC #2 lore raw

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

Quoted 42 lines · Joel Fernandes

Quoted 11 lines · 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.
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
Quoted 21 lines
Signed-off-by Brian Geffon <[email protected]>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/mremap.c +1 -2
index c881abeba0bf..6aa6ea605068 100644
@@ -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

Joel Fernandes

2020-05-07 00:56 UTC #3 lore raw

Hi Brian,

Quoted 3 lines · 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?

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.

Quoted 4 lines

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

Quoted 47 lines

Brian
Quoted 42 lines · Joel Fernandes
Quoted 11 lines · 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.
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
Quoted 21 lines
Signed-off-by Brian Geffon <[email protected]>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/mremap.c +1 -2
index c881abeba0bf..6aa6ea605068 100644
@@ -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

Joel Fernandes

2020-05-07 01:06 UTC #4 lore raw

In reply to Brian Geffon

Quoted 21 lines · 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 <[email protected]>
---
 mm/mremap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/mremap.c +0 -0
index c881abeba0bf..6aa6ea605068 100644

mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 797

Quoted 5 lines

mm/mremap.c +1 -1

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

Quoted 6 lines

mm/mremap.c +0 -1

        userfaultfd_unmap_complete(mm, &uf_unmap);
        return ret;
 }
--

2.26.2.526.g744177e7f7-goog

Brian Geffon

author

2020-05-07 01:11 UTC #5 lore raw

Quoted 6 lines

mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 797
Quoted 2 lines
mm/mremap.c +1 -1
-       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

Joel Fernandes

2020-05-07 01:35 UTC #6 lore raw

Quoted 8 lines · Brian Geffon

Quoted 7 lines
mm/mremap.c SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len, · line 797
Quoted 3 lines
mm/mremap.c +1 -1
-       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.

Quoted 8 lines

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