Re: [RFC PATCH v25 07/14] mm: keep the RSS counters of single-threaded processes exact

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mm: clear mm->rss_owner on fork failure in dup_mm() and
  copy_process()
--

commit 388976e575ede4fc50ab3cc88ddde7c67d80cb2e
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

mm: keep the RSS counters of single-threaded processes exact

This commit initializes the RSS counter tree lazily when a second thread
shares the mm, using a plain owner counter and an atomic counter for
single-threaded processes.

> diff --git a/kernel/fork.c b/kernel/fork.c
> index d9bdaf2bd9984..dc1db6cb5da04 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c

[ ... ]

> @@ -1550,6 +1550,8 @@ static struct mm_struct *dup_mm(struct task_struct *tsk,
>  		mmap_write_unlock(oldmm);
>  		goto fail_uprobe;
>  	}
> +	/* The child owns its RSS counters, see __add_mm_counter(). */
> +	mm->rss_owner = tsk;
>  
>  	err = dup_mmap(mm, oldmm);
>  	if (err)

[Severity: Medium]
Can a failed fork leave mm->rss_owner pointing to a freed task_struct?

Once dup_mmap() succeeds, the new mm is reachable by external
mmget_not_zero() callers such as userfaultfd or swapoff.  If the fork then
fails in dup_mm() at try_module_get() or later in copy_process() (for
example when put_user(pidfd, args->pidfd) returns -EFAULT), both error
paths clear mm->owner before mmput(), but neither clears mm->rss_owner:

kernel/fork.c:dup_mm() {
	...
free_pt:
	/* don't put binfmt in mmput, we haven't got module yet */
	mm->binfmt = NULL;
	mm_init_owner(mm, NULL);
	mmput(mm);
	...
}

kernel/fork.c:copy_process() {
	...
bad_fork_cleanup_mm:
	sched_cache_fork_cleanup(p);
	if (p->mm) {
		mm_clear_owner(p->mm, p);
		mmput(p->mm);
	}
	...
}

If an external reference holds mm->mm_users > 1 across that mmput(), mm
outlives tsk.  A new task allocated at the same slab address as tsk can
then match current == READ_ONCE(mm->rss_owner) in __add_mm_counter() and
perform unsynchronized non-atomic updates via
percpu_counter_tree_add_owner(), losing counter updates or triggering a
Bad rss-counter state report in check_mm().

Clearing mm->rss_owner alongside mm->owner on both error paths before
mmput() would avoid leaving the dangling pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111203626.95838-1-mathieu.desnoyers@efficios.com?part=7