linux-kernel - Re: [PATCH 4/4] oom: don't ignore rss in nascent mm

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100916174433.GA4842@redhat.com>
Date:	Thu, 16 Sep 2010 19:44:33 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, oss-security@...ts.openwall.com,
	Solar Designer <solar@...nwall.com>,
	Kees Cook <kees.cook@...onical.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	Neil Horman <nhorman@...driver.com>,
	linux-fsdevel@...r.kernel.org, pageexec@...email.hu,
	Brad Spengler <spender@...ecurity.net>,
	Eugene Teo <eugene@...hat.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	linux-mm <linux-mm@...ck.org>,
	David Rientjes <rientjes@...gle.com>
Subject: Re: [PATCH 4/4] oom: don't ignore rss in nascent mm

On 09/16, KOSAKI Motohiro wrote:
>
> ChangeLog
>  o since v1
>    - Always use thread group leader's ->in_exec_mm.

Confused ;)

> +static unsigned long oom_rss_swap_usage(struct task_struct *p)
> +{
> +	struct task_struct *t = p;
> +	struct task_struct *leader = p->group_leader;
> +	unsigned long points = 0;
> +
> +	do {
> +		task_lock(t);
> +		if (t->mm) {
> +			points += get_mm_rss(t->mm);
> +			points += get_mm_counter(t->mm, MM_SWAPENTS);
> +			task_unlock(t);
> +			break;
> +		}
> +		task_unlock(t);
> +	} while_each_thread(p, t);
> +
> +	/*
> +	 * If the process is in execve() processing, we have to concern
> +	 * about both old and new mm.
> +	 */
> +	task_lock(leader);
> +	if (leader->in_exec_mm) {
> +		points += get_mm_rss(leader->in_exec_mm);
> +		points += get_mm_counter(leader->in_exec_mm, MM_SWAPENTS);
> +	}
> +	task_unlock(leader);
> +
> +	return points;
> +}

This patch relies on fact that we can't race with de_thread() (and btw
the change in de_thread() looks bogus). Then why ->in_exec_mm lives in
task_struct ?

To me, this looks a bit strange. I think we should either do not use
->group_leader to hold ->in_exec_mm like your previous patch did, or
move ->in_exec_mm into signal_struct. The previous 3/4 ensures that
only one thread can set ->in_exec_mm.

And I don't think oom_rss_swap_usage() should replace find_lock_task_mm()
in oom_badness(), I mean something like this:

	static unsigned long oom_rss_swap_usage(struct mm_struct *mm)
	{
		return get_mm_rss(mm) + get_mm_counter(mm, MM_SWAPENTS);
	}

	unsigned int oom_badness(struct task_struct *p, ...)
	{
		int points = 0;

		if (unlikely(p->signal->in_exec_mm)) {
			task_lock(p->group_leader);
			if (p->signal->in_exec_mm)
				points = oom_rss_swap_usage(p->signal->in_exec_mm);
			task_unlock(p->group_leader);
		}

		p = find_lock_task_mm(p);
		if (!p)
			return points;

		...
	}

but this is the matter of taste.

What do you think?

Oleg.

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