Re: [PATCH] memcg: avoid charging the root memcg from obj_cgroup_charge_pages()

[Farhad Alemi]

The patched kernel (this patch on 28924df2a08f) no longer panics on
the attached reproducer.


On Tue, Sep 1, 2026 at 9:47 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Fri, Aug 28, 2026 at 07:32:51PM -0700, Shakeel Butt wrote:
> > obj_cgroup_charge_pages() resolves the objcg to its memcg and calls
> > try_charge_memcg(), which does not short circuit the root memcg.  That
> > memcg can be the root memcg: obj_cgroup_is_root() reflects the memcg the
> > objcg was created for and is never updated, while memcg_reparent_objcgs()
> > does redirect objcg->memcg to the parent on rmdir.  An objcg of a dying
> > child of root therefore passes every obj_cgroup_is_root() filter but
> > resolves to the root memcg.
> >
> > Folios keep the objcg they were charged with, so this is easy to reach
> > through zswap: allocate anon memory in a cgroup, move the task out,
> > remove the cgroup, then write to the root cgroup's memory.reclaim.  The
> > reclaimed folios are charged through the reparented objcg and end up in
> > refill_stock() with the root memcg:
> >
> >   WARNING: mm/memcontrol.c:2198 at refill_stock+0x644/0x940
> >    refill_stock+0x644/0x940
> >    try_charge_memcg+0x12d6/0x1570
> >    __obj_cgroup_charge+0x35/0xf0
> >    obj_cgroup_charge+0x1de/0x210
> >    obj_cgroup_charge_zswap+0x83/0x270
> >    zswap_store+0x1620/0x2000
> >    swap_writeout+0x94c/0x14c0
> >    shrink_folio_list+0x3388/0x52b0
> >    [...]
> >    try_to_free_mem_cgroup_pages+0x30d/0x830
> >    user_proactive_reclaim+0x504/0x840
> >    memory_reclaim+0x1f/0x30
> >
> > Beyond the warning, the charge is asymmetric: obj_cgroup_uncharge_pages()
> > skips refill_stock() for the root memcg, so the root's page counter grows
> > and is never uncharged.  It is not user visible, since memory.current is
> > not exposed on the root, but it is a leak.
> >
> > Use try_charge(), which returns early for the root memcg, restoring the
> > symmetry with obj_cgroup_uncharge_pages().
> >
> > The above sequence was scripted into a standalone reproducer (zswap on,
> > swap on a virtio disk, 512MB of anon memory faulted in inside a child of
> > the root cgroup, the task then migrated to the root cgroup, the child
> > removed, followed by "echo 600M swappiness=max > memory.reclaim" on the
> > root) and run in a CONFIG_DEBUG_VM=y VM.  It reproduces the splat on the
> > first zswap store of a reparented folio, with the same call chain as the
> > report.  With this patch applied the splat is gone while the zswap store
> > count over the run is unchanged, so the same path is still exercised.
> > cgroup selftests test_zswap, test_kmem and test_memcontrol show no new
> > failures.
> >
> > Fixes: 20d6c1725228 ("memcg: avoid refill_stock for root memcg")
> > Reported-by: Farhad Alemi <farhad.alemi@xxxxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/CA+0ovCgWzUMK+nNbbtH7eV65Ca=fDN4Ozu7iASgryjvv8Tk8zQ@xxxxxxxxxxxxxx/
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
>
> Reviewed-by: Johannes Weiner <hannes@xxxxxxxxxxx>
/*
 * Reproducer for 145-memcg-vm_warn-refill_stock-root-cgroup
 */
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/wait.h>
#include <unistd.h>

#define CGROUP_ROOT "/sys/fs/cgroup"
#define VICTIM_CGROUP CGROUP_ROOT "/victim"
#define SWAPFILE  "/swapfile.145"
#define SWAP_SIZE (512ULL << 20)
#define ANON_SIZE (400ULL << 20)

static int write_file(const char *path, const char *value)
{
	int fd = open(path, O_WRONLY);
	int rc;

	if (fd < 0) {
		printf("open %s: %s\n", path, strerror(errno));
		return -1;
	}
	rc = write(fd, value, strlen(value));
	if (rc < 0)
		printf("write %s <- %s: %s\n", path, value, strerror(errno));
	close(fd);
	return rc < 0 ? -1 : 0;
}

