memcg: fix oops in oom handling

PATCH In review Li Zefan 5 participants 9 messages Apr 14, 2008

Subsystems (2)

Memory management Memory management - OOM killer

lore

Share & tools

lore

Li Zefan

author

2008-04-14 06:52 UTC #0 lore raw

PATCH 1/1 memcg: fix oops in oom handling

When I used a test program to fork mass processes and immediately move them to a cgroup where the memory limit is low enough to trigger oom kill, I got oops:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000808
IP: [<ffffffff8045c47f>] _spin_lock_irqsave+0x8/0x18
PGD 4c95f067 PUD 4406c067 PMD 0
Oops: 0002 [1] SMP
CPU 2
Modules linked in:

Pid: 11973, comm: a.out Not tainted 2.6.25-rc7 #5

RIP: 0010:[<ffffffff8045c47f>]  [<ffffffff8045c47f>] _spin_lock_irqsave+0x8/0x18
RSP: 0018:ffff8100448c7c30  EFLAGS: 00010002
RAX: 0000000000000202 RBX: 0000000000000009 RCX: 000000000001c9f3
RDX: 0000000000000100 RSI: 0000000000000001 RDI: 0000000000000808
RBP: ffff81007e444080 R08: 0000000000000000 R09: ffff8100448c7900
R10: ffff81000105f480 R11: 00000100ffffffff R12: ffff810067c84140
R13: 0000000000000001 R14: ffff8100441d0018 R15: ffff81007da56200
FS:  00007f70eb1856f0(0000) GS:ffff81007fbad3c0(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000808 CR3: 000000004498a000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process a.out (pid: 11973, threadinfo ffff8100448c6000, task ffff81007da533e0)
Stack:  ffffffff8023ef5a 00000000000000d0 ffffffff80548dc0 00000000000000d0
 ffff810067c84140 ffff81007e444080 ffffffff8026cef9 00000000000000d0
 ffff8100441d0000 00000000000000d0 ffff8100441d0000 ffff8100505445c0
Call Trace:
 [<ffffffff8023ef5a>] ? force_sig_info+0x25/0xb9
 [<ffffffff8026cef9>] ? oom_kill_task+0x77/0xe2
 [<ffffffff8026d696>] ? mem_cgroup_out_of_memory+0x55/0x67
 [<ffffffff802910ad>] ? mem_cgroup_charge_common+0xec/0x202
 [<ffffffff8027997b>] ? handle_mm_fault+0x24e/0x77f
 [<ffffffff8022c4af>] ? default_wake_function+0x0/0xe
 [<ffffffff8027a17a>] ? get_user_pages+0x2ce/0x3af
 [<ffffffff80290fee>] ? mem_cgroup_charge_common+0x2d/0x202
 [<ffffffff8027a441>] ? make_pages_present+0x8e/0xa4
 [<ffffffff8027d1ab>] ? mmap_region+0x373/0x429
 [<ffffffff8027d7eb>] ? do_mmap_pgoff+0x2ff/0x364
 [<ffffffff80210471>] ? sys_mmap+0xe5/0x111
 [<ffffffff8020bfc9>] ? tracesys+0xdc/0xe1

Code: 00 00 01 48 8b 3c 24 e9 46 d4 dd ff f0 ff 07 48 8b 3c 24 e9 3a d4 dd ff fe 07 48 8b 3c 24 e9 2f d4 dd ff 9c 58 fa ba 00 01 00 00 <f0> 66 0f c1 17 38 f2 74 06 f3 90 8a 17 eb f6 c3 fa b8 00 01 00
RIP  [<ffffffff8045c47f>] _spin_lock_irqsave+0x8/0x18
 RSP <ffff8100448c7c30>
CR2: 0000000000000808
---[ end trace c3702fa668021ea4 ]---

It's reproducable in a x86_64 box, but doesn't happen in x86_32.

This is because tsk->sighand is not guarded by RCU, so we have to hold tasklist_lock, just as what out_of_memory() does.

Signed-off-by Li Zefan <[email protected]>

---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +2 -2

index f255eda..beb592f 100644
@@ -423,7 +423,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	cgroup_lock();
-	rcu_read_lock();
+	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, mem);
 	if (PTR_ERR(p) == -1UL)

