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

[no-reply at sashiko.virtuozzo.com]

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 at 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?

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?

[ ... ]

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