linux-kernel - Re: [patch 1/5] oom: cleanup android low memory killer

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: <alpine.DEB.2.00.0905041607170.6073@chino.kir.corp.google.com>
Date:	Mon, 4 May 2009 16:12:57 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Greg KH <greg@...ah.com>
cc:	Arve Hjønnevåg <arve@...roid.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Nick Piggin <npiggin@...e.de>, San Mehat <san@...roid.com>,
	linux-kernel@...r.kernel.org, Greg KH <gregkh@...e.de>
Subject: Re: [patch 1/5] oom: cleanup android low memory killer

On Mon, 4 May 2009, Greg KH wrote:

> > This patch in the series is really more of a convenience than anything 
> > else since it doesn't change anything functionally.  I had to modify the 
> > lowmemorykiller later because there's a potential for a NULL pointer from 
> > dereferencing p->mm without holding task_lock(p) and also because I moved 
> > oomkilladj from struct task_struct to struct mm_struct.
> 
> Is this still the case on top of Arve's changes?
> 

Yeah, the first of two patches Arve just sent is broken:

> @@ -71,23 +71,30 @@ static int lowmem_shrink(int nr_to_scan, gfp_t gfp_mask)
>  	}
>  	if(nr_to_scan > 0)
>  		lowmem_print(3, "lowmem_shrink %d, %x, ofree %d, ma %d\n", nr_to_scan, gfp_mask, other_free, min_adj);
> +	rem = global_page_state(NR_ACTIVE) + global_page_state(NR_INACTIVE);
> +	if (nr_to_scan <= 0 || min_adj == OOM_ADJUST_MAX + 1) {
> +		lowmem_print(5, "lowmem_shrink %d, %x, return %d\n", nr_to_scan, gfp_mask, rem);
> +		return rem;
> +	}
> +
>  	read_lock(&tasklist_lock);
>  	for_each_process(p) {
> -		if(p->oomkilladj >= 0 && p->mm) {
> -			tasksize = get_mm_rss(p->mm);
> -			if(nr_to_scan > 0 && tasksize > 0 && p->oomkilladj >= min_adj) {
> -				if(selected == NULL ||
> -				   p->oomkilladj > selected->oomkilladj ||
> -				   (p->oomkilladj == selected->oomkilladj &&
> -				    tasksize > selected_tasksize)) {
> -					selected = p;
> -					selected_tasksize = tasksize;
> -					lowmem_print(2, "select %d (%s), adj %d, size %d, to kill\n",
> -					             p->pid, p->comm, p->oomkilladj, tasksize);
> -				}
> -			}
> -			rem += tasksize;
> +		if (p->oomkilladj < min_adj || !p->mm)
> +			continue;
> +		tasksize = get_mm_rss(p->mm);

You can't do that without holding task_lock(p).

> Right now, people are still arguing that the android low memory driver
> is not needed, but something is, yet no one has proposed a viable
> solution for all parties :(
> 

There was an interest in a low mem userspace notifier that applications 
can poll() on at configurable low mem levels to react accordingly.  This 
would probably address the problem that the Android team is trying to fix.

Regardless, my patchset includes two fixes for current bugs in the oom 
killer: a possible NULL pointer when /proc/sys/vm/oom_dump_tasks is 
enabled and a possible livelock when killing a task that shares memory 
with an OOM_DISABLE task.  I'm not really interested in seeing who can get 
their patches into the staging tree first, I'm more concerned about fixing 
the oom killer.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/