linux-kernel - Re: [PATCH] mm: oom_kill_process: do not abort if the victim is exiting

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:	Wed, 25 May 2016 00:05:09 +0900
From:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To:	mhocko@...nel.org, vdavydov@...tuozzo.com
Cc:	akpm@...ux-foundation.org, rientjes@...gle.com, hannes@...xchg.org,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: oom_kill_process: do not abort if the victim is exiting

Michal Hocko wrote:
> On Tue 24-05-16 15:24:02, Vladimir Davydov wrote:
> > After selecting an oom victim, we first check if it's already exiting
> > and if it is, we don't bother killing tasks sharing its mm. We do try to
> > reap its mm though, but we abort if any of the processes sharing it is
> > still alive. This might result in oom deadlock if an exiting task got
> > stuck trying to acquire a lock held by another task sharing the same mm
> > which needs memory to continue: if oom killer happens to keep selecting
> > the stuck task, we won't even try to kill other processes or reap the
> > mm.
> 
> I plan to extend task_will_free_mem to catch this case because we will
> need it for other changes.

Isn't mm_is_reapable() more useful than playing with fatal_signal_pending()
or task_will_free_mem()?

bool mm_is_reapable(struct mm_struct *mm)
{
	struct task_struct *p;

	if (!mm)
		return false;
	if (test_bit(MMF_OOM_REAPABLE, &mm->flags))
		return true;
	if (!down_read_trylock(&mm->mmap_sem))
		return false;
	up_read(&mm->mmap_sem);
	/*
	 * There might be other threads/processes which are either not
	 * dying or even not killable.
	 */
	if (atomic_read(&mm->mm_users) > 1) {
		rcu_read_lock();
		for_each_process(p) {
			bool exiting;

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

			/*
			 * If the task is exiting make sure the whole thread group
			 * is exiting and cannot acces mm anymore.
			 */
			spin_lock_irq(&p->sighand->siglock);
			exiting = signal_group_exit(p->signal);
			spin_unlock_irq(&p->sighand->siglock);
			if (exiting)
				continue;

			/* Give up */
			rcu_read_unlock();
			return false;
		}
		rcu_read_unlock();
	}
	set_bit(MMF_OOM_REAPABLE, &mm->flags);
	return true;
}

 	/*
-	 * If the task is already exiting, don't alarm the sysadmin or kill
-	 * its children or threads, just set TIF_MEMDIE so it can die quickly
+	 * If the victim's memory is already reapable, don't alarm the sysadmin
+	 * or kill its children or threads, just set TIF_MEMDIE and let the
+	 * OOM reaper reap the victim's memory.
 	 */
 	task_lock(p);
-	if (p->mm && task_will_free_mem(p)) {
+	if (mm_is_reapable(p->mm)) {
 		mark_oom_victim(p);
-		try_oom_reaper(p);
+		wake_oom_reaper(p);
 		task_unlock(p);
 		put_task_struct(p);
 		return;
 	}
 	task_unlock(p);

I suggest doing mm_is_reapable() test at __oom_reap_task() side as well
so that we can proceed to next victim by always calling wake_oom_reaper()
whenever TIF_MEMDIE is set.

-	if (can_oom_reap)
-		wake_oom_reaper(victim);
+	wake_oom_reaper(victim);

p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN is not a problem if that p is
already killed (not by the OOM killer) or exiting. We don't need to needlessly
make can_oom_reap false. mm_is_reapable() should do correct test.