Re: [patch] mm: fix anon_vma races

Linus Torvalds
2008-10-19T18:25:03Z

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

---