Re: [PATCH] mm: memcontrol: don't miss processes whose leader has exited in OOM scans

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] mm: memcontrol: use-after-free on group_leader in mem_cgroup_scan_skip()
- [High] mm: memcontrol: duplicate or missed process scan in mem_cgroup_scan_skip()
- [High] mm: memcontrol: mem_cgroup_scan_skip() skips exiting OOM victims
--

commit dc2989b6546fee5aa393d439ca588a7e5bf50831
Author: Zhan Xusheng <zhanxusheng@xiaomi.com>

mm: memcontrol: don't miss processes whose leader has exited in OOM scans

This commit changes mem_cgroup_scan_tasks() to iterate over all tasks in a
memory cgroup without CSS_TASK_ITER_PROCS and adds mem_cgroup_scan_skip() to
pick either the thread group leader or the first non-exiting thread when the
leader is exiting.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4d00748c8a5b8..8823c549b6af6 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1427,6 +1427,36 @@ static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
>  						dead_memcg);
>  }
>  
> +/*
> + * Take one task per thread group: the leader, or the first live thread once
> + * the leader is exiting, since migrating the group leaves an exiting leader
> + * behind.  Exiting threads are skipped, as one left behind may belong to a
> + * group that has moved out.
> + */
> +static bool mem_cgroup_scan_skip(struct task_struct *task)
> +{
> +	struct task_struct *leader = task->group_leader;
> +	struct task_struct *t;
> +	bool skip = true;
> +
> +	if (READ_ONCE(task->flags) & PF_EXITING)
> +		return true;

[Severity: High]
Won't unconditionally skipping every task with PF_EXITING set hide in-flight
OOM victims from concurrent OOM scans before they finish releasing their
memory?

When a process is killed by the OOM killer via __oom_kill_process(),
mark_oom_victim() sets task->signal->oom_mm. In do_exit(), each dying thread
sets PF_EXITING in exit_signals() and decrements signal->live before reaching
exit_mm() -> mmput() -> __mmput() -> exit_mmap(), where the address space is
unmapped and MMF_OOM_SKIP is finally set:

kernel/exit.c:do_exit() {
	exit_signals(tsk);  /* sets PF_EXITING */

	seccomp_filter_release(tsk);

	acct_update_integrals(tsk);
	group_dead = atomic_dec_and_test(&tsk->signal->live);
	...
	exit_mm();
}

In mm/oom_kill.c, oom_evaluate_task() relies on mem_cgroup_scan_tasks()
visiting an OOM victim until MMF_OOM_SKIP is set so that concurrent OOM scans
abort rather than killing another process:

mm/oom_kill.c:oom_evaluate_task() {
	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
		if (mm_flags_test(MMF_OOM_SKIP, task->signal->oom_mm))
			goto next;
		goto abort;
	}
}

Once all threads of an OOM victim have passed exit_signals() and set
PF_EXITING (and with css_task_iter_start() called without
CSS_TASK_ITER_WITH_DEAD once signal->live reaches zero),
mem_cgroup_scan_skip() skips every thread of the victim before exit_mmap()
sets MMF_OOM_SKIP. Could this cause select_bad_process() in a concurrent memcg
OOM scan to miss the exiting victim and kill additional processes
unnecessarily?