@@ -436,7 +436,7 @@ retry:
 				"Memory cgroup out of memory"))
 		goto retry;
 out:
-	rcu_read_unlock();
+	read_unlock(&tasklist_lock);
 	cgroup_unlock();
 }
 #endif

-- 1.5.4.rc3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

KAMEZAWA Hiroyuki

2008-04-14 07:11 UTC #1 lore raw

Quoted 6 lines · On Mon, 14 Apr 2008 14:52:00 +0800 Li Zefan <[email protected]> wrote:

It's reproducable in a x86_64 box, but doesn't happen in x86_32.
This is because tsk->sighand is not guarded by RCU, so we have to hold tasklist_lock, just as what out_of_memory() does.

Good catch! and patch seems good.

Paul, I have one confirmation. Lock hierarchy of

	cgroup_lock()
	->	read_lock(&tasklist_lock)

is ok ? (I think this is ok.)

Thanks,
-Kame

Quoted 32 lines

Signed-off-by Li Zefan <[email protected]>

---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +2 -3

index f255eda..beb592f 100644
@@ -423,7 +423,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	cgroup_lock();
-	rcu_read_lock();
+	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, mem);
 	if (PTR_ERR(p) == -1UL)

@@ -436,7 +436,7 @@ retry:
 				"Memory cgroup out of memory"))
 		goto retry;
 out:
-	rcu_read_unlock();
+	read_unlock(&tasklist_lock);
 	cgroup_unlock();
 }
 #endif
-- 1.5.4.rc3

Li Zefan

author

2008-04-14 07:24 UTC #2 lore raw

Quoted 8 lines · KAMEZAWA Hiroyuki

Quoted 5 lines · On Mon, 14 Apr 2008 14:52:00 +0800 Li Zefan <[email protected]> wrote:
It's reproducable in a x86_64 box, but doesn't happen in x86_32.
This is because tsk->sighand is not guarded by RCU, so we have to hold tasklist_lock, just as what out_of_memory() does.

Quoted 8 lines

Good catch! and patch seems good.
Paul, I have one confirmation. Lock hierarchy of
	cgroup_lock()
	->	read_lock(&tasklist_lock)
is ok ? (I think this is ok.)

I think so, because we used to have this in cgroup:

mutex_lock(&cgroup_mutex);
-> attach_task_by_pid()

       -> read_lock(&tasklist_lock)

But afte some time, some uses of tasklist_lock is replaced by rcu.

Quoted 27 lines

Thanks,
-Kame

Quoted 24 lines

Signed-off-by Li Zefan <[email protected]>

---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +2 -2

index f255eda..beb592f 100644
@@ -423,7 +423,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	cgroup_lock();
-	rcu_read_lock();
+	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, mem);
 	if (PTR_ERR(p) == -1UL)

@@ -436,7 +436,7 @@ retry:
 				"Memory cgroup out of memory"))
 		goto retry;
 out:
-	rcu_read_unlock();
+	read_unlock(&tasklist_lock);

Quoted 9 lines

mm/oom_kill.c retry:
Quoted 8 lines
mm/oom_kill.c +0 -1
 	cgroup_unlock();
 }
 #endif
-- 1.5.4.rc3 

Paul Menage

2008-04-14 07:53 UTC #3 lore raw

In reply to KAMEZAWA Hiroyuki

Quoted 6 lines · KAMEZAWA Hiroyuki

Paul, I have one confirmation. Lock hierarchy of
        cgroup_lock()
        ->      read_lock(&tasklist_lock)
is ok ? (I think this is ok.)

Should be fine, I think.

Have you built/booted with lockdep?

Paul

Li Zefan

author

2008-04-14 08:07 UTC #4 lore raw

Quoted 6 lines · Paul Menage

Quoted 4 lines · KAMEZAWA Hiroyuki
Paul, I have one confirmation. Lock hierarchy of
        cgroup_lock()
        ->      read_lock(&tasklist_lock)

Quoted 6 lines

Quoted 1 line
is ok ? (I think this is ok.)
Should be fine, I think.
Have you built/booted with lockdep?

