Re: [Devel] [PATCH RFC PATCH hci-8.1 3/4] mm: move MADV_DOEXEC memory instead of copying it

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] mm: vma_dup() copies vm_file without get_file() for private /dev/zero VMAs
- [Low] mm: vma_dup() discards ksm_madvise() flags so VM_MERGEABLE stays on moved VMA
- [Low] mm: padata workers serialize on anon_vma write lock in move_ptes()
--

commit b89406aa304220e73194db3c04662412c6cbfe44
Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

mm: move MADV_DOEXEC memory instead of copying it

This commit changes MADV_DOEXEC preservation across exec from a
fork-style copy to a move of the page tables into the new mm. vma_dup()
now clones the anon_vma chain, unmerges KSM, drops mlock state, and calls
move_page_range_exec(). The parallel copy code and the exec-specific
pinned-page exception in the rmap dup path are removed.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-103860

> diff --git a/mm/mmap.c b/mm/mmap.c
> --- a/mm/mmap.c
> +++ b/mm/mmap.c

[ ... ]

>  int vma_dup(struct vm_area_struct *old_vma, struct mm_struct *mm)
>  {
>  	struct vm_area_struct *vma;
>  	unsigned long npages;
> +	vm_flags_t flags = old_vma->vm_flags;
>  	int ret = -ENOMEM;

[ ... ]

> +	/* KSM tracks mappings by mm and address; unmerge before moving them. */
> +	ret = ksm_madvise(old_vma, old_vma->vm_start, old_vma->vm_end,
> +			  MADV_UNMERGEABLE, &flags);
> +	if (ret)
> +		return ret;

[Severity: Low]
Is the result of this ksm_madvise() call ever applied?

ksm_madvise() clears VM_MERGEABLE only through its vm_flags output
argument:

mm/ksm.c:ksm_madvise() {
    ...
    *vm_flags &= ~VM_MERGEABLE;
    ...
}

Here that argument is the local variable flags, which is never written
back to old_vma->vm_flags and is not used afterwards.

vm_area_dup() then copies the original vm_flags into the new VMA, and the
later vm_flags_clear() on the new VMA does not include VM_MERGEABLE:

> +	vm_flags_clear(vma, VM_LOCKED_MASK | VM_UFFD_MISSING | VM_UFFD_WP |
> +		       VM_UFFD_MINOR | VM_EXEC_KEEP);

[ ... ]

> +	if (!test_bit(MMF_VM_MERGEABLE, &mm->flags))
> +		ksm_fork(mm, old_vma->vm_mm);
> +	return move_page_range_exec(vma, old_vma);

For a VMA with both MADV_DOEXEC and MADV_MERGEABLE, ksm_fork() registers
the new mm with ksmd while the moved VMA still has VM_MERGEABLE set.
Could ksmd then scan and re-merge pages in the moved VMA?

The commit message says the patch will "unmerge KSM". Is the intent to
leave the moved VMA KSM-eligible, as fork() does, or to disable KSM on it?
If the latter, should VM_MERGEABLE be cleared on the new VMA, for example
by adding it to the vm_flags_clear() call?

> +	/*
> +	 * Transfer the reservation to the linked destination VMA. If moving
> +	 * page tables fails partway through, exec cannot resume the old image:
> +	 * it must terminate and tear down both mms. Each mm then frees its
> +	 * remaining mappings, and the destination releases the reservation.
>  	 */
> -	__vm_flags_mod(old_vma, 0, VM_ACCOUNT);
> +	vm_flags_clear(old_vma, VM_ACCOUNT);

[ ... ]

>  	vma = vm_area_dup(old_vma);
>  	if (!vma)
>  		goto fail_nomem;

[Severity: Critical]
Can this leave two VMAs holding a single reference on vm_file?

vm_area_dup() memcpy()s the VMA, including vm_file, and takes no file
reference. Nothing later in vma_dup() calls get_file() or clears vm_file,
and vma_link() then calls vma_link_file() on the new VMA.

The MADV_DOEXEC gate in madvise_vma_behavior() only requires
vma_is_anonymous() and rejects VM_EXEC, VM_SHARED and VM_STACK. It does
not reject vm_file != NULL. A private mapping of /dev/zero passes that
gate, because mmap_zero() calls vma_set_anonymous() and leaves vm_file
set:

drivers/char/mem.c:mmap_zero() {
    ...
    if (vma->vm_flags & VM_SHARED)
        return shmem_zero_setup(vma);
    vma_set_anonymous(vma);
    return 0;
}

The call chain would then be:

exec_mmap()
  vma_dup_some()
    vma_dup()
      vm_area_dup()    /* copies vm_file, no get_file() */
      vma_link()       /* new VMA now in the file's i_mmap */

Each mm's teardown then reaches remove_vma():

mm/vma.c:remove_vma() {
    ...
    if (vma->vm_file)
        fput(vma->vm_file);
}

That would call fput() twice for one reference. Should vma_dup() either
take get_file() when vma->vm_file is set, or reject VMAs with a vm_file in
the MADV_DOEXEC check?

The old vma_dup() had the same vm_area_dup() path. However, this patch
rewrites the function and builds the new move semantics on it, so it seemed
worth raising here.

> diff --git a/mm/mremap.c b/mm/mremap.c

[ ... ]

> static int move_page_range_chunk(unsigned long addr, unsigned long end, void *arg)
> {
> 	...
> 	return move_page_tables(args->old_vma, addr, args->new_vma, addr,
> 				len, true, false) == len ? 0 : -ENOMEM;
> }

[Severity: Low]
Is the 16-thread padata job in move_page_range_exec() able to scale here?

Every worker calls move_page_tables() with need_rmap_locks set to true.
For each PMD extent, move_ptes() then does:

move_page_range_chunk()
  move_page_tables(..., need_rmap_locks = true, ...)
    move_ptes()
      take_rmap_locks()
        anon_vma_lock_write(vma->anon_vma)

The anon_vma write lock is held across the per-PMD PTE move loop and the
TLB flush, and is dropped only in drop_rmap_locks() at the out label. All
workers operate on the same old_vma, so they take the same anon_vma root
rwsem for write.

Does this mostly serialize the workers, so that padata_do_multithreaded()
with max_threads = 16 gives little speedup over a single thread? I don't
see a correctness problem. Has the performance relative to the old
copy_page_range_mt() path been measured?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261009-vfio_v1-v1-0-1901cd6c3c74@virtuozzo.com?part=3

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel