Re: [patch] mm: fix anon_vma races

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sun, 19 Oct 2008, Peter Zijlstra wrote:
> 
> Part of the confusion is that we don't clear those pointers at the end
> of their lifetimes (page_remove_rmap and anon_vma_unlink).
> 
> I guess the !page_mapping() test in page_lock_anon_vma() is meant to
> deal with this

Hmm. So that part I'm still not entirely convinced about.

The thing is, we have two issues on anon_vma usage, and the
page_lock_anon_vma() usage in particular:

 - the integrity of the list itself

   Here it should be sufficient to just always get the lock, to the point 
   where we don't need to care about anything else. So getting the lock 
   properly on new allocation makes all the other races irrelevant.

 - the integrity of the _result_ of traversing the list

   This is what the !page_mapping() thing is supposedly protecting 
   against, I think.

   But as far as I can tell, there's really two different use cases here: 
   (a) people who care deeply about the result and (b) people who don't.

   And the difference between the two cases is whether they had the page 
   locked or not. The "try_to_unmap()" callers care deeply, and lock the 
   page. In contrast, some "page_referenced()" callers (really just 
   shrink_active_list) don't care deeply, and to them the return value is 
   really just a heuristic.

As far as I can tell, all the people who care deeply will lock the page 
(and _have_ to lock the page), and thus 'page->mapping' should be stable 
for those cases.

And then we have the other cases, who just want a heuristic, and they 
don't hold the page lock, but if we look at the wrong active_vma that has 
gotten reallocated to something else, they don't even really care. 

So I'm not seeing the reason for that check for page_mapped() at the end. 
Does it actually protect against anything relevant?

Anyway, I _think_ the part that everybody agrees about is the initial 
locking of the anon_vma. Whether we then even need any memory barriers 
and/or the page_mapped() check is an independent question. Yes? No?

So I'm suggesting this commit as the part we at least all agree on. But I 
haven't pushed it out yet, so you can still holler.. But I think all the 
discussion is about other issues, and we all agree on at least this part?

		Linus

---