> +	if (task == leader)
> +		return false;
> +	if (!(READ_ONCE(leader->flags) & PF_EXITING))
> +		return true;
> +
> +	rcu_read_lock();
> +	for_each_thread(leader, t) {

[Severity: Critical]
Can this trigger a use-after-free on leader when dereferencing leader->flags
and leader->signal?

In mem_cgroup_scan_tasks(), css_task_iter_next() acquires a reference only on
task via tryget_task_struct() and drops css_set_lock before returning task in
preemptible context. Holding a reference on a non-leader thread pins task and
task->signal, but does not pin task->group_leader.

Here, mem_cgroup_scan_skip() reads leader = task->group_leader without
rcu_read_lock() or READ_ONCE(), dereferences READ_ONCE(leader->flags) outside
rcu_read_lock(), and then passes that earlier leader pointer to
for_each_thread(leader, t), which expands to __for_each_thread(leader->signal,
t).

Two races can free leader after task->group_leader is read and before
rcu_read_lock() is acquired:

1. If the non-leader thread task calls execve(), task does not set PF_EXITING,
and de_thread() replaces group_leader and releases the old leader:

fs/exec.c:de_thread() {
		tsk->group_leader = tsk;
		leader->group_leader = tsk;
		...
		write_unlock_irq(&tasklist_lock);
		cgroup_threadgroup_change_end(tsk);

		release_task(leader);
}

2. If leader is already in EXIT_ZOMBIE and task exits after passing the
PF_EXITING check in mem_cgroup_scan_skip(), release_task(task) unhashes task
from the thread group and reaps leader via put_task_struct_rcu_user(leader)
even while css_task_iter still holds a usage reference on task:

kernel/exit.c:release_task() {
	leader = p->group_leader;
	if (leader != p && thread_group_empty(leader)
			&& leader->exit_state == EXIT_ZOMBIE) {
		...
		zap_leader = do_notify_parent(leader, leader->exit_signal);
		if (zap_leader)
			leader->exit_state = EXIT_DEAD;
	}
	...
	put_task_struct_rcu_user(p);

	p = leader;
	if (unlikely(zap_leader))
		goto repeat;
}

If mem_cgroup_scan_skip() is preempted before rcu_read_lock() and an RCU grace
period elapses, leader is freed before leader->flags and leader->signal are
dereferenced. Should rcu_read_lock() cover the READ_ONCE(task->group_leader)
load and all dereferences of leader?

> +		if (!(READ_ONCE(t->flags) & PF_EXITING)) {
> +			skip = t != task;
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();
> +	return skip;
> +}
> +
>  /**
>   * mem_cgroup_scan_tasks - iterate over tasks of a memory cgroup hierarchy
>   * @memcg: hierarchy root
> @@ -1452,9 +1482,10 @@ void mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
>  		struct css_task_iter it;
>  		struct task_struct *task;
>  
> -		css_task_iter_start(&iter->css, CSS_TASK_ITER_PROCS, &it);
> +		css_task_iter_start(&iter->css, 0, &it);
>  		while (!ret && (task = css_task_iter_next(&it))) {
> -			ret = fn(task, arg);
> +			if (!mem_cgroup_scan_skip(task))
> +				ret = fn(task, arg);
>  			/* Avoid potential softlockup warning */
>  			cond_resched();
>  		}

[Severity: High]
Because mem_cgroup_scan_skip() recomputes which thread represents the process
as each thread is visited across cond_resched(), can a multi-threaded process
be evaluated multiple times or missed entirely during a single scan?

1. Duplicate evaluation and repeated OOM kills: Suppose a process has threads
[leader, T1, T2] and css_task_iter visits leader first. mem_cgroup_scan_skip()
returns false and invokes fn(leader, arg). When fn is oom_kill_memcg_member()
(for memory.oom.group), it calls __oom_kill_process() and returns 0 to
continue scanning:

mm/oom_kill.c:oom_kill_memcg_member() {
	if (task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN &&
	    !is_global_init(task)) {
		get_task_struct(task);
		__oom_kill_process(task, message);
	}
	return 0;
}

If leader enters do_exit() and sets PF_EXITING before T1 sets PF_EXITING (or
if the first visited thread exits concurrently during oom_evaluate_task() or
dump_task()), css_task_iter_next() next visits T1. At that point
mem_cgroup_scan_skip(T1) sees leader has PF_EXITING set, finds T1 as the first
non-PF_EXITING thread in for_each_thread(leader, t), and returns false again
for the same process, causing oom_kill_memcg_member() to call
__oom_kill_process() repeatedly on sibling threads.

2. Missed process when iteration order differs from signal->thread_head:
css_task_iter walks css_sets and cset->tasks in cgroup attachment order, which
can differ from signal->thread_head order (for example, when threads are
distributed across threaded child cgroups under the same memory domain or
created with CLONE_INTO_CGROUP). If css_task_iter visits T2 before T1 while T1
is earlier in signal->thread_head, mem_cgroup_scan_skip(T2) skips T2 because
T1 is still alive; if T1 then sets PF_EXITING before css_task_iter reaches T1,
T1 is also skipped and the process is missed completely.

3. Missed process when threads are split across memcgs in cgroup v1: If the
leader or first non-PF_EXITING thread in signal->thread_head resides in
another memcg while sibling threads reside in the memcg being scanned,
mem_cgroup_scan_skip() will return true for every thread in this memcg because
the first non-PF_EXITING thread in signal->thread_head is never one of the
tasks in this memcg.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026120248.2188024-1-zhanxusheng1024@gmail.com?part=1