I should have done this. :(

I'll check it.

Quoted 2 lines

Paul

Li Zefan

author

2008-04-14 09:20 UTC #5 lore raw

Quoted 8 lines · Li Zefan

Quoted 7 lines · Paul Menage
Quoted 5 lines · KAMEZAWA Hiroyuki
Paul, I have one confirmation. Lock hierarchy of
        cgroup_lock()
        ->      read_lock(&tasklist_lock)
is ok ? (I think this is ok.)

Quoted 9 lines

Quoted 4 lines
Should be fine, I think.
Have you built/booted with lockdep?
I should have done this. :(
I'll check it.

I've done the test, and nothing is broken ;)

KAMEZAWA Hiroyuki

2008-04-14 07:19 UTC #6 lore raw

In reply to Li Zefan

Quoted 6 lines · On Mon, 14 Apr 2008 14:52:00 +0800 Li Zefan <[email protected]> wrote:

It's reproducable in a x86_64 box, but doesn't happen in x86_32.
This is because tsk->sighand is not guarded by RCU, so we have to hold tasklist_lock, just as what out_of_memory() does.
Signed-off-by Li Zefan <[email protected]>

Andrew, fast-path for 2.6.25 is still not-closed ? I think this patch is worth to be merged as bugfix to 2.6.25 if enough acks.

Thanks,
-Kame

Quoted 31 lines

---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +2 -3

index f255eda..beb592f 100644
@@ -423,7 +423,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	cgroup_lock();
-	rcu_read_lock();
+	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, mem);
 	if (PTR_ERR(p) == -1UL)

@@ -436,7 +436,7 @@ retry:
 				"Memory cgroup out of memory"))
 		goto retry;
 out:
-	rcu_read_unlock();
+	read_unlock(&tasklist_lock);
 	cgroup_unlock();
 }
 #endif
-- 1.5.4.rc3

Andrew Morton

maintainer

2008-04-14 07:48 UTC #7 lore raw

Quoted 10 lines · On Mon, 14 Apr 2008 16:24:04 +0900 KAMEZAWA Hiroyuki…

Quoted 6 lines · On Mon, 14 Apr 2008 14:52:00 +0800 Li Zefan <[email protected]> wrote:
It's reproducable in a x86_64 box, but doesn't happen in x86_32.
This is because tsk->sighand is not guarded by RCU, so we have to hold tasklist_lock, just as what out_of_memory() does.
Signed-off-by Li Zefan <[email protected]>
Andrew, fast-path for 2.6.25 is still not-closed ?

If it was closed, there'd be no point in having rc9 ;)

Quoted 2 lines

I think this patch is worth
to be merged as bugfix to 2.6.25 if enough acks.

Yes, it's in my for-2.6.25 queue:

mmc-fix-platform-driver-hotplug-coldplug.patch rtc-fix-the-error-in-the-function-of-cmos_set_alarm.patch leds-fix-platform-driver-hotplug-coldplug.patch fbdev-fix-proc-fb-oops-after-module-removal.patch fix-sys_unsharesem_undo-add-support-for-clone_sysvsem.patch fix-sys_unsharesem_undo-add-support-for-clone_sysvsem-cleanup.patch fix-sys_unsharesem_undo-perform-an-implicit-clone_sysvsem-in-clone_newipc.patch misc-fix-platform-driver-hotplug-coldplug.patch pcmcia-fix-platform-driver-hotplug-coldplug.patch serial-fix-platform-driver-hotplug-coldplug.patch memcg-fix-oops-in-oom-handling.patch

Balbir Singh

Acked-by on v1 1/1

2008-04-14 07:57 UTC #8 lore raw

In reply to Li Zefan

Quoted 73 lines · Li Zefan

When I used a test program to fork mass processes and immediately move them to a cgroup where the memory limit is low enough to trigger oom kill, I got oops:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000808
IP: [<ffffffff8045c47f>] _spin_lock_irqsave+0x8/0x18
PGD 4c95f067 PUD 4406c067 PMD 0
Oops: 0002 [1] SMP
CPU 2
Modules linked in:

Pid: 11973, comm: a.out Not tainted 2.6.25-rc7 #5

