linux-kernel - Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 8 Aug 2016 20:59:53 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	akpm@...ux-foundation.org
Cc:	geert@...ux-m68k.org, mhocko@...e.com, oleg@...hat.com,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH/RFC] mm, oom: Fix uninitialized ret in task_will_free_mem()

Andrew Morton wrote:
> On Thu, 4 Aug 2016 21:28:13 +0900 Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp> wrote:
> 
> > > 
> > > Fixes: 1af8bb43269563e4 ("mm, oom: fortify task_will_free_mem()")
> > > Signed-off-by: Geert Uytterhoeven <geert@...ux-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?
> 
>