Re: [PATCH] mm: Add MREMAP_DONTUNMAP to mremap().

[kbuild test robot]

CC: kbuil...@lists.01.org
In-Reply-To: <20200123014627....@google.com>
References: <20200123014627....@google.com>
TO: Brian Geffon <bge...@google.com>
CC: linu...@kvack.org, Andrew Morton <ak...@linux-foundation.org>, "Michael S . Tsirkin" <m...@redhat.com>, Arnd Bergmann <ar...@arndb.de>, Brian Geffon <bge...@google.com>, Sonny Rao <sonn...@google.com>, Minchan Kim <min...@kernel.org>, Joel Fernandes <jo...@joelfernandes.org>, Lokesh Gidra <lokes...@google.com>, linux-...@vger.kernel.org, linu...@vger.kernel.org, Yu Zhao <yuz...@google.com>, Jesse Barnes <jsba...@google.com>, Andrew Morton <ak...@linux-foundation.org>, "Michael S . Tsirkin" <m...@redhat.com>, Arnd Bergmann <ar...@arndb.de>, Brian Geffon <bge...@google.com>, Sonny Rao <sonn...@google.com>, Minchan Kim <min...@kernel.org>, Joel Fernandes <jo...@joelfernandes.org>, Lokesh Gidra <lokes...@google.com>, linux-...@vger.kernel.org, linu...@vger.kernel.org, Yu Zhao <yuz...@google.com>, Jesse Barnes <jsba...@google.com>
CC: Andrew Morton <ak...@linux-foundation.org>, "Michael S . Tsirkin" <m...@redhat.com>, Arnd Bergmann <ar...@arndb.de>, Brian Geffon <bge...@google.com>, Sonny Rao <sonn...@google.com>, Minchan Kim <min...@kernel.org>, Joel Fernandes <jo...@joelfernandes.org>, Lokesh Gidra <lokes...@google.com>, linux-...@vger.kernel.org, linu...@vger.kernel.org, Yu Zhao <yuz...@google.com>, Jesse Barnes <jsba...@google.com>

Hi Brian,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.5-rc7 next-20200124]
[cannot apply to mmotm/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Brian-Geffon/mm-Add-MREMAP_DONTUNMAP-to-mremap/20200125-013342
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4703d9119972bf586d2cca76ec6438f819ffa30e
config: arm64-defconfig (attached as .config)
compiler: clang version 11.0.0 (git://gitmirror/llvm_project 0b83c5a78fae96dd66150e7a14c8c6d0292de01d)
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <l...@intel.com>

All warnings (new ones prefixed by >>):

>> mm/mremap.c:562:5: warning: variable 'vma' is uninitialized when used here [-Wuninitialized]
vma->vm_flags, old_len >> PAGE_SHIFT))) {
^~~
include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
mm/mremap.c:513:28: note: initialize the variable 'vma' to silence this warning
struct vm_area_struct *vma;
^
= NULL
1 warning generated.

vim +/vma +562 mm/mremap.c

505
506 static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
507 unsigned long new_addr, unsigned long new_len, bool *locked,
508 unsigned long flags, struct vm_userfaultfd_ctx *uf,
509 struct list_head *uf_unmap_early,
510 struct list_head *uf_unmap)
511 {
512 struct mm_struct *mm = current->mm;
513 struct vm_area_struct *vma;
514 unsigned long ret = -EINVAL;
515 unsigned long charged = 0;
516 unsigned long map_flags;
517
518 if (offset_in_page(new_addr))
519 goto out;
520
521 if (new_len > TASK_SIZE || new_addr > TASK_SIZE - new_len)
522 goto out;
523
524 /* Ensure the old/new locations do not overlap */
525 if (addr + old_len > new_addr && new_addr + new_len > addr)
526 goto out;
527
528 /*
529 * move_vma() need us to stay 4 maps below the threshold, otherwise
530 * it will bail out at the very beginning.
531 * That is a problem if we have already unmaped the regions here
532 * (new_addr, and old_addr), because userspace will not know the
533 * state of the vma's after it gets -ENOMEM.
534 * So, to avoid such scenario we can pre-compute if the whole
535 * operation has high chances to success map-wise.
536 * Worst-scenario case is when both vma's (new_addr and old_addr) get
537 * split in 3 before unmaping it.
538 * That means 2 more maps (1 for each) to the ones we already hold.
539 * Check whether current map count plus 2 still leads us to 4 maps below
540 * the threshold, otherwise return -ENOMEM here to be more safe.
541 */
542 if ((mm->map_count + 2) >= sysctl_max_map_count - 3)
543 return -ENOMEM;
544
545 ret = do_munmap(mm, new_addr, new_len, uf_unmap_early);
546 if (ret)
547 goto out;
548
549 if (old_len >= new_len) {
550 ret = do_munmap(mm, addr+new_len, old_len - new_len, uf_unmap);
551 if (ret && old_len != new_len)
552 goto out;
553 old_len = new_len;
554 }
555
556 /*
557 * MREMAP_DONTUNMAP expands by old_len + (new_len - old_len), we will
558 * check that we can expand by old_len and vma_to_resize will handle
559 * the vma growing.
560 */
561 if (unlikely(flags & MREMAP_DONTUNMAP && !may_expand_vm(mm,
> 562 vma->vm_flags, old_len >> PAGE_SHIFT))) {
563 ret = -ENOMEM;
564 goto out;
565 }
566
567 vma = vma_to_resize(addr, old_len, new_len, &charged);
568 if (IS_ERR(vma)) {
569 ret = PTR_ERR(vma);
570 goto out;
571 }
572
573 map_flags = MAP_FIXED;
574 if (vma->vm_flags & VM_MAYSHARE)
575 map_flags |= MAP_SHARED;
576
577 ret = get_unmapped_area(vma->vm_file, new_addr, new_len, vma->vm_pgoff +
578 ((addr - vma->vm_start) >> PAGE_SHIFT),
579 map_flags);
580 if (IS_ERR_VALUE(ret))
581 goto out1;
582
583 ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, flags, uf,
584 uf_unmap);
585 if (!(offset_in_page(ret)))
586 goto out;
587 out1:
588 vm_unacct_memory(charged);
589
590 out:
591 return ret;
592 }
593

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuil...@lists.01.org Intel Corporation

