mm: handle no memcg case in memcg_kmem_charge() properly

PATCH In review Roman Gushchin 4 participants 7 messages Oct 29, 2018 – Oct 30, 2018

Subsystems (2)

Memory management memcg

lore

Share & tools

lore

Roman Gushchin

author

2018-10-29 21:52 UTC #0 lore raw

PATCH 1/1 mm: handle no memcg case in memcg_kmem_charge() properly

Mike Galbraith reported a regression caused by the commit 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") on a system with "cgroup_disable=memory" boot option: the system panics with the following stack trace:

  [0.928542] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff 0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX: 0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI: 00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09: ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12: 0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15: ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000) knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4: 0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

The problem occurs because get_mem_cgroup_from_current() returns the NULL pointer if memory controller is disabled. Let's check if this is a case at the beginning of memcg_kmem_charge() and just return 0 if mem_cgroup_disabled() returns true. This is how we handle this case in many other places in the memory controller code.

Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")

Reported-by Mike Galbraith <[email protected]>

Signed-off-by Roman Gushchin <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Andrew Morton <[email protected]>

---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memcontrol.c +1 -1

index 54920cbc46bf..6e1469b80cb7 100644
@@ -2593,7 +2593,7 @@ int memcg_kmem_charge(struct page *page, gfp_t gfp, int order)
 	struct mem_cgroup *memcg;
 	int ret = 0;
 
-	if (memcg_kmem_bypass())
+	if (mem_cgroup_disabled() || memcg_kmem_bypass())
 		return 0;
 
 	memcg = get_mem_cgroup_from_current();

Signature · 1 line

2.17.2

Shakeel Butt

2018-10-30 00:50 UTC #1 lore raw

Quoted 61 lines · Roman Gushchin

Mike Galbraith reported a regression caused by the commit 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") on a system with "cgroup_disable=memory" boot option: the system panics with the following stack trace:
  [0.928542] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff 0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX: 0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI: 00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09: ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12: 0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15: ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000) knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4: 0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
The problem occurs because get_mem_cgroup_from_current() returns the NULL pointer if memory controller is disabled. Let's check if this is a case at the beginning of memcg_kmem_charge() and just return 0 if mem_cgroup_disabled() returns true. This is how we handle this case in many other places in the memory controller code.
Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Roman Gushchin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Vladimir Davydov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +0 -0
index 54920cbc46bf..6e1469b80cb7 100644

mm/memcontrol.c int memcg_kmem_charge(struct page *page, gfp_t gfp, int order) · line 2598

Quoted 7 lines

mm/memcontrol.c +1 -1

@@ -2593,7 +2593,7 @@ int memcg_kmem_charge(struct page *page, gfp_t gfp, int order)
        struct mem_cgroup *memcg;
        int ret = 0;
 
-       if (memcg_kmem_bypass())
+       if (mem_cgroup_disabled() || memcg_kmem_bypass())
                return 0;

Why not check memcg_kmem_enabled() before calling memcg_kmem_charge() in memcg_charge_kernel_stack()?

Quoted 4 lines

mm/memcontrol.c +0 -1

        memcg = get_mem_cgroup_from_current();
--

2.17.2

Rik van Riel

Acked-by

2018-10-30 01:01 UTC #2 lore raw

Quoted 80 lines · Shakeel Butt

Quoted 79 lines · Roman Gushchin

Mike Galbraith reported a regression caused by the commit 9b6f7e163cd0
("mm: rework memcg kernel stack accounting") on a system with "cgroup_disable=memory" boot option: the system panics with the following stack trace:

  [0.928542] BUG: unable to handle kernel NULL pointer dereference
at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-
preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff
0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX:
0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI:
00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09:
ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12:
0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15:
ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000)
knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4:
0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")

Reported-by Mike Galbraith <[email protected]>

Signed-off-by Roman Gushchin <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Andrew Morton <[email protected]>

---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memcontrol.c +0 -0

index 54920cbc46bf..6e1469b80cb7 100644
@@ -2593,7 +2593,7 @@ int memcg_kmem_charge(struct page *page,
 gfp_t gfp, int order)
        struct mem_cgroup *memcg;
        int ret = 0;

Quoted 7 lines

mm/memcontrol.c int memcg_kmem_charge(struct page *page, gfp_t gfp, int order) · line 2598
Quoted 4 lines
mm/memcontrol.c +1 -1
-       if (memcg_kmem_bypass())
+       if (mem_cgroup_disabled() || memcg_kmem_bypass())
                return 0;
 
Why not check memcg_kmem_enabled() before calling memcg_kmem_charge() in memcg_charge_kernel_stack()?

Check Roman's backtrace again. The function memcg_charge_kernel_stack() is not in it.

That is why it is generally better to check in the called function, rather than add a check to every call site (and maybe miss one or two).

