linux-kernel - Re: [PATCH 07/10] mm, oom: fortify 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]

Message-Id: <201606111710.IGF51027.OJLSOQtHVOFFFM@I-love.SAKURA.ne.jp>
Date:	Sat, 11 Jun 2016 17:10:03 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	mhocko@...nel.org
Cc:	linux-mm@...ck.org, rientjes@...gle.com, oleg@...hat.com,
	vdavydov@...allels.com, akpm@...ux-foundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/10] mm, oom: fortify task_will_free_mem

Michal Hocko wrote:
> > Also, I think setting TIF_MEMDIE on p when find_lock_task_mm(p) != p is
> > wrong. While oom_reap_task() will anyway clear TIF_MEMDIE even if we set
> > TIF_MEMDIE on p when p->mm == NULL, it is not true for CONFIG_MMU=n case.
> 
> Yes this would be racy for !CONFIG_MMU but does it actually matter?

I don't know because I've never used CONFIG_MMU=n kernels. But I think it
actually matters. You fixed this race by commit 83363b917a2982dd ("oom:
make sure that TIF_MEMDIE is set under task_lock").

> > > @@ -940,14 +968,10 @@ bool out_of_memory(struct oom_control *oc)
> > >  	 * If current has a pending SIGKILL or is exiting, then automatically
> > >  	 * select it.  The goal is to allow it to allocate so that it may
> > >  	 * quickly exit and free its memory.
> > > -	 *
> > > -	 * But don't select if current has already released its mm and cleared
> > > -	 * TIF_MEMDIE flag at exit_mm(), otherwise an OOM livelock may occur.
> > >  	 */
> > > -	if (current->mm &&
> > > -	    (fatal_signal_pending(current) || task_will_free_mem(current))) {
> > > +	if (task_will_free_mem(current)) {
> > 
> > Setting TIF_MEMDIE on current when current->mm == NULL and
> > find_lock_task_mm(current) != NULL is wrong.
> 
> Why? Or is this still about the !CONFIG_MMU?

Yes, mainly about CONFIG_MMU=n kernels. This change reintroduces possibility
of failing to clear TIF_MEMDIE which was fixed by commit d7a94e7e11badf84
("oom: don't count on mm-less current process").

I was also thinking about possibility of the OOM reaper failing to clear
TIF_MEMDIE due to race with PM/freezing. That is, TIF_MEMDIE is set on a
thread which already released mm, then try_to_freeze_tasks(true) from
freeze_processes() freezes such thread, then oom_reap_task() fails to
clear TIF_MEMDIE from such thread due to oom_killer_disabled == true, then
subsequent memory allocation fails and hibernation aborts, then all threads
are thawed, but TIF_MEMDIE thread won't clear TIF_MEMDIE, and finally the
system locks up because nobody will clear TIF_MEMDIE.

But as I posted in other thread, we can use an approach which does not
introduce possibility of failing to clear TIF_MEMDIE due to race with
PM/freezing. So, this is purely about CONFIG_MMU=n kernels.