lkml.org 
[lkml]   [2016]   [Aug]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()
From
Date
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?
>
>

\
 
 \ /
  Last update: 2016-08-08 14:41    [W:0.062 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site