/* swapon() refuses a sparse file, so the area is written out in full */
static int setup_swapfile(void)
{
	unsigned long page_size = 4096;
	unsigned char *header = calloc(1, page_size);
	unsigned char *zeros = calloc(1, 1 << 20);
	unsigned long long off;
	int fd = open(SWAPFILE, O_RDWR | O_CREAT | O_TRUNC, 0600);

	if (fd < 0)
		return -1;
	for (off = 0; off < SWAP_SIZE; off += (1 << 20))
		if (write(fd, zeros, 1 << 20) != (1 << 20))
			return -1;

	/* struct swap_header: 1024 bootbits, then version/last_page/nr_badpages,
	 * and the "SWAPSPACE2" magic in the last 10 bytes of the first page */
	*(unsigned int *)(header + 1024) = 1;
	*(unsigned int *)(header + 1028) = (SWAP_SIZE / page_size) - 1;
	memcpy(header + page_size - 10, "SWAPSPACE2", 10);
	if (pwrite(fd, header, page_size, 0) != (ssize_t)page_size)
		return -1;
	fsync(fd);
	close(fd);
	return syscall(SYS_swapon, SWAPFILE, 0);
}

int main(void)
{
	char pid_str[32];
	int charged_pipe[2], placed_pipe[2];
	pid_t child;
	char sync_byte;

	setvbuf(stdout, NULL, _IONBF, 0);

	/* zswap must be on, or nothing is charged to the objcg at all */
	write_file("/sys/module/zswap/parameters/enabled", "Y");
	if (setup_swapfile() < 0) {
		printf("swapon: %s\n", strerror(errno));
		return 1;
	}
	write_file(CGROUP_ROOT "/cgroup.subtree_control", "+memory");
	if (mkdir(VICTIM_CGROUP, 0755) < 0 && errno != EEXIST) {
		printf("mkdir %s: %s\n", VICTIM_CGROUP, strerror(errno));
		return 1;
	}

	pipe(charged_pipe);
	pipe(placed_pipe);

	child = fork();
	if (child == 0) {
		char *anon;
		unsigned long long i;

		close(charged_pipe[0]);
		close(placed_pipe[1]);
		read(placed_pipe[0], &sync_byte, 1);

		anon = mmap(NULL, ANON_SIZE, PROT_READ | PROT_WRITE,
			    MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
		if (anon == MAP_FAILED)
			_exit(1);
		/* compressible: zswap only charges what it actually stores */
		for (i = 0; i < ANON_SIZE; i += 4096) {
			memset(anon + i, 0x41 + (i % 7), 4096);
			*(unsigned long *)(anon + i) = i;
		}
		write(charged_pipe[1], "x", 1);
		pause();		/* keep the folios charged */
		_exit(0);
	}
	close(charged_pipe[1]);
	close(placed_pipe[0]);

	snprintf(pid_str, sizeof(pid_str), "%d", child);
	write_file(VICTIM_CGROUP "/cgroup.procs", pid_str);
	write(placed_pipe[1], "x", 1);
	read(charged_pipe[0], &sync_byte, 1);

	/* cgroup v2 does not recharge on migration, so the folios keep
	 * pointing at victim's objcg after the owner leaves */
	write_file(CGROUP_ROOT "/cgroup.procs", pid_str);

	/* rmdir reparents that objcg to the ROOT memcg while leaving
	 * objcg->is_root false, which is what defeats the
	 * obj_cgroup_is_root() guard in obj_cgroup_charge_zswap() */
	if (rmdir(VICTIM_CGROUP) < 0)
		printf("rmdir %s: %s\n", VICTIM_CGROUP, strerror(errno));

	/* reclaim on the ROOT: the zswap charge lands on root_mem_cgroup and
	 * refill_stock() trips VM_WARN_ON_ONCE(mem_cgroup_is_root(memcg)) */
	write_file(CGROUP_ROOT "/memory.reclaim", "1G");

	kill(child, SIGKILL);
	waitpid(child, NULL, 0);
	return 0;
}