linux-kernel - Re: [PATCH 1/2] mm,migration: Prevent rmap_walk_[anon|ksm] seeing the wrong VMA information

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.LFD.2.00.1005051737290.901@i5.linux-foundation.org>
Date:	Wed, 5 May 2010 17:42:19 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Mel Gorman <mel@....ul.ie>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linux-MM <linux-mm@...ck.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Minchan Kim <minchan.kim@...il.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Christoph Lameter <cl@...ux.com>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 1/2] mm,migration: Prevent rmap_walk_[anon|ksm] seeing
 the wrong VMA information



On Thu, 6 May 2010, Mel Gorman wrote:
> +	/*
> +	 * Get the root anon_vma on the list by depending on the ordering
> +	 * of the same_vma list setup by __page_set_anon_rmap. Basically
> +	 * we are doing
> +	 *
> +	 * local anon_vma -> local vma -> deepest vma -> anon_vma
> +	 */
> +	avc = list_first_entry(&anon_vma->head, struct anon_vma_chain, same_anon_vma);
> +	vma = avc->vma;
> +	root_avc = list_entry(vma->anon_vma_chain.prev, struct anon_vma_chain, same_vma);
> +	root_anon_vma = root_avc->anon_vma;
> +	if (!root_anon_vma) {
> +		/* XXX: Can this happen? Don't think so but get confirmation */
> +		WARN_ON_ONCE(1);
> +		return anon_vma;
> +	}

No, that can't happen. If you find an avc struct, it _will_ have a 
anon_vma pointer. So there's no point in testing for NULL. If some bug 
happens, you're much better off with the oops than with the warning.

> +	/* Get the lock of the root anon_vma */
> +	if (anon_vma != root_anon_vma) {
> +		/*
> +		 * XXX: This doesn't seem safe. What prevents root_anon_vma
> +		 * getting freed from underneath us? Not much but if
> +		 * we take the second lock first, there is a deadlock
> +		 * possibility if there are multiple callers of rmap_walk
> +		 */
> +		spin_unlock(&anon_vma->lock);
> +		spin_lock(&root_anon_vma->lock);
> +	}

What makes this ok is the fact that it must be running under the RCU read 
lock, and anon_vma's thus cannot be released. My version of the code made 
that explicit. Yours does not, and doesn't even have comments about the 
fact that it needs to be called RCU read-locked. Tssk, tssk.

Please don't just assume locking. Either lock it, or say "this must be 
called with so-and-so held". Not just a silent "this would be buggy if 
anybody ever called it without the RCU lock".

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