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

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 10/14/26 19:24, no-reply@sashiko.virtuozzo.com wrote:
> 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?

  No. The old VMA is write-locked before the copy and stays so through
  the zap.

  vma_dup() clears VM_ACCOUNT on the source with
  vm_flags_clear(old_vma, VM_ACCOUNT) before copy_page_range_mt(), and
  vm_flags_clear() starts with vma_start_write(). That waits for the
  per-VMA readers already inside, such as uffd_move_lock() or
  uffd_lock_vma(), and makes lock_vma_under_rcu() fail for new ones until
  the mmap lock is dropped. exec_mmap() keeps old_mm write-locked from
  vma_dup_some() until after zap_vma_mt(), so the lock taken in vma_dup()
  still covers the zap.

  An UFFDIO_MOVE from a task that holds the uffd therefore falls back to
  mmap_read_lock() and waits for the exec. Whatever it does to old_mm
  afterwards no longer reaches the preserved pages: they are zapped by
  then.

  The write lock is a side effect of vm_flags_clear(), though. "mm: allow
  MADV_DOEXEC on shared PFN mappings of device memory" makes that clear
  conditional and adds an explicit vma_start_write(old_vma) for this
  reason.

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

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