linux-kernel - Re: [PATCH 4/6] mm, oom: skip vforked tasks from being selected

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: <20160530192856.GA25696@redhat.com>
Date:	Mon, 30 May 2016 21:28:57 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Michal Hocko <mhocko@...nel.org>
Cc:	linux-mm@...ck.org,
	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
	David Rientjes <rientjes@...gle.com>,
	Vladimir Davydov <vdavydov@...allels.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH 4/6] mm, oom: skip vforked tasks from being selected

On 05/30, Michal Hocko wrote:
>
> Make sure to not select vforked task as an oom victim by checking
> vfork_done in oom_badness.

I agree, this look like a good change to me... But.

> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -176,11 +176,13 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
>  
>  	/*
>  	 * Do not even consider tasks which are explicitly marked oom
> -	 * unkillable or have been already oom reaped.
> +	 * unkillable or have been already oom reaped or the are in
> +	 * the middle of vfork
>  	 */
>  	adj = (long)p->signal->oom_score_adj;
>  	if (adj == OOM_SCORE_ADJ_MIN ||
> -			test_bit(MMF_OOM_REAPED, &p->mm->flags)) {
> +			test_bit(MMF_OOM_REAPED, &p->mm->flags) ||
> +			p->vfork_done) {

I don't think we can trust vfork_done != NULL.

copy_process() doesn't disallow CLONE_VFORK without CLONE_VM, so with this patch
it would be trivial to make the exploit which hides a memory hog from oom-killer.

So perhaps we need something like

		bool in_vfork(p)
		{
			return	p->vfork_done &&
				p->real_parent->mm == mm;

			
		}

task_lock() is not enough if CLONE_VM was used along with CLONE_PARENT... so this
also needs rcu_read_lock() to access ->real_parent.

Or I am totally confused?

Oleg.