[Nathan Chancellor]

Hi Brian,

On Fri, Jan 24, 2020 at 11:06:25AM -0800, Brian Geffon wrote:
> When remapping an anonymous, private mapping, if MREMAP_DONTUNMAP is
> set, the source mapping will not be removed. Instead it will be
> cleared as if a brand new anonymous, private mapping had been created
> atomically as part of the mremap() call. If a userfaultfd was watching
> the source, it will continue to watch the new mapping. For a mapping
> that is shared or not anonymous, MREMAP_DONTUNMAP will cause the
> mremap() call to fail. MREMAP_DONTUNMAP implies that MREMAP_FIXED is
> also used. The final result is two equally sized VMAs where the
> destination contains the PTEs of the source.
>
> We hope to use this in Chrome OS where with userfaultfd we could write
> an anonymous mapping to disk without having to STOP the process or worry
> about VMA permission changes.
>
> This feature also has a use case in Android, Lokesh Gidra has said
> that "As part of using userfaultfd for GC, We'll have to move the physical
> pages of the java heap to a separate location. For this purpose mremap
> will be used. Without the MREMAP_DONTUNMAP flag, when I mremap the java
> heap, its virtual mapping will be removed as well. Therefore, we'll
> require performing mmap immediately after. This is not only time consuming
> but also opens a time window where a native thread may call mmap and
> reserve the java heap's address range for its own usage. This flag
> solves the problem."
>
> Signed-off-by: Brian Geffon <bge...@google.com>
> ---
> include/uapi/linux/mman.h | 5 +++--
> mm/mremap.c | 37 ++++++++++++++++++++++++++++++-------
> 2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/include/uapi/linux/mman.h b/include/uapi/linux/mman.h
> index fc1a64c3447b..923cc162609c 100644
> --- a/include/uapi/linux/mman.h
> +++ b/include/uapi/linux/mman.h
> @@ -5,8 +5,9 @@
> #include <asm/mman.h>
> #include <asm-generic/hugetlb_encode.h>
>
> -#define MREMAP_MAYMOVE 1
> -#define MREMAP_FIXED 2
> +#define MREMAP_MAYMOVE 1
> +#define MREMAP_FIXED 2
> +#define MREMAP_DONTUNMAP 4
>
> #define OVERCOMMIT_GUESS 0
> #define OVERCOMMIT_ALWAYS 1
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 122938dcec15..bf97c3eb538b 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -318,8 +318,8 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
> static unsigned long move_vma(struct vm_area_struct *vma,
> unsigned long old_addr, unsigned long old_len,
> unsigned long new_len, unsigned long new_addr,
> - bool *locked, struct vm_userfaultfd_ctx *uf,
> - struct list_head *uf_unmap)
> + bool *locked, unsigned long flags,
> + struct vm_userfaultfd_ctx *uf, struct list_head *uf_unmap)
> {
> struct mm_struct *mm = vma->vm_mm;
> struct vm_area_struct *new_vma;
> @@ -408,6 +408,13 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> if (unlikely(vma->vm_flags & VM_PFNMAP))
> untrack_pfn_moved(vma);
>
> + if (unlikely(!err && (flags & MREMAP_DONTUNMAP))) {
> + if (vm_flags & VM_ACCOUNT)
> + vma->vm_flags |= VM_ACCOUNT;
> +
> + goto out;
> + }
> +
> if (do_munmap(mm, old_addr, old_len, uf_unmap) < 0) {
> /* OOM: unable to split vma, just get accounts right */
> vm_unacct_memory(excess >> PAGE_SHIFT);
> @@ -422,6 +429,7 @@ static unsigned long move_vma(struct vm_area_struct *vma,
> vma->vm_next->vm_flags |= VM_ACCOUNT;
> }
>
> +out:
> if (vm_flags & VM_LOCKED) {
> mm->locked_vm += new_len >> PAGE_SHIFT;
> *locked = true;
> @@ -497,7 +505,7 @@ static struct vm_area_struct *vma_to_resize(unsigned long addr,

>
> static unsigned long mremap_to(unsigned long addr, unsigned long old_len,

> unsigned long new_addr, unsigned long new_len, bool *locked,

> - struct vm_userfaultfd_ctx *uf,
> + unsigned long flags, struct vm_userfaultfd_ctx *uf,
> struct list_head *uf_unmap_early,
> struct list_head *uf_unmap)
> {
> @@ -545,6 +553,17 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> old_len = new_len;
> }
>
> + /*
> + * MREMAP_DONTUNMAP expands by old_len + (new_len - old_len), we will
> + * check that we can expand by old_len and vma_to_resize will handle
> + * the vma growing.
> + */
> + if (unlikely(flags & MREMAP_DONTUNMAP && !may_expand_vm(mm,
> + vma->vm_flags, old_len >> PAGE_SHIFT))) {

We received a Clang build report that vma is used uninitialized here
(they aren't being publicly sent to LKML due to GCC vs Clang
warning/error overlap):

https://groups.google.com/d/msg/clang-built-linux/gE5wRaeHdSI/xVA0MBQVEgAJ

Sure enough, vma is initialized first in the next block. Not sure if
this section should be moved below that initialization or if something
else should be done to resolve it but that dereference will obviously be
fatal.

Cheers,
Nathan

> + ret = -ENOMEM;
> + goto out;
> + }
> +

> vma = vma_to_resize(addr, old_len, new_len, &charged);

> if (IS_ERR(vma)) {
> ret = PTR_ERR(vma);
> @@ -561,7 +580,7 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len,
> if (IS_ERR_VALUE(ret))
> goto out1;
>
> - ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, uf,
> + ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, flags, uf,
> uf_unmap);
> if (!(offset_in_page(ret)))
> goto out;
> @@ -609,12 +628,15 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
> addr = untagged_addr(addr);
> new_addr = untagged_addr(new_addr);
>
> - if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE))
> + if (flags & ~(MREMAP_FIXED | MREMAP_MAYMOVE | MREMAP_DONTUNMAP))
> return ret;
>
> if (flags & MREMAP_FIXED && !(flags & MREMAP_MAYMOVE))
> return ret;
>
> + if (flags & MREMAP_DONTUNMAP && !(flags & MREMAP_FIXED))
> + return ret;
> +
> if (offset_in_page(addr))
> return ret;
>
> @@ -634,7 +656,8 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,
>
> if (flags & MREMAP_FIXED) {
> ret = mremap_to(addr, old_len, new_addr, new_len,
> - &locked, &uf, &uf_unmap_early, &uf_unmap);
> + &locked, flags, &uf, &uf_unmap_early,
> + &uf_unmap);
> goto out;
> }
>
> @@ -712,7 +735,7 @@ SYSCALL_DEFINE5(mremap, unsigned long, addr, unsigned long, old_len,

> }
>
> ret = move_vma(vma, addr, old_len, new_len, new_addr,

> - &locked, &uf, &uf_unmap);
> + &locked, flags, &uf, &uf_unmap);
> }
> out:
> if (offset_in_page(ret)) {
> --
> 2.25.0.341.g760bfbb309-goog
>

[Brian Geffon]

Hi Nathan,
Thank you! That was an oversight on my part. I'll address it in the next patch.

Brian