[Patch v3 1/2] freezer: check OOM kill while being frozen

[Patch v3 1/2] freezer: check OOM kill while being frozen

[Cong Wang]

There is a race condition between OOM killer and freezer when
they try to operate on the same process, something like below:

        Process A       Process B               Process C
trigger page fault
then trigger oom
B=oom_scan_process_thread()
                                        cgroup freezer freeze(A, B)
                        ...
                        try_to_freeze()
                        stay in D state
oom_kill_process(B)
restart page fault
...

In this case, process A triggered a page fault in user-space,
and the kernel page fault handler triggered OOM, then kernel
selected process B as the victim, right before being killed
process B was frozen by process C therefore went to D state,
then kernel sent SIGKILL but it is already too late as
process B will not care about pending signals any more.

David Rientjes tried to fix same issue with commit
f660daac474c6f (oom: thaw threads if oom killed thread is
frozen before deferring) but it doesn't work any more, because
__thaw_task() just checks if it's frozen and then wakes it up,
but the frozen task, after waking up, will check if freezing()
is still true and continue to freeze itself if so. __thaw_task()
can't make freezing() return false since it doesn't change any
of these conditions, especially cgroup_freezing().

Fix this straightly by checking if the frozen process itself
has been killed by OOM killer, so that the frozen process will
thaw itself and be killed finally.

Cc: David Rientjes <rientjes@google.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 kernel/freezer.c | 13 +++++++++++--
 mm/oom_kill.c    |  2 --
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index aa6a8aa..5b25351 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -52,6 +52,16 @@ bool freezing_slow_path(struct task_struct *p)
 }
 EXPORT_SYMBOL(freezing_slow_path);
 
+static bool should_thaw_current(bool check_kthr_stop)
+{
+	if (!freezing(current) ||
+	    (check_kthr_stop && kthread_should_stop()) ||
+	    test_thread_flag(TIF_MEMDIE))
+		return true;
+	else
+		return false;
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 bool __refrigerator(bool check_kthr_stop)
 {
@@ -67,8 +77,7 @@ bool __refrigerator(bool check_kthr_stop)
 
 		spin_lock_irq(&freezer_lock);
 		current->flags |= PF_FROZEN;
-		if (!freezing(current) ||
-		    (check_kthr_stop && kthread_should_stop()))
+		if (should_thaw_current(check_kthr_stop))
 			current->flags &= ~PF_FROZEN;
 		spin_unlock_irq(&freezer_lock);
 
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1e11df8..112c278 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -266,8 +266,6 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,
 	 * Don't allow any other task to have access to the reserves.
 	 */
 	if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
-		if (unlikely(frozen(task)))
-			__thaw_task(task);
 		if (!force_kill)
 			return OOM_SCAN_ABORT;
 	}
-- 
1.8.3.1

[Patch v3 2/2] freezer: remove obsolete comments in __thaw_task()

[Cong Wang]

__thaw_task() no longer clears frozen flag.

Cc: David Rientjes <rientjes@google.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 kernel/freezer.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 5b25351..33cbcb0 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -156,12 +156,6 @@ void __thaw_task(struct task_struct *p)
 {
 	unsigned long flags;
 
-	/*
-	 * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
-	 * be visible to @p as waking up implies wmb.  Waking up inside
-	 * freezer_lock also prevents wakeups from leaking outside
-	 * refrigerator.
-	 */
 	spin_lock_irqsave(&freezer_lock, flags);
 	if (frozen(p))
 		wake_up_process(p);
-- 
1.8.3.1

Re: [Patch v3 2/2] freezer: remove obsolete comments in __thaw_task()

[Michal Hocko]

On Thu 14-08-14 14:15:26, Cong Wang wrote:
> __thaw_task() no longer clears frozen flag.

since a3201227f803 (freezer: make freezing() test freeze conditions in
effect instead of TIF_FREEZE).

> Cc: David Rientjes <rientjes@google.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Reviewed-by: Michal Hocko <mhocko@suse.cz>

> ---
>  kernel/freezer.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index 5b25351..33cbcb0 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -156,12 +156,6 @@ void __thaw_task(struct task_struct *p)
>  {
>  	unsigned long flags;
>  
> -	/*
> -	 * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
> -	 * be visible to @p as waking up implies wmb.  Waking up inside
> -	 * freezer_lock also prevents wakeups from leaking outside
> -	 * refrigerator.
> -	 */
>  	spin_lock_irqsave(&freezer_lock, flags);
>  	if (frozen(p))
>  		wake_up_process(p);
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [Patch v3 1/2] freezer: check OOM kill while being frozen

[Tejun Heo]

On Thu, Aug 14, 2014 at 02:15:25PM -0700, Cong Wang wrote:
> +static bool should_thaw_current(bool check_kthr_stop)
> +{
> +	if (!freezing(current) ||
> +	    (check_kthr_stop && kthread_should_stop()) ||
> +	    test_thread_flag(TIF_MEMDIE))
> +		return true;
> +	else
> +		return false;
> +}

I'm not sure this is safe w.r.t. hibernation.  Please cc Rafael on
freezer related changes.

Thanks.

-- 
tejun

Re: [Patch v3 1/2] freezer: check OOM kill while being frozen

[Cong Wang]

On Sat, Aug 16, 2014 at 5:20 AM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Aug 14, 2014 at 02:15:25PM -0700, Cong Wang wrote:
>> +static bool should_thaw_current(bool check_kthr_stop)
>> +{
>> +     if (!freezing(current) ||
>> +         (check_kthr_stop && kthread_should_stop()) ||
>> +         test_thread_flag(TIF_MEMDIE))
>> +             return true;
>> +     else
>> +             return false;
>> +}
>
> I'm not sure this is safe w.r.t. hibernation.  Please cc Rafael on
> freezer related changes.
>

Rafael has been Cc'ed from the beginning. :)

Rafael, any objection?

Re: [Patch v3 1/2] freezer: check OOM kill while being frozen

[Cong Wang]

On Sat, Aug 16, 2014 at 5:20 AM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Aug 14, 2014 at 02:15:25PM -0700, Cong Wang wrote:
>> +static bool should_thaw_current(bool check_kthr_stop)
>> +{
>> +     if (!freezing(current) ||
>> +         (check_kthr_stop && kthread_should_stop()) ||
>> +         test_thread_flag(TIF_MEMDIE))
>> +             return true;
>> +     else
>> +             return false;
>> +}
>
> I'm not sure this is safe w.r.t. hibernation.  Please cc Rafael on
> freezer related changes.

Or we can just check for cgroup freeze, something like below:

diff --git a/kernel/freezer.c b/kernel/freezer.c
index 33cbcb0..b06a059 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -56,7 +56,8 @@ static bool should_thaw_current(bool check_kthr_stop)
 {
        if (!freezing(current) ||
            (check_kthr_stop && kthread_should_stop()) ||
-           test_thread_flag(TIF_MEMDIE))
+           /* It might not be safe to check TIF_MEMDIE for pm freeze */
+           (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE)))
                return true;
        else
                return false;


Are you happy now, Tejun? :)

Re: [Patch v3 1/2] freezer: check OOM kill while being frozen

[Tejun Heo]

Hello, Cong.

On Tue, Sep 02, 2014 at 03:52:40PM -0700, Cong Wang wrote:
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index 33cbcb0..b06a059 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -56,7 +56,8 @@ static bool should_thaw_current(bool check_kthr_stop)
>  {
>         if (!freezing(current) ||
>             (check_kthr_stop && kthread_should_stop()) ||
> -           test_thread_flag(TIF_MEMDIE))
> +           /* It might not be safe to check TIF_MEMDIE for pm freeze */
> +           (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE)))
>                 return true;
>         else
>                 return false;
> 
> Are you happy now, Tejun? :)

