[PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Geert Uytterhoeven]

    mm/oom_kill.c: In function a??task_will_free_mema??:
    mm/oom_kill.c:767: warning: a??reta?? may be used uninitialized in this function

If __task_will_free_mem() is never called inside the for_each_process()
loop, ret will not be initialized.

Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Untested. I'm not familiar with the code, hence the default value of
true was deducted from the logic in the loop (return false as soon as
__task_will_free_mem() has returned false).
---
 mm/oom_kill.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7d0a275df822e9e1..d53a9aa00977cbd0 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -764,7 +764,7 @@ bool task_will_free_mem(struct task_struct *task)
 {
 	struct mm_struct *mm = task->mm;
 	struct task_struct *p;
-	bool ret;
+	bool ret = true;
 
 	/*
 	 * Skip tasks without mm because it might have passed its exit_mm and
-- 
1.9.1

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

Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Tetsuo Handa]

On 2016/08/04 5:19, Geert Uytterhoeven wrote:
>     mm/oom_kill.c: In function a??task_will_free_mema??:
>     mm/oom_kill.c:767: warning: a??reta?? may be used uninitialized in this function
> 
> If __task_will_free_mem() is never called inside the for_each_process()
> loop, ret will not be initialized.

Recently we are likely overlook this warning because newer versions (!?) do
not warn it. We need to try to compile using newer and older versions.

> 
> Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Untested. I'm not familiar with the code, hence the default value of
> true was deducted from the logic in the loop (return false as soon as
> __task_will_free_mem() has returned false).

I think ret = true is correct. Andrew, please send to linux.git.

> ---
>  mm/oom_kill.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7d0a275df822e9e1..d53a9aa00977cbd0 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -764,7 +764,7 @@ bool task_will_free_mem(struct task_struct *task)
>  {
>  	struct mm_struct *mm = task->mm;
>  	struct task_struct *p;
> -	bool ret;
> +	bool ret = true;
>  
>  	/*
>  	 * Skip tasks without mm because it might have passed its exit_mm and
> 

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

Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Andrew Morton]

On Thu, 4 Aug 2016 21:28:13 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> > 
> > Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Untested. I'm not familiar with the code, hence the default value of
> > true was deducted from the logic in the loop (return false as soon as
> > __task_will_free_mem() has returned false).
> 
> I think ret = true is correct. Andrew, please send to linux.git.

task_will_free_mem() is too hard to understand.

We're examining task "A":

: 	for_each_process(p) {
: 		if (!process_shares_mm(p, mm))
: 			continue;
: 		if (same_thread_group(task, p))
: 			continue;

So here, we've found a process `p' which shares A's mm and which does
not share A's thread group.

: 		ret = __task_will_free_mem(p);

And here we check to see if killing `p' would free up memory.

: 		if (!ret)
: 			break;

If killing `p' will not free memory then give up the scan of all
processes because <reasons>, and we decide that killing `A' will
not free memory either, because some other task is holding onto
A's memory anyway.

: 	}

And if no task is found to be sharing A's mm while not sharing A's
thread group then fall through and decide to kill A.  In which case the
patch to return `true' is correct.

Correctish?  Maybe.  Can we please get some comments in there to
demystify the decision-making?

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

Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Tetsuo Handa]

Andrew Morton wrote:
> On Thu, 4 Aug 2016 21:28:13 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> > > 
> > > Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > ---
> > > Untested. I'm not familiar with the code, hence the default value of
> > > true was deducted from the logic in the loop (return false as soon as
> > > __task_will_free_mem() has returned false).
> > 
> > I think ret = true is correct. Andrew, please send to linux.git.
> 
> task_will_free_mem() is too hard to understand.
> 
> We're examining task "A":
> 
> : 	for_each_process(p) {
> : 		if (!process_shares_mm(p, mm))
> : 			continue;
> : 		if (same_thread_group(task, p))
> : 			continue;
> 
> So here, we've found a process `p' which shares A's mm and which does
> not share A's thread group.

Correct.

> 
> : 		ret = __task_will_free_mem(p);
> 
> And here we check to see if killing `p' would free up memory.

Not correct. Basic idea of __task_will_free_mem() is "check whether
the given task is already killed or exiting" in order to avoid sending
SIGKILL to tasks more than needed, and task_will_free_mem() is "check
whether all of the given mm users are already killed or exiting" in
order to avoid sending SIGKILL to tasks more than needed.

__task_will_free_mem(p) == true means p is already killed or exiting
and therefore the OOM killer does not need to send SIGKILL to `p'.

> 
> : 		if (!ret)
> : 			break;
> 
> If killing `p' will not free memory then give up the scan of all
> processes because <reasons>, and we decide that killing `A' will
> not free memory either, because some other task is holding onto
> A's memory anyway.

If `p' is not already killed or exiting, the OOM reaper cannot reap
p->mm because p will crash if p->mm suddenly disappears. Therefore,
the OOM killer needs to send SIGKILL to somebody.

> 
> : 	}
> 
> And if no task is found to be sharing A's mm while not sharing A's
> thread group then fall through and decide to kill A.  In which case the
> patch to return `true' is correct.

`A' is already killed or exiting, for it passed

	if (!__task_will_free_mem(task))
		return false;

test before the for_each_process(p) loop.

Although

	if (atomic_read(&mm->mm_users) <= 1)
		return true;

test was false as of atomic_read(), it is possible that `p'
releases its mm before reaching

	if (!process_shares_mm(p, mm))
		continue;

test. Therefore, it is possible that __task_will_free_mem(p) is
never called inside the for_each_process(p) loop. In that case,
task_will_free_mem(task) should return true, for it passed

	if (!__task_will_free_mem(task))
		return false;

test before the for_each_process(p) loop.



It is possible that `p' and `A' are the same thread group because
`A' (which can be "current") is not always a thread group leader.
If there is no external process sharing A's mm,

	if (!process_shares_mm(p, mm))
		continue;

test is true for all processes except the process for `A', and

	if (same_thread_group(task, p))
		continue;

test is true for the process for `A'. Therefore, it is possible that
__task_will_free_mem(p) is never called inside the for_each_process(p)
loop. In that case, task_will_free_mem(task) should return true.

> 
> Correctish?  Maybe.  Can we please get some comments in there to
> demystify the decision-making?
> 
> 

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

Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Michal Hocko]

On Thu 04-08-16 14:46:49, Andrew Morton wrote:
> On Thu, 4 Aug 2016 21:28:13 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
> > > 
> > > Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > > ---
> > > Untested. I'm not familiar with the code, hence the default value of
> > > true was deducted from the logic in the loop (return false as soon as
> > > __task_will_free_mem() has returned false).
> > 
> > I think ret = true is correct. Andrew, please send to linux.git.
> 
> task_will_free_mem() is too hard to understand.
> 
> We're examining task "A":
> 
> : 	for_each_process(p) {
> : 		if (!process_shares_mm(p, mm))
> : 			continue;
> : 		if (same_thread_group(task, p))
> : 			continue;
> 
> So here, we've found a process `p' which shares A's mm and which does
> not share A's thread group.
> 
> : 		ret = __task_will_free_mem(p);
> 
> And here we check to see if killing `p' would free up memory.
> 
> : 		if (!ret)
> : 			break;
> 
> If killing `p' will not free memory then give up the scan of all
> processes because <reasons>, and we decide that killing `A' will
> not free memory either, because some other task is holding onto
> A's memory anyway.
> 
> : 	}
> 
> And if no task is found to be sharing A's mm while not sharing A's
> thread group then fall through and decide to kill A.  In which case the
> patch to return `true' is correct.
> 
> Correctish? 

Yes this is more or less correct. task_will_free_mem is a bit misnomer
but I couldn't come up with something better when reworking it and so
I kept the original name. task_will_free_mem basically says that the
task is dying and we hope it will free some memory so it doesn't make
much sense to send it SIGKILL.

> Maybe.  Can we please get some comments in there to
> demystify the decision-making?
 
Does this help?
---
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 908c097c8b47..ce02db7f8661 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -803,8 +803,9 @@ static bool task_will_free_mem(struct task_struct *task)
 		return true;
 
 	/*
-	 * This is really pessimistic but we do not have any reliable way
-	 * to check that external processes share with our mm
+	 * Make sure that all tasks which share the mm with the given tasks
+	 * are dying as well to make sure that a) nobody pins its mm and 
+	 * b) the task is also reapable by the oom reaper.
 	 */
 	rcu_read_lock();
 	for_each_process(p) {

-- 
Michal Hocko
SUSE Labs

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

Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

[Michal Hocko]

On Wed 03-08-16 22:19:59, Geert Uytterhoeven wrote:
>     mm/oom_kill.c: In function a??task_will_free_mema??:
>     mm/oom_kill.c:767: warning: a??reta?? may be used uninitialized in this function
> 
> If __task_will_free_mem() is never called inside the for_each_process()
> loop, ret will not be initialized.
> 
> Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks for catching that!

> ---
> Untested. I'm not familiar with the code, hence the default value of
> true was deducted from the logic in the loop (return false as soon as
> __task_will_free_mem() has returned false).
> ---
>  mm/oom_kill.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 7d0a275df822e9e1..d53a9aa00977cbd0 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -764,7 +764,7 @@ bool task_will_free_mem(struct task_struct *task)
>  {
>  	struct mm_struct *mm = task->mm;
>  	struct task_struct *p;
> -	bool ret;
> +	bool ret = true;
>  
>  	/*
>  	 * Skip tasks without mm because it might have passed its exit_mm and
> -- 
> 1.9.1
> 

-- 
Michal Hocko
SUSE Labs

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