Re: [Devel] [PATCH hci-8.1 6/8] oracle/mm: zap the preserved ranges of the old mm after copying them

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: zap_vma_mt() clears old_mm PTEs without vma_start_write()
--

commit f34df816bcbe51e2402a6b8ae419dea8ef679f49
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

oracle/mm: zap the preserved ranges of the old mm after copying them

After the VM_EXEC_KEEP ranges are copied into the new mm, this patch zaps
them from the old mm under its write lock. This is meant to leave the
copies as the only mappings of the preserved pages. A new zap_vma_mt()
helper spreads the zap of an anonymous VMA over several threads.

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

> diff --git a/fs/exec.c b/fs/exec.c
> --- a/fs/exec.c
> +++ b/fs/exec.c

[ ... ]

> @@ -997,7 +997,15 @@ static int vma_dup_some(struct mm_struct *old_mm, struct mm_struct *new_mm)
>  		}
>  
>  	mmap_write_unlock(new_mm);
> -	return ret;
> +	if (ret)
> +		return ret;
> +
> +	/* Whoever keeps old_mm alive past the exec must not reach the pages through it. */
> +	vma_iter_set(&vmi, 0);
> +	for_each_vma(vmi, vma)
> +		if (vma->vm_flags & VM_EXEC_KEEP)
> +			zap_vma_mt(vma);
> +	return 0;
>  }

[Severity: Medium]
Can this zap race with a per-VMA lock holder on old_mm?

The loop calls zap_vma_mt() on old_mm VMAs without calling
vma_start_write(vma) first:

    vma_dup_some()
      for_each_vma(vmi, vma)
        zap_vma_mt(vma)

mmap_write_lock() alone does not drain holders that already took the
per-VMA read lock through lock_vma_under_rcu().

One such holder is UFFDIO_MOVE:

    uffd_move_lock()
      *src_vmap = lock_vma_under_rcu(mm, src_start);

This path never takes mmap_lock. A task that owns a userfaultfd context
bound to old_mm has mm_users on old_mm, and its own t->mm is a different
mm. reserve_preserved_mem() only rejects tasks whose t->mm == mm, so such
a task is not excluded.

It could therefore run folio_move_anon_rmap() or a fault in old_mm while
the zap clears the same PTEs. That would reintroduce the problems the
commit message describes, since the preserved pages still carry
PageAnonExclusive in both mms.

Would it make sense to call vma_start_write(vma) before zapping each
VM_EXEC_KEEP VMA, either in vma_dup_some() or at the start of
zap_vma_mt()?

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

[ ... ]

> +void zap_vma_mt(struct vm_area_struct *vma)
> +{
> +#ifdef CONFIG_PADATA
> +	struct padata_mt_job job = {

[ ... ]

> +	mmap_assert_write_locked(vma->vm_mm);
> +	if (vma_is_anonymous(vma)) {
> +		lru_add_drain();
> +		mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
> +					vma->vm_start, vma->vm_end);
> +		update_hiwater_rss(vma->vm_mm);
> +		mmu_notifier_invalidate_range_start(&range);
> +		padata_do_multithreaded(&job);
> +		mmu_notifier_invalidate_range_end(&range);
> +		return;
> +	}
> +#endif
> +	zap_page_range_single(vma, vma->vm_start, vma->vm_end - vma->vm_start,
> +			      NULL);
> +}

[Severity: Medium]
The same concern applies here. zap_vma_mt() only asserts the mmap write
lock with mmap_assert_write_locked() before clearing PTEs, either in the
padata worker threads through zap_vma_chunk() or in
zap_page_range_single().

It never calls vma_start_write(vma).

For the anonymous case, can PTEs be cleared while a per-VMA locked fault
or UFFDIO_MOVE is still operating on the same VMA? Both the padata path
and the zap_page_range_single() path appear to be exposed.

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261014-b4-prep-khorenko-vstor-149847-2-doexec-pinned-memory-v1-0-36f1120f9b57@virtuozzo.com?part=6

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