Re: [PATCH v2] 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 2 potential issue(s) to consider:
- [High] mm: memcontrol: missing CSS_TASK_ITER_WITH_DEAD in mem_cgroup_scan_tasks()
- [Medium] mm: memcontrol: mem_cgroup_scan_skip() exposes left-behind exiting threads
--

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

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

This patch walks all tasks in mem_cgroup_scan_tasks() instead of using
CSS_TASK_ITER_PROCS, adding mem_cgroup_scan_skip() to select one
representative task per thread group in the scanned memcg so that thread
groups migrated after their leader exited are still visible to OOM scans.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4d00748c8a5b8..cdb8ca4d4625d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1427,6 +1427,38 @@ static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
>  						dead_memcg);
>  }
>  
> +/*
> + * Let one task of each thread group in @memcg stand for the group: the
> + * first one that is not exiting, or, once the whole group is exiting, the
> + * first one that has not exited yet, so that an OOM victim stays visible.
> + * The leader is not enough: a group whose leader has exited can be moved
> + * without it.  Exiting threads left behind in @memcg do not count while the
> + * group has live threads elsewhere.
> + */
> +static bool mem_cgroup_scan_skip(struct mem_cgroup *memcg,
> +				 struct task_struct *task)
> +{
> +	struct task_struct *t, *dying = NULL;
> +	bool live_elsewhere = false;
> +
> +	guard(rcu)();
> +	__for_each_thread(task->signal, t) {
> +		bool here;
> +
> +		if (READ_ONCE(t->exit_state))
> +			continue;
> +		here = mem_cgroup_from_task(t) == memcg;
> +		if (!(READ_ONCE(t->flags) & PF_EXITING)) {
> +			if (here)
> +				return t != task;
> +			live_elsewhere = true;
> +		} else if (here && !dying) {
> +			dying = t;
> +		}
> +	}
> +	return live_elsewhere || dying != task;
> +}

[Severity: Medium]
Can mem_cgroup_scan_skip() still expose a left-behind exiting thread in the
source memcg once the migrated threads in the destination memcg also begin
exiting?

In kernel/cgroup/cgroup.c, cgroup_migrate_add_task() skips any thread with
PF_EXITING set:

	/* @task either already exited or can't exit until the end */
	if (task->flags & PF_EXITING)
		return;

Consider a thread group in memcg_A where thread T1 enters do_exit(), sets
PF_EXITING in exit_signals(), clears T1->mm in exit_mm(), and has not yet
reached exit_notify() (so T1->exit_state == 0 and T1 remains on memcg_A's
cset->tasks).

If the process is then migrated to memcg_B via cgroup.procs, T1 is skipped and
left behind in memcg_A while live thread T2 moves to memcg_B.

If an OOM scan in memcg_A advances its iterator to T1 while signal->live > 0,
and T2 in memcg_B then enters do_exit() and sets PF_EXITING before completing
exit_mm(), mem_cgroup_scan_skip(memcg_A, T1) will see PF_EXITING on all
threads. Because live_elsewhere only tracks threads without PF_EXITING,
live_elsewhere remains false and dying is set to T1, so
mem_cgroup_scan_skip(memcg_A, T1) returns false.

Wouldn't that expose the migrated thread group to memcg_A's OOM scan in
oom_evaluate_task(), either aborting memcg_A's scan if T2 was marked an OOM
victim in memcg_B (via tsk_is_oom_victim(T1) checking T1->signal->oom_mm) or
reaching T2's mm via find_lock_task_mm(T1) and killing the process in memcg_B?

> +
>  /**
>   * mem_cgroup_scan_tasks - iterate over tasks of a memory cgroup hierarchy
>   * @memcg: hierarchy root
> @@ -1452,9 +1484,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(iter, task))
> +				ret = fn(task, arg);

[Severity: High]
Should css_task_iter_start() pass CSS_TASK_ITER_WITH_DEAD here so that the
dying-group fallback in mem_cgroup_scan_skip() can actually see exiting OOM
victims?

In kernel/exit.c:do_exit(), a thread sets PF_EXITING and almost immediately
decrements tsk->signal->live before tearing down its address space in
exit_mm():

	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();

However, in kernel/cgroup/cgroup.c:css_task_iter_advance(), when
CSS_TASK_ITER_WITH_DEAD is not set in it->flags, any PF_EXITING task whose
signal->live has reached 0 is skipped by the iterator itself:

	if (!(it->flags & CSS_TASK_ITER_WITH_DEAD) &&
	    (task->flags & PF_EXITING) && !atomic_read(&task->signal->live))
		goto repeat;

When a concurrent memcg OOM occurs while an OOM victim is unmapping its memory
in exit_mm() before setting MMF_OOM_SKIP, css_task_iter_next() will skip all
threads of the victim group because flags is 0.

Doesn't this prevent mem_cgroup_scan_skip() and oom_evaluate_task() from ever
seeing the exiting OOM victim during exit_mm(), causing out_of_memory() ->
select_bad_process() -> mem_cgroup_scan_tasks() to kill additional processes
instead of aborting the OOM scan?

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