Acked-by Rik van Riel <[email protected]>

Signature · 1 line

All Rights Reversed.

Shakeel Butt

2018-10-30 02:27 UTC #3 lore raw

Quoted 90 lines · Rik van Riel

Quoted 80 lines · Shakeel Butt
Quoted 79 lines · Roman Gushchin
Mike Galbraith reported a regression caused by the commit 9b6f7e163cd0
("mm: rework memcg kernel stack accounting") on a system with "cgroup_disable=memory" boot option: the system panics with the following stack trace:
  [0.928542] BUG: unable to handle kernel NULL pointer dereference
at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-
preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff
0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX:
0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI:
00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09:
ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12:
0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15:
ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000)
knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4:
0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
The problem occurs because get_mem_cgroup_from_current() returns the NULL pointer if memory controller is disabled. Let's check if this is a case at the beginning of memcg_kmem_charge() and just return 0 if mem_cgroup_disabled() returns true. This is how we handle this case in many other places in the memory controller code.
Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Roman Gushchin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Vladimir Davydov <[email protected]>
Cc Andrew Morton <[email protected]>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +0 -0
index 54920cbc46bf..6e1469b80cb7 100644
@@ -2593,7 +2593,7 @@ int memcg_kmem_charge(struct page *page,
 gfp_t gfp, int order)
        struct mem_cgroup *memcg;
        int ret = 0;
 
Quoted 7 lines
mm/memcontrol.c int memcg_kmem_charge(struct page *page, gfp_t gfp, int order) · line 2598
Quoted 4 lines
mm/memcontrol.c +1 -1
-       if (memcg_kmem_bypass())
+       if (mem_cgroup_disabled() || memcg_kmem_bypass())
                return 0;
 
Why not check memcg_kmem_enabled() before calling memcg_kmem_charge() in memcg_charge_kernel_stack()?

Quoted 3 lines

Check Roman's backtrace again. The function memcg_charge_kernel_stack() is not in it.

It got inlined.

Quoted 5 lines

That is why it is generally better to check in the called function, rather than add a check to every call site (and maybe miss one or two).

I think the reason the check was at the call site was not to introduce jmp/call in the allocation hot path for processes in the root memcg. I don't have any strong preference but we should be persistent i.e. checks at call site for all or check in the called function for all.

Shakeel

Michal Hocko

maintainer Acked-by on v1 1/1

2018-10-30 06:12 UTC #4 lore raw

In reply to Roman Gushchin

Quoted 44 lines · Roman Gushchin

  [0.928542] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff 0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX: 0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI: 00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09: ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12: 0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15: ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000) knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4: 0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Quoted 7 lines

Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Roman Gushchin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Vladimir Davydov <[email protected]>
Cc Andrew Morton <[email protected]>

I tend to agree with Shakeel that consistency with the other caller would be less confusing. I would split the function to __memcg_kmem_charge without any checks and call it from __alloc_pages_nodemask and add the check to memcg_kmem_charge. This would be less confusing I guess.

Something for a follow up clean up though.

Acked-by Michal Hocko <[email protected]>

Quoted 19 lines

---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/memcontrol.c +1 -1

index 54920cbc46bf..6e1469b80cb7 100644
@@ -2593,7 +2593,7 @@ int memcg_kmem_charge(struct page *page, gfp_t gfp, int order)
 	struct mem_cgroup *memcg;
 	int ret = 0;
 
-	if (memcg_kmem_bypass())
+	if (mem_cgroup_disabled() || memcg_kmem_bypass())
 		return 0;
 
 	memcg = get_mem_cgroup_from_current();

Signature · 1 line

2.17.2

Signature · 2 lines

Michal Hocko
SUSE Labs

Roman Gushchin

author

2018-10-30 15:56 UTC #5 lore raw

Quoted 53 lines · Michal Hocko

Quoted 44 lines · Roman Gushchin
Mike Galbraith reported a regression caused by the commit 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") on a system with "cgroup_disable=memory" boot option: the system panics with the following stack trace:
  [0.928542] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff 0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX: 0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI: 00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09: ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12: 0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15: ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000) knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4: 0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
The problem occurs because get_mem_cgroup_from_current() returns the NULL pointer if memory controller is disabled. Let's check if this is a case at the beginning of memcg_kmem_charge() and just return 0 if mem_cgroup_disabled() returns true. This is how we handle this case in many other places in the memory controller code.
Quoted 7 lines
Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Roman Gushchin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Vladimir Davydov <[email protected]>
Cc Andrew Morton <[email protected]>

Quoted 2 lines

I tend to agree with Shakeel that consistency with the other caller would be less confusing.

I totally agree that consistency is a thing here (and everywhere), however using memcg_kmem_enabled() here is not consistent at all. memcg_kmem_enabled() is tight to the slab allocation accounting, but here we have a different type of allocation: we actually charge an area preallocated using vmalloc.