Yes, this should be a lot safer.  The function still looks weird to me
tho.

	if (cond)
		return true;
	else
		return false;

is equivalent to

	return cond;

If you're worried that the conditional is too complex and harms
readability you can do

	/* explain cond0 */
	if (cond0)
		return true;

	/* explain cond1 */
	if (cond1)
		return true;

	return false;

Thanks.

-- 
tejun

Re: [Patch v3 1/2] freezer: check OOM kill while being frozen

[Cong Wang]

On Wed, Sep 3, 2014 at 8:42 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Cong.
>
> On Tue, Sep 02, 2014 at 03:52:40PM -0700, Cong Wang wrote:
>> diff --git a/kernel/freezer.c b/kernel/freezer.c
>> index 33cbcb0..b06a059 100644
>> --- a/kernel/freezer.c
>> +++ b/kernel/freezer.c
>> @@ -56,7 +56,8 @@ static bool should_thaw_current(bool check_kthr_stop)
>>  {
>>         if (!freezing(current) ||
>>             (check_kthr_stop && kthread_should_stop()) ||
>> -           test_thread_flag(TIF_MEMDIE))
>> +           /* It might not be safe to check TIF_MEMDIE for pm freeze */
>> +           (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE)))
>>                 return true;
>>         else
>>                 return false;
>>
>> Are you happy now, Tejun? :)
>
> Yes, this should be a lot safer.  The function still looks weird to me
> tho.

Cool.

>
>         if (cond)
>                 return true;
>         else
>                 return false;
>
> is equivalent to
>
>         return cond;
>
> If you're worried that the conditional is too complex and harms
> readability you can do
>
>         /* explain cond0 */
>         if (cond0)
>                 return true;
>
>         /* explain cond1 */
>         if (cond1)
>                 return true;
>
>         return false;
>

Sure, definitely much more readable. I will update this patch.

Thanks!