Re: [Devel] [PATCH hci-8.1 7/8] exec: protect preserved memory against surviving shared-mm tasks

[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 2 potential issue(s) to consider:
> - [Low] fork: copy_mm()/copy_process() take page_table_lock twice per shared-mm clone
> - [Low] exec: VM_EXEC_KEEP scan in reserve_preserved_mem() is stale by exec_mmap()
> --
> 
> commit 3fae5b8a82902f91e53a5e8e729322cb7b034228
> Author: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> 
> exec: protect preserved memory against surviving shared-mm tasks
> 
> This patch makes an exec that accepts preserved memory fail with EBUSY
> when a task outside the thread group still uses the mm and the mm has a
> VM_EXEC_KEEP VMA. copy_mm() refuses and counts CLONE_VM, non-thread clones
> while the mm is marked. exec_mmap() takes the preserve path only when the
> mm was marked.
> 
> 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
> 
> [ ... ]
> 
>> +static int reserve_preserved_mem(struct linux_binprm *bprm)
>> +{
>> +	struct mm_struct *mm = current->mm;
> 
> [ ... ]
> 
>> +	mmap_read_unlock(mm);
>> +	if (!preserve)
>> +		return 0;
> 
> [Severity: Low]
> Can the result of this scan be stale by the time exec_mmap() runs?


  Yes, a range another task marks after the scan is not preserved. I
  think that is acceptable:

  - A sibling thread is killed by de_thread() right after. Its madvise()
    races with the execve() in userspace either way; the scan is where
    the exec decides, and a mark that comes later counts as one made
    after the exec.
  - A CLONE_VM task keeps the old mm, marks included, so it loses nothing.

  Nothing is left half done either: without preserving_mem, exec_mmap()
  takes the normal path and the marked VMAs go away with the old mm, as
  on any exec.

> 
> When no VM_EXEC_KEEP VMA is found here, nothing is reserved:
> exec_keep_active and bprm->preserving_mem both stay unset.  Sibling
> threads have not yet been removed by de_thread(), and copy_mm() only
> refuses clones while exec_keep_active is set.  So a sibling thread or a
> CLONE_VM task can still call madvise(MADV_DOEXEC) after mmap_read_unlock()
> and set VM_EXEC_KEEP on a range.
> 
>> @@ -1040,7 +1112,7 @@ static int exec_mmap(struct linux_binprm *bprm)
>>  			up_write(&tsk->signal->exec_update_lock);
>>  			return ret;
>>  		}
>> -		if (unlikely(bprm->accepts_preserved_mem)) {
>> +		if (unlikely(bprm->preserving_mem)) {
> 
> Here the preserve path (write lock plus vma_dup_some()) is now selected by
> the early sample rather than by the VMAs present under the lock at copy
> time.  A VM_EXEC_KEEP VMA created after the scan in
> reserve_preserved_mem() would not be copied, and the range would be lost
> with the old mm without any error.
> 
> Before this patch, accepts_preserved_mem alone selected the path, and
> vma_dup_some() copied any VM_EXEC_KEEP VMA present under
> mmap_write_lock.  Should the decision be revalidated, or the mark taken
> before the scan, so that a range marked during begin_new_exec() is not
> silently dropped?
> 
> [ ... ]
> 
>> diff --git a/kernel/fork.c b/kernel/fork.c
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
> 
> [ ... ]
> 
>> +/* Keep unpublished CLONE_VM tasks visible to preserving-exec admission. */
>> +static int exec_keep_clone_begin(struct mm_struct *mm)
>> +{
>> +	int ret = 0;
>> +
>> +	spin_lock(&mm->page_table_lock);
>> +	if (mm->exec_keep_active)
>> +		ret = -EBUSY;
>> +	else
>> +		mm->exec_keep_clones++;
>> +	spin_unlock(&mm->page_table_lock);
>> +	return ret;
>> +}
>> +
>> +static void exec_keep_clone_end(struct mm_struct *mm)
>> +{
>> +	spin_lock(&mm->page_table_lock);
>> +	if (!WARN_ON_ONCE(!mm->exec_keep_clones))
>> +		mm->exec_keep_clones--;
>> +	spin_unlock(&mm->page_table_lock);
>> +}
> 
> [Severity: Low]
> This isn't a correctness problem, but is the cost of this accounting
> intended for every spawn?
> 
> exec_keep_clone_begin() in copy_mm() and exec_keep_clone_end() in
> copy_process() each take and release the parent's mm->page_table_lock.
> This happens on every CLONE_VM clone that is not a thread (vfork(),
> posix_spawn(), clone(CLONE_VM)), whether or not the mm has any
> VM_EXEC_KEEP VMA and whether or not an exec is in progress.
> 
> page_table_lock also protects page table operations, so a multithreaded
> parent taking page faults can contend with it.
> 
> Would an atomic counter, or skipping the accounting until the mm has ever
> had MADV_DOEXEC applied, avoid the two lock round trips per spawn?


  Only CLONE_VM clones that are not threads pay them, vfork() and
  posix_spawn() among them, and two uncontended round trips are small
  next to the rest of such a clone. A flag set by the first MADV_DOEXEC
  would remove them; I would leave that for a follow-up if it ever shows
  in a profile.

> 
> [ ... ]
> 

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