Quoted 4 lines

I would split the function to __memcg_kmem_charge
without any checks and call it from __alloc_pages_nodemask and add the check to memcg_kmem_charge. This would be less confusing I guess. Something for a follow up clean up though.

Sure. Alternatively, we can check the pointer returned by get_mem_cgroup_from_current() in memcg_kmem_charge().

Anyway, let's postpone this clean up a bit, now the main goal is to fix the panic.

Quoted 1 line

Acked-by Michal Hocko <[email protected]>

Thanks!

Andrew, can you, please, pull this patch to 4.20-rc1 or -rc2? It has been acked by Rik and Michal, and tested by Mike.

Thanks!

Shakeel Butt

2018-10-30 17:55 UTC #6 lore raw

Quoted 55 lines · Roman Gushchin

Quoted 53 lines · Michal Hocko
Quoted 51 lines · Roman Gushchin
Mike Galbraith reported a regression caused by the commit 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting") on a system with "cgroup_disable=memory" boot option: the system panics with the following stack trace:
  [0.928542] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
  [0.929317] PGD 0 P4D 0
  [0.929573] Oops: 0002 [#1] PREEMPT SMP PTI
  [0.929984] CPU: 0 PID: 1 Comm: systemd Not tainted 4.19.0-preempt+ #410
  [0.930637] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20180531_142017-buildhw-08.phx2.fed4
  [0.931862] RIP: 0010:page_counter_try_charge+0x22/0xc0
  [0.932376] Code: 41 5d c3 c3 0f 1f 40 00 0f 1f 44 00 00 48 85 ff 0f 84 a7 00 00 00 41 56 48 89 f8 49 89 fe 49
  [0.934283] RSP: 0018:ffffacf68031fcb8 EFLAGS: 00010202
  [0.934826] RAX: 00000000000000f8 RBX: 0000000000000000 RCX: 0000000000000000
  [0.935558] RDX: ffffacf68031fd08 RSI: 0000000000000020 RDI: 00000000000000f8
  [0.936288] RBP: 0000000000000001 R08: 8000000000000063 R09: ffff99ff7cd37a40
  [0.937021] R10: ffffacf68031fed0 R11: 0000000000200000 R12: 0000000000000020
  [0.937749] R13: ffffacf68031fd08 R14: 00000000000000f8 R15: ffff99ff7da1ec60
  [0.938486] FS:  00007fc2140bb280(0000) GS:ffff99ff7da00000(0000) knlGS:0000000000000000
  [0.939311] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [0.939905] CR2: 00000000000000f8 CR3: 0000000012dc8002 CR4: 0000000000760ef0
  [0.940638] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
  [0.941366] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
  [0.942110] PKRU: 55555554
  [0.942412] Call Trace:
  [0.942673]  try_charge+0xcb/0x780
  [0.943031]  memcg_kmem_charge_memcg+0x28/0x80
  [0.943486]  ? __vmalloc_node_range+0x1e4/0x280
  [0.943971]  memcg_kmem_charge+0x8b/0x1d0
  [0.944396]  copy_process.part.41+0x1ca/0x2070
  [0.944853]  ? get_acl+0x1a/0x120
  [0.945200]  ? shmem_tmpfile+0x90/0x90
  [0.945596]  _do_fork+0xd7/0x3d0
  [0.945934]  ? trace_hardirqs_off_thunk+0x1a/0x1c
  [0.946421]  do_syscall_64+0x5a/0x180
  [0.946798]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
The problem occurs because get_mem_cgroup_from_current() returns the NULL pointer if memory controller is disabled. Let's check if this is a case at the beginning of memcg_kmem_charge() and just return 0 if mem_cgroup_disabled() returns true. This is how we handle this case in many other places in the memory controller code.
Fixes 9b6f7e163cd0 ("mm: rework memcg kernel stack accounting")
Reported-by Mike Galbraith <[email protected]>
Signed-off-by Roman Gushchin <[email protected]>
Cc Michal Hocko <[email protected]>
Cc Johannes Weiner <[email protected]>
Cc Vladimir Davydov <[email protected]>
Cc Andrew Morton <[email protected]>

Quoted 6 lines

Quoted 2 lines
I tend to agree with Shakeel that consistency with the other caller would be less confusing.
I totally agree that consistency is a thing here (and everywhere), however using memcg_kmem_enabled() here is not consistent at all. memcg_kmem_enabled() is tight to the slab allocation accounting,

Not really, see __alloc_pages_nodemask() where page allocations with __GFP_ACCOUNT call memcg_kmem_charge() only if memcg_kmem_enabled(). Anyways it's a separate discussion and can be done in the followup cleanup.

Shakeel