oom: fix livelock when frozen task is selected

run_guest tries to freeze the current process after it has handled pending interrupts and before it calls lguest_arch_run_guest. This doesn't work nicely if the task has been killed while being frozen and when we want to handle that signal as soon as possible. Let's move try_to_freeze before we check for pending signal so that we can get out of the loop as soon as possible.

Signed-off-by Michal Hocko <[email protected]>

Acked-by Rusty Russell <[email protected]>

---
 drivers/lguest/core.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

drivers/lguest/core.c +7 -7

index 2535933..e7dda91 100644
@@ -232,6 +232,13 @@ int run_guest(struct lg_cpu *cpu, unsigned long __user *user)
 			}
 		}
 
+		/*
+		 * All long-lived kernel loops need to check with this horrible
+		 * thing called the freezer.  If the Host is trying to suspend,
+		 * it stops us.
+		 */
+		try_to_freeze();
+
 		/* Check for signals */
 		if (signal_pending(current))
 			return -ERESTARTSYS;

@@ -246,13 +253,6 @@ int run_guest(struct lg_cpu *cpu, unsigned long __user *user)
 			try_deliver_interrupt(cpu, irq, more);
 
 		/*
-		 * All long-lived kernel loops need to check with this horrible
-		 * thing called the freezer.  If the Host is trying to suspend,
-		 * it stops us.
-		 */
-		try_to_freeze();
-
-		/*
 		 * Just make absolutely sure the Guest is still alive.  One of
 		 * those hypercalls could have been fatal, for example.
 		 */

Signature · 1 line

1.7.5.4

Hi Rusty,
what is the current state of this patch? Are you planning to push it for 3.2?

Quoted 3 lines · Michal Hocko

Hi Rusty,
what is the current state of this patch? Are you planning to push it for 3.2?

Oh. Having acked it, I assumed you'd push. But I've put it in my queue now.

Thanks,
Rusty.

mm/oom_kill.c +3 -0

index 626303b..c419a7e 100644
@@ -32,6 +32,7 @@
 #include <linux/mempolicy.h>
 #include <linux/security.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;

@@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
 				task_pid_nr(q), q->comm);
 			task_unlock(q);
 			force_sig(SIGKILL, q);
+
+			if (frozen(q))

Quoted 18 lines · Michal Hocko

mm/oom_kill.c +3 -0

index 626303b..c419a7e 100644
@@ -32,6 +32,7 @@
 #include <linux/mempolicy.h>
 #include <linux/security.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;

@@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
 				task_pid_nr(q), q->comm);
 			task_unlock(q);
 			force_sig(SIGKILL, q);
+
+			if (frozen(q))

mm/oom_kill.c static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem) · line 466

Quoted 10 lines

mm/oom_kill.c +3 -0

+				thaw_process(q);
 		}
 
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 	force_sig(SIGKILL, p);
+	if (frozen(p))
+		thaw_process(p);
 
 	return 0;
 }

Also needs this...

oom: thaw threads if oom killed thread is frozen before deferring

If a thread has been oom killed and is frozen, thaw it before returning to the page allocator. Otherwise, it can stay frozen indefinitely and no memory will be freed.

Signed-off-by David Rientjes <[email protected]>

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

mm/oom_kill.c +4 -2

@@ -318,8 +318,11 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 		 * blocked waiting for another task which itself is waiting
 		 * for memory. Is there a better alternative?
 		 */
-		if (test_tsk_thread_flag(p, TIF_MEMDIE))
+		if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
+			if (unlikely(frozen(p)))
+				thaw_process(p);
 			return ERR_PTR(-1UL);
+		}
 		if (!p->mm)
 			continue;
 
 
--

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 34 lines · David Rientjes

Quoted 18 lines · Michal Hocko

mm/oom_kill.c +3 -0

index 626303b..c419a7e 100644
@@ -32,6 +32,7 @@
 #include <linux/mempolicy.h>
 #include <linux/security.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;

@@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
 				task_pid_nr(q), q->comm);
 			task_unlock(q);
 			force_sig(SIGKILL, q);
+
+			if (frozen(q))

mm/oom_kill.c static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem) · line 466

Quoted 10 lines

mm/oom_kill.c +3 -0

+				thaw_process(q);
 		}
 
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 	force_sig(SIGKILL, p);
+	if (frozen(p))
+		thaw_process(p);
 
 	return 0;
 }