RIP: 0010:[<ffffffff8045c47f>]  [<ffffffff8045c47f>] _spin_lock_irqsave+0x8/0x18
RSP: 0018:ffff8100448c7c30  EFLAGS: 00010002
RAX: 0000000000000202 RBX: 0000000000000009 RCX: 000000000001c9f3
RDX: 0000000000000100 RSI: 0000000000000001 RDI: 0000000000000808
RBP: ffff81007e444080 R08: 0000000000000000 R09: ffff8100448c7900
R10: ffff81000105f480 R11: 00000100ffffffff R12: ffff810067c84140
R13: 0000000000000001 R14: ffff8100441d0018 R15: ffff81007da56200
FS:  00007f70eb1856f0(0000) GS:ffff81007fbad3c0(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000808 CR3: 000000004498a000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process a.out (pid: 11973, threadinfo ffff8100448c6000, task ffff81007da533e0)
Stack:  ffffffff8023ef5a 00000000000000d0 ffffffff80548dc0 00000000000000d0
 ffff810067c84140 ffff81007e444080 ffffffff8026cef9 00000000000000d0
 ffff8100441d0000 00000000000000d0 ffff8100441d0000 ffff8100505445c0
Call Trace:
 [<ffffffff8023ef5a>] ? force_sig_info+0x25/0xb9
 [<ffffffff8026cef9>] ? oom_kill_task+0x77/0xe2
 [<ffffffff8026d696>] ? mem_cgroup_out_of_memory+0x55/0x67
 [<ffffffff802910ad>] ? mem_cgroup_charge_common+0xec/0x202
 [<ffffffff8027997b>] ? handle_mm_fault+0x24e/0x77f
 [<ffffffff8022c4af>] ? default_wake_function+0x0/0xe
 [<ffffffff8027a17a>] ? get_user_pages+0x2ce/0x3af
 [<ffffffff80290fee>] ? mem_cgroup_charge_common+0x2d/0x202
 [<ffffffff8027a441>] ? make_pages_present+0x8e/0xa4
 [<ffffffff8027d1ab>] ? mmap_region+0x373/0x429
 [<ffffffff8027d7eb>] ? do_mmap_pgoff+0x2ff/0x364
 [<ffffffff80210471>] ? sys_mmap+0xe5/0x111
 [<ffffffff8020bfc9>] ? tracesys+0xdc/0xe1

Code: 00 00 01 48 8b 3c 24 e9 46 d4 dd ff f0 ff 07 48 8b 3c 24 e9 3a d4 dd ff fe 07 48 8b 3c 24 e9 2f d4 dd ff 9c 58 fa ba 00 01 00 00 <f0> 66 0f c1 17 38 f2 74 06 f3 90 8a 17 eb f6 c3 fa b8 00 01 00
RIP  [<ffffffff8045c47f>] _spin_lock_irqsave+0x8/0x18
 RSP <ffff8100448c7c30>
CR2: 0000000000000808
---[ end trace c3702fa668021ea4 ]---

It's reproducable in a x86_64 box, but doesn't happen in x86_32.

This is because tsk->sighand is not guarded by RCU, so we have to hold tasklist_lock, just as what out_of_memory() does.

Signed-off-by Li Zefan <[email protected]>

---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +1 -1

index f255eda..beb592f 100644
@@ -423,7 +423,7 @@ void mem_cgroup_out_of_memory(struct mem_cgroup *mem, gfp_t gfp_mask)
 	struct task_struct *p;
 
 	cgroup_lock();
-	rcu_read_lock();
+	read_lock(&tasklist_lock);
 retry:
 	p = select_bad_process(&points, mem);
 	if (PTR_ERR(p) == -1UL)

mm/oom_kill.c retry:

Quoted 9 lines

mm/oom_kill.c +1 -2

@@ -436,7 +436,7 @@ retry:
 				"Memory cgroup out of memory"))
 		goto retry;
 out:
-	rcu_read_unlock();
+	read_unlock(&tasklist_lock);
 	cgroup_unlock();
 }
 #endif
-- 1.5.4.rc3

This looks sane to me

Acked-by Balbir Singh <[email protected]>

Signature · 10 lines

	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>