linux-kernel - Re: mm->oom_disable

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, 01 Aug 2011 20:52:18 +0900
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	oleg@...hat.com
CC:	rientjes@...gle.com, akpm@...ux-foundation.org,
	torvalds@...ux-foundation.org, roland@...k.frob.com, tj@...nel.org,
	dvlasenk@...hat.com, matt.fleming@...ux.intel.com,
	linux-kernel@...r.kernel.org, avagin@...nvz.org,
	fhrbata@...hat.com, yinghan@...gle.com
Subject: Re: mm->oom_disable_count is broken

(2011/07/31 0:22), Oleg Nesterov wrote:
> On 07/30, Oleg Nesterov wrote:
>>
>> So I'd suggest this hack^Wpatch as a quick fix. I still think this
>> code is not right, but the patch tries to keep the current logic.
> 
> And this reminds me. mm->oom_disable_count looks absolutely broken.
> IIRC, I already complained but nobody replied.
>
> What does this counter mean?
> 
> 	/* How many tasks sharing this mm are OOM_DISABLE */
> 	atomic_t oom_disable_count;
> 
> tasks? processes or threads?
> 
> Lets look at oom_adjust_write(),
> 
> 		if (task->signal->oom_adj == OOM_DISABLE)
> 			atomic_inc(&task->mm->oom_disable_count);
> 
> OK, so it is per-process. No matter how many threads this process
> has, mm->oom_disable_count becomes 1 (ignoring CLONE_VM).
> 
> 
> However, exit_mm() does:
> 
> 	if (tsk->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> 		atomic_dec(&mm->oom_disable_count);
> 
> but this is per-thread! it becomes zero and then negative after
> pthread_exit().
> 
> 
> copy_process()->copy_mm() seems to think it is per-thread too. But,
> bad_fork_cleanup_mm:
> 
> 	if (p->mm) {
> 		task_lock(p);
> 		if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> 			atomic_dec(&p->mm->oom_disable_count);
> 		task_unlock(p);
> 		mmput(p->mm);
> 	}
> 
> Why do we take task_lock() ? OK, oom_score_adj_write() does task_lock()
> too, but this can't help in the multithreaded case? Why copy_mm() checks
> ->oom_score_adj lockless?

IIRC, I did pointed out this issue. But nobody replied.
I think ->oom_disable_count is currently broken. but now I have no time to
audit this stuff. So, I'd suggest to revert this code if nobody don't fix it.




--
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/