Also needs this...

Quoted 5 lines

oom: thaw threads if oom killed thread is frozen before deferring

If a thread has been oom killed and is frozen, thaw it before returning to the page allocator. Otherwise, it can stay frozen indefinitely and no memory will be freed.

OK, I can see the race now:
oom_kill_task refrigerator
set_tsk_thread_flag(p, TIF_MEMDIE);
force_sig(SIGKILL, p);
if (frozen(p))
thaw_process(p)

					  frozen_process();
					  [...]
					  if (!frozen(current))
					  	break;
					  schedule();

select_bad_process
[...]
if (test_tsk_thread_flag(p, TIF_MEMDIE))

	  return ERR_PTR(-1UL);

So we either have to make sure that TIF_MEMDIE task is not frozen in select_bad_process (your patch) or check for fatal_signal_pending in refrigerator before we schedule and break out of the loop. Maybe the later one is safer? Rafael?

Quoted 6 lines · Michal Hocko

kernel/freezer.c +0 -0

@@ -48,6 +48,10 @@ void refrigerator(void)
 	current->flags |= PF_FREEZING;
 

kernel/freezer.c void refrigerator(void) · line 52

Quoted 3 lines

kernel/freezer.c +2 -0

 	for (;;) {
+		if (fatal_signal_pending(current)) {
+			current->flags &= ~PF_FROZEN;

We can't do this.

If PF_FROZEN was set, we must not modify current->flags, this can race with, say, thaw_process().

OK, we can take task_lock(), but this doesn't close other races. Say, a SIGKILL'ed task can do try_to_freeze(). Perhaps we should simply call thaw_process() unconditionally, this also clears TIF_FREEZE.
Or check freezing() || frozen(). Afacis this solves the race you described.

But of course this can't help if freeze_task() is called later. May be freezable() should check TIF_MEMDIE...

Oleg.

From 3c6c4b4f1a21c34ea1db76b765ce671ca97d9c3e Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 29 Sep 2011 13:45:22 +0200
Subject: [PATCH] freezer: Get out of refrigerator if fatal signals are
pending

We should make sure that the current task doesn't enter refrigerator if it has fatal signals pending because it should get to the signals processing as soon as possible. Thaw the process if it is either freezing or still frozen to prevent from races with thaw_process.

This closes a possible race when OOM killer selects a task which is about to enter the fridge but it is not set as frozen yet. This will lead to a livelock because select_bad_process would skip that task due to TIF_MEMDIE set for the process but there is no chance for further process.
oom_kill_task refrigerator
set_tsk_thread_flag(p, TIF_MEMDIE);
force_sig(SIGKILL, p);
if (frozen(p))

        thaw_process(p)
                                          frozen_process();
                                          [...]
                                          if (!frozen(current))
                                                break;
                                          schedule();

select_bad_process
[...]
if (test_tsk_thread_flag(p, TIF_MEMDIE))

          return ERR_PTR(-1UL);

Let's skip try_to_freeze in get_signal_to_deliver if fatal signals are pending to make sure that we will not somebody will keep us looping between refrigerator and get_signal_to_deliver for ever.

Signed-off-by Michal Hocko <[email protected]>

---
 kernel/freezer.c |    5 +++++
 kernel/signal.c  |    4 +++-
 2 files changed, 8 insertions(+), 1 deletions(-)

kernel/freezer.c +5 -0

index 7b01de9..0531661 100644
@@ -48,6 +48,11 @@ void refrigerator(void)
 	current->flags |= PF_FREEZING;
 
 	for (;;) {
+		if (fatal_signal_pending(current)) {
+			if (freezing(current) || frozen(current))
+				thaw_process(current);
+			break;
+		}
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		if (!frozen(current))
 			break;

kernel/signal.c +3 -1

index 291c970..bc97a6a 100644
@@ -2147,8 +2147,10 @@ relock:
 	 * While in TASK_STOPPED, we were considered "frozen enough".
 	 * Now that we woke up, it's crucial if we're supposed to be
 	 * frozen that we freeze now before running anything substantial.
+	 * Let's ignore the freezing request if we are about to die anyway.
 	 */
-	try_to_freeze();
+	if (!fatal_signal_pending(curret))
+		try_to_freeze();
 
 	spin_lock_irq(&sighand->siglock);
 	/*

Signature · 9 lines

1.7.6.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Quoted 2 lines · Oleg Nesterov

But of course this can't help if freeze_task() is called later. May be freezable() should check TIF_MEMDIE...

Wouldn't it be easier to ignore try_to_freeze when fatal signals are pending in get_signal_to_deliver?

Quoted 6 lines · Michal Hocko

Quoted 2 lines · Oleg Nesterov

But of course this can't help if freeze_task() is called later. May be freezable() should check TIF_MEMDIE...

Quoted 2 lines

Wouldn't it be easier to ignore try_to_freeze when fatal signals are pending in get_signal_to_deliver?

Oh, I don't think so. For what? This doesn't close other races, and in fact the fatal_signal_pending() this patch adds is itself racy, SIGKILL can come in between.

kernel/freezer.c void refrigerator(void) · line 53

Quoted 10 lines

kernel/freezer.c +3 -0

@@ -48,6 +48,11 @@ void refrigerator(void)
 	current->flags |= PF_FREEZING;
 
 	for (;;) {
+		if (fatal_signal_pending(current)) {
+			if (freezing(current) || frozen(current))
+				thaw_process(current);

Ah, I didn't mean refrigerator() should check freezing/frozen.

I meant, oom_kill can do this before thaw thaw_process(), afaics this should fix the particular race you described (but not others).

And. It is simply wrong to return from refrigerator() after we set PF_FROZEN, this can fool try_to_freeze_tasks(). Sure, thaw_process() from oom_kill is not nice too, but at least this is the special case, we already have the problem.

Oleg.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 2 lines

I meant, oom_kill can do this before thaw thaw_process(), afaics this should fix the particular race you described (but not others).

This is what the follow up fix from David is doing. Check frozen in select_bad_process if the task is TIF_MEMDIE and thaw the process.

And it seems that the David's follow up fix is sufficient so let's leave refrigerator alone.
Or am I still missing something?

Hello,

Quoted 6 lines · Michal Hocko

Quoted 2 lines

I meant, oom_kill can do this before thaw thaw_process(), afaics this should fix the particular race you described (but not others).

This is what the follow up fix from David is doing. Check frozen in select_bad_process if the task is TIF_MEMDIE and thaw the process.

Quoted 3 lines

And it seems that the David's follow up fix is sufficient so let's leave refrigerator alone.
Or am I still missing something?

With pending freezer changes, allowing TIF_MEMDIE tasks to exit freezer by modifying freezing() shouldn't be difficult, which should be race-free and much simpler than diddling with thaw_task().

How urgent is this? Can we wait for the next merge window?

Thanks.

Signature · 8 lines

tejun

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 3 lines

With pending freezer changes, allowing TIF_MEMDIE tasks to exit freezer by modifying freezing() shouldn't be difficult, which should be race-free and much simpler than diddling with thaw_task().

Will the rework help with the initial problem of unkillable OOM selected frozen tasks or it will just help with other races that might be present with the patch? In other words will this work deprecate the 2 patches sent earlier in this thread?

Quoted 1 line

How urgent is this? Can we wait for the next merge window?

Yes, I think we can wait some more.

Hello,

Quoted 4 lines · Michal Hocko

Quoted 3 lines

With pending freezer changes, allowing TIF_MEMDIE tasks to exit freezer by modifying freezing() shouldn't be difficult, which should be race-free and much simpler than diddling with thaw_task().

Quoted 4 lines

Will the rework help with the initial problem of unkillable OOM selected frozen tasks or it will just help with other races that might be present with the patch? In other words will this work deprecate the 2 patches sent earlier in this thread?

I think it shouldn't be difficult to allow OOM-killing frozen tasks. That should be good enough, right?

Quoted 4 lines

Quoted 2 lines

How urgent is this? Can we wait for the next merge window?

Yes, I think we can wait some more.

I'm still processing rather large backlog. I'll ping you back once I sort out the pending freezer changes.

Thanks.

Signature · 8 lines

tejun

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 4 lines · Oleg Nesterov

Oh, I don't think so. For what? This doesn't close other races, and in fact the fatal_signal_pending() this patch adds is itself racy, SIGKILL can come in between.

OK, I think I see your point. You mean that oom will send KILL after both fatal_signal_pending in refrigerator and signal_pending check in schedule, right?

This is what the follow up fix from David is doing. Check frozen in select_bad_process if the task is TIF_MEMDIE and thaw the process.

And it seems that the David's follow up fix is sufficient so let's leave refrigerator alone.

Konstantin Khlebnikov has reported (https://lkml.org/lkml/2011/8/23/45)
that OOM can end up in a live lock if select_bad_process picks up a frozen task.
Unfortunately we cannot mark such processes as unkillable to ignore them because we could panic the system even though there is a chance that somebody could thaw the process so we can make a forward process (e.g. a process from another cpuset or with a different nodemask).

Let's thaw an OOM selected frozen process right after we've sent fatal signal from oom_kill_task.
Thawing is safe if the frozen task doesn't access any suspended device (e.g. by ioctl) on the way out to the userspace where we handle the signal and die. Note, we are not interested in the kernel threads because they are not oom killable.

Accessing suspended devices by a userspace processes shouldn't be an issue because devices are suspended only after userspace is already frozen and oom is disabled at that time.

Other than that userspace accesses the fridge only from the signal handling routines so we are able to handle SIGKILL without any negative side effects or we always check for pending signals after we return from try_to_freeze (e.g. in lguest).

Signed-off-by Michal Hocko <[email protected]>

Reported-by Konstantin Khlebnikov <[email protected]>

Reviewed-by KAMEZAWA Hiroyuki <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Acked-by David Rientjes <[email protected]>

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

mm/oom_kill.c +3 -0

index 626303b..c419a7e 100644
@@ -32,6 +32,7 @@
 #include <linux/mempolicy.h>
 #include <linux/security.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;

@@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
 				task_pid_nr(q), q->comm);
 			task_unlock(q);
 			force_sig(SIGKILL, q);
+
+			if (frozen(q))

Quoted 59 lines · On Tue, 27 Sep 2011 10:01:47 +0200 Michal Hocko <[email protected]> wrote:

Konstantin Khlebnikov has reported (https://lkml.org/lkml/2011/8/23/45)
that OOM can end up in a live lock if select_bad_process picks up a frozen task.
Unfortunately we cannot mark such processes as unkillable to ignore them because we could panic the system even though there is a chance that somebody could thaw the process so we can make a forward process (e.g. a process from another cpuset or with a different nodemask).

Let's thaw an OOM selected frozen process right after we've sent fatal signal from oom_kill_task.
Thawing is safe if the frozen task doesn't access any suspended device (e.g. by ioctl) on the way out to the userspace where we handle the signal and die. Note, we are not interested in the kernel threads because they are not oom killable.

Accessing suspended devices by a userspace processes shouldn't be an issue because devices are suspended only after userspace is already frozen and oom is disabled at that time.

Other than that userspace accesses the fridge only from the signal handling routines so we are able to handle SIGKILL without any negative side effects or we always check for pending signals after we return from try_to_freeze (e.g. in lguest).

Signed-off-by Michal Hocko <[email protected]>

Reported-by Konstantin Khlebnikov <[email protected]>

Reviewed-by KAMEZAWA Hiroyuki <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

Acked-by David Rientjes <[email protected]>

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

mm/oom_kill.c +6 -0

index 626303b..c419a7e 100644
@@ -32,6 +32,7 @@
 #include <linux/mempolicy.h>
 #include <linux/security.h>
 #include <linux/ptrace.h>
+#include <linux/freezer.h>
 
 int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;

@@ -451,10 +452,15 @@ static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem)
 				task_pid_nr(q), q->comm);
 			task_unlock(q);
 			force_sig(SIGKILL, q);
+
+			if (frozen(q))
+				thaw_process(q);
 		}
 
 	set_tsk_thread_flag(p, TIF_MEMDIE);
 	force_sig(SIGKILL, p);
+	if (frozen(p))
+		thaw_process(p);
 

mm/oom_kill.c static int oom_kill_task(struct task_struct *p, struct mem_cgroup *mem) · line 466

Quoted 2 lines

mm/oom_kill.c +0 -0

 	return 0;
 }

I'm not sure this is 1000% correct. Perhaps there's a conceivable window after the "if (frozen)" test where the task can flip itself into the frozen state.

thaw_process() itself appears to be callable regardless of the frozen state and will do the right thing under the right lock. So this code would be safer, correcter and slower if it unconditionally called thaw_process().

I'm sure it doesn't matter though ;)

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>