memcg: avoid charging the root memcg from obj_cgroup_charge_pages()
Subsystems (4)
Shakeel Butt
maintainerPATCH 1/1 memcg: avoid charging the root memcg from obj_cgroup_charge_pages()
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.
---
Note: I kept it simple for easy backports. I will followup with more cleanups where I will remove try_charge().
mm/memcontrol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +1 -1
@@ -3157,7 +3157,7 @@ static int obj_cgroup_charge_pages(struct obj_cgroup *objcg, gfp_t gfp, memcg =get_mem_cgroup_from_objcg (objcg); - ret =try_charge_memcg (memcg, gfp, nr_pages); + ret =try_charge (memcg, gfp, nr_pages);if (ret) goto out;
Signature · 1 line
2.53.0-Meta
Quoted 51 lines · Shakeel Butt
obj_cgroup_charge_pages()resolves the objcg to its memcg and callstry_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, whilememcg_reparent_objcgs()does redirectobjcg->memcgto the parent on rmdir. An objcg of a dying child of root therefore passes everyobj_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/0x30Beyond the warning, the charge is asymmetric:
obj_cgroup_uncharge_pages()skipsrefill_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 withobj_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=yVM. 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 selfteststest_zswap,test_kmemandtest_memcontrolshow no new failures.
Quoted 5 lines
Fixes 20d6c1725228 ("memcg: avoid refill_stock for root memcg")Reported-by Farhad Alemi <[email protected]>Closes https://lore.kernel.org/all/CA+0ovCgWzUMK+nNbbtH7eV65Ca=fDN4Ozu7iASgryjvv8Tk8zQ@mail.gmail.com/ open hereSigned-off-by Shakeel Butt <[email protected]>
LGTM.
Thanks.
Quoted 47 lines · Shakeel Butt
obj_cgroup_charge_pages()resolves the objcg to its memcg and callstry_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, whilememcg_reparent_objcgs()does redirectobjcg->memcgto the parent on rmdir. An objcg of a dying child of root therefore passes everyobj_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/0x30Beyond the warning, the charge is asymmetric:
obj_cgroup_uncharge_pages()skipsrefill_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 withobj_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=yVM. 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 selfteststest_zswap,test_kmemandtest_memcontrolshow no new failures.
Quoted 5 lines
Fixes 20d6c1725228 ("memcg: avoid refill_stock for root memcg")Reported-by Farhad Alemi <[email protected]>Closes https://lore.kernel.org/all/CA+0ovCgWzUMK+nNbbtH7eV65Ca=fDN4Ozu7iASgryjvv8Tk8zQ@mail.gmail.com/ open hereSigned-off-by Shakeel Butt <[email protected]>
The patched kernel (this patch on 28924df2a08f) no longer panics on the attached reproducer.
Quoted 49 lines · Johannes Weiner
Quoted 47 lines · Shakeel Butt
obj_cgroup_charge_pages()resolves the objcg to its memcg and callstry_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, whilememcg_reparent_objcgs()does redirectobjcg->memcgto the parent on rmdir. An objcg of a dying child of root therefore passes everyobj_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/0x30Beyond the warning, the charge is asymmetric:
obj_cgroup_uncharge_pages()skipsrefill_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 withobj_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=yVM. 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 selfteststest_zswap,test_kmemandtest_memcontrolshow no new failures.
Quoted 7 lines
Quoted 5 lines
Fixes 20d6c1725228 ("memcg: avoid refill_stock for root memcg")Reported-by Farhad Alemi <[email protected]>Closes https://lore.kernel.org/all/CA+0ovCgWzUMK+nNbbtH7eV65Ca=fDN4Ozu7iASgryjvv8Tk8zQ@mail.gmail.com/ open hereSigned-off-by Shakeel Butt <[email protected]>Reviewed-by Johannes Weiner <[email protected]>
/*
* 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;
}