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

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

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

mem_cgroup_scan_tasks() walks with CSS_TASK_ITER_PROCS, so it sees only
thread group leaders, plus a dying leader that still has live threads.
Moving a process whose leader has exited, by writing its PID to
cgroup.procs, moves the live threads but leaves the leader behind, as
cgroup_migrate_add_task() skips exiting tasks.

The destination's OOM killer then cannot see the process.  If it is
alone there, every attempt ends with "Out of memory and no killable
processes..." and the charge is retried indefinitely, and
memory.oom.group kills nothing; otherwise another process is killed in
its place.  The source's OOM killer still finds the leader, reaches the
moved threads' mm through find_lock_task_mm(), and kills a process that
has left the memcg.

Walk all tasks and let one task of each thread group in the memcg stand
for it: the first one that is not exiting, or the first one that has not
exited yet once the whole group is exiting, so that an OOM victim stays
visible until it is gone, as the leader does today.  Exiting threads left
behind do not count while the group has live threads elsewhere.

Tested in a VM with memory.max=64M: in each case above the process using
the memory is now killed, and a process whose exited leader was not
moved, or whose leader is alive, is killed as before, with and without
memory.oom.group.

Fixes: f168a9a54ec3 ("mm: memcontrol: use CSS_TASK_ITER_PROCS at mem_cgroup_scan_tasks()")
Cc: stable@vger.kernel.org
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
v2:
 - Do the whole check under RCU and walk the group through task->signal;
   v1 read task->group_leader, which a reference on a thread does not pin
 - Keep exiting threads visible while the whole group is exiting, so a
   concurrent scan still finds an OOM victim and aborts
 - Pick the representative among the tasks in the scanned memcg

Tao's Reviewed-by and Tested-by are not carried over, as the check
changed.

The walk visits every thread again, as it did before f168a9a54ec3, but
the check stops at the first thread of a group whose first thread is
live and in the scanned memcg, which is the usual case.  A group can
still be visited twice, or not at all, if the task standing for it
finishes exiting during the scan.

v1: https://lore.kernel.org/all/20261026120248.2188024-1-zhanxusheng1024@gmail.com/

 mm/memcontrol.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d00748c8a5b..cdb8ca4d4625 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;
+}
+
 /**
  * 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);
 			/* Avoid potential softlockup warning */
 			cond_resched();
 		}

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.43.0

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

[sashiko-bot]

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