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

[PATCH] 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 take one per thread group: the leader, or the first
live thread once the leader is exiting.  Skip exiting threads, which may
belong to a group that has moved out.

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>
---
 mm/memcontrol.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d00748c8a5b..8823c549b6af 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;
+	if (task == leader)
+		return false;
+	if (!(READ_ONCE(leader->flags) & PF_EXITING))
+		return true;
+
+	rcu_read_lock();
+	for_each_thread(leader, t) {
+		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();
 		}

base-commit: 6b4b543fa18757297ac756a6fe6e64f506b7844a
-- 
2.43.0

[PATCH] selftests: cgroup: test memcg OOM on a process whose leader has exited

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

Moving a process whose thread group leader has exited leaves the leader
behind, so its live threads sit in the destination memcg with nothing
listed in cgroup.procs.  Check that an OOM there still kills the process.

Without "mm: memcontrol: don't miss processes whose leader has exited in
OOM scans" no task is selected and the test fails after five seconds.

Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 .../selftests/cgroup/test_memcontrol.c        | 123 ++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index bb673012dcf0..bddbe923fa42 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -4,6 +4,8 @@
 #include <linux/limits.h>
 #include <linux/oom.h>
 #include <fcntl.h>
+#include <sched.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -18,6 +20,7 @@
 #include <netdb.h>
 #include <errno.h>
 #include <sys/mman.h>
+#include <sys/syscall.h>
 #include <sys/sysinfo.h>
 
 #include "kselftest.h"
@@ -1417,6 +1420,125 @@ static bool memcg_sockets_disabled(void)
  * The TCP client interleaves reads from the socket with check whether
  * memory.current and memory.stat.sock are similar.
  */
+static int exited_leader_pipe[2];
+
+static int exited_leader_thread(void *arg)
+{
+	char c;
+
+	/* Wait until the process has been moved, then fill memory. */
+	if (read(exited_leader_pipe[0], &c, 1) == 1)
+		alloc_anon(NULL, (void *)MB(100));
+	for (;;)
+		pause();
+	return 0;
+}
+
+/* Start a thread and exit only the thread group leader. */
+static int exited_leader_fn(const char *cgroup, void *arg)
+{
+	const size_t stack_size = 64 * 1024;
+	char *stack = malloc(stack_size);
+
+	if (!stack)
+		return EXIT_FAILURE;
+
+	if (clone(exited_leader_thread, stack + stack_size,
+		  CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND |
+		  CLONE_THREAD | CLONE_SYSVSEM, NULL) < 0)
+		return EXIT_FAILURE;
+
+	syscall(SYS_exit, 0);
+	return EXIT_FAILURE;
+}
+
+/*
+ * Moving a process whose leader has exited leaves the leader behind, so
+ * the destination's cgroup.procs is empty.  Check that the destination's
+ * OOM killer still finds the process using its memory.
+ */
+static int test_memcg_oom_exited_leader(const char *root)
+{
+	char *parent = NULL, *src = NULL, *dst = NULL;
+	int ret = KSFT_FAIL;
+	pid_t pid = -ESRCH;
+	int i;
+
+	if (pipe(exited_leader_pipe))
+		return KSFT_FAIL;
+
+	parent = cg_name(root, "memcg_test_exited_leader");
+	if (!parent)
+		goto cleanup;
+	src = cg_name(parent, "src");
+	if (!src)
+		goto cleanup;
+	dst = cg_name(parent, "dst");
+	if (!dst)
+		goto cleanup;
+
+	if (cg_create(parent))
+		goto cleanup;
+	if (cg_write(parent, "cgroup.subtree_control", "+memory"))
+		goto cleanup;
+	if (cg_create(src) || cg_create(dst))
+		goto cleanup;
+	if (cg_write(dst, "memory.max", "30M"))
+		goto cleanup;
+	if (cg_write(dst, "memory.swap.max", "0"))
+		goto cleanup;
+
+	pid = cg_run_nowait(src, exited_leader_fn, NULL);
+	if (pid < 0)
+		goto cleanup;
+
+	for (i = 0; i < 100; i++) {
+		if (!proc_read_strstr(pid, false, "stat", ") Z "))
+			break;
+		usleep(10000);
+	}
+	if (i == 100)
+		goto cleanup;
+
+	/* The exited leader stays in src, the live thread moves. */
+	if (cg_enter(dst, pid))
+		goto cleanup;
+	if (cg_read_strcmp(dst, "cgroup.procs", ""))
+		goto cleanup;
+
+	if (write(exited_leader_pipe[1], "x", 1) != 1)
+		goto cleanup;
+
+	if (cg_read_key_long_poll(dst, "memory.events", "oom_kill ", 1,
+				  50, 100000) != 1)
+		goto cleanup;
+
+	if (cg_read_strcmp_wait(dst, "cgroup.events", "populated 0\n"))
+		goto cleanup;
+
+	ret = KSFT_PASS;
+
+cleanup:
+	if (dst)
+		cg_write(dst, "cgroup.kill", "1");
+	if (pid > 0) {
+		kill(pid, SIGKILL);
+		waitpid(pid, NULL, 0);
+	}
+	close(exited_leader_pipe[0]);
+	close(exited_leader_pipe[1]);
+	if (dst)
+		cg_destroy(dst);
+	if (src)
+		cg_destroy(src);
+	if (parent)
+		cg_destroy(parent);
+	free(dst);
+	free(src);
+	free(parent);
+	return ret;
+}
+
 static int test_memcg_sock(const char *root)
 {
 	int bind_retries = 5, ret = KSFT_FAIL, pid, err;
@@ -1816,6 +1938,7 @@ struct memcg_test {
 	T(test_memcg_oom_group_leaf_events),
 	T(test_memcg_oom_group_parent_events),
 	T(test_memcg_oom_group_score_events),
+	T(test_memcg_oom_exited_leader),
 	T(test_memcg_inotify_delete_file),
 	T(test_memcg_inotify_delete_dir),
 };

base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
-- 
2.43.0

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

[sashiko-bot]

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

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

[Tao Cui]

Hi, Xusheng,

在 2026/10/26 20:02, 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 take one per thread group: the leader, or the first
> live thread once the leader is exiting.  Skip exiting threads, which may
> belong to a group that has moved out.
> 
> 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.
> 

Tested on next-20261026 (x86_64 KVM guest, cgroup v2, 48M memory.max
on the destination group). Without the patch the worker thread stays
alive and the OOM scan keeps printing "Out of memory and no killable
processes..." (2887 oom, 0 oom_kill in one minute). With the patch
the worker is killed right away. A live leader and an exited leader
that was not moved away are still killed as before.

Reviewed-by: Tao Cui <cuitao@kylinos.cn>
Tested-by: Tao Cui <cuitao@kylinos.cn>

Thanks,
Tao

> 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>
> ---
>  mm/memcontrol.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4d00748c8a5b..8823c549b6af 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;
> +	if (task == leader)
> +		return false;
> +	if (!(READ_ONCE(leader->flags) & PF_EXITING))
> +		return true;
> +
> +	rcu_read_lock();
> +	for_each_thread(leader, t) {
> +		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();
>  		}
> 
> base-commit: 6b4b543fa18757297ac756a6fe6e64f506b7844a