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]

Date:	Wed, 5 May 2010 11:34:05 -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 Wed, 5 May 2010, Mel Gorman wrote:
> 
> In the direction I was taking, only rmap_walk took the deepest lock (I called
> it oldest but hey) and would take other anon_vma locks as well. The objective
> was to make sure the order the locks were taken in was correct.
>
> I think you are suggesting that any anon_vma lock that is taken should always
> take the deepest lock. Am I right and is that necessary? The downsides is that
> there is a single lock that is hotter. The upside is that rmap_walk no longer
> has different semantics as vma_adjust and friends because it's the same lock.

I could personally go either way, I don't really care that deeply.

I think you could easily just take the root lock in the rmap_walk_anon/ksm 
paths, and _also_ take the individual locks as you walk it (safe, since 
now the root lock avoids the ABBA issue - you only need to compare the 
individual lock against the root lock to not take it twice, of course).

Or you could take the "heavy lock" approach that Andrea was arguing for, 
but rather than iterating you'd just take the root lock.

I absolutely _hated_ the "iterate over all locks in the normal case" idea, 
but with the root lock it's much more targeted and no longer is about 
nested locks of the same type.

So the things I care about are just:

 - I hate that "retry" logic that made things more complex and had the 
   livelock problem.

   The "root lock" helper function certainly wouldn't be any fewer lines 
   than your retry version, but it's a clearly separate locking function, 
   rather than mixed in with the walking code. And it doesn't do livelock.

 - I detest "take all locks" in normal paths. I'm ok with it for special 
   case code (and I think the migrate code counts as special case), but I 
   think it was really horribly and fundamentally wrong in that "mm: Take 
   all anon_vma locks in anon_vma_lock" patch I saw.

but whether we want to take the root lock in "anon_vma_lock()" or not is 
just a "detail" as far as I'm concerned. It's no longer "horribly wrong". 
It might have scalability issues etc, of course, but likely only under 
insane loads.

So either way works for me. 

> > 	if (!list_empty(&anon_vma->head)) {
> 
> Can it be empty? I didn't think it was possible as the anon_vma must
> have at least it's own chain.

Ok, so that was answered in the other email - I think it's necessary in 
the general case, although depending on exactly _how_ the page was looked 
up, that may not be true.

If you have guarantees that the page is still mapped (thanks for page 
table lock or something) and the anon_vma can't go away (just a read lock 
on a mm_sem that was used to look up the page would also be sufficient), 
that list_empty() check is unnecessary.

So it's a bit context-dependent.

			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/