Re: [patch] mm: fix anon_vma races

Linus Torvalds
2008-10-18T02:11:38Z

On Sat, 18 Oct 2008, Nick Piggin wrote:
> >
> > Side note: it would be nicer if we had a "spin_lock_init_locked()", so
> > that we could avoid the more expensive "true lock" when doing the initial
> > allocation, but we don't. That said, the case of having to allocate a new
> > anon_vma _should_ be the rare one.
>
> We can't do that, unfortuantely, because anon_vmas are allocated with
> SLAB_DESTROY_BY_RCU.

Aughh. I see what you're saying. We don't _free_ them by RCU, we just
destroy the page allocation. So an anon_vma can get _re-allocated_ for
another page (without being destroyed), concurrently with somebody
optimistically being busy with that same anon_vma that they got through
that optimistic 'page_lock_anon_vma()' thing.

So if we were to just set the lock, we might actually be messing with
something that is still actively used by the previous page that was
unmapped concurrently and still being accessed by try_to_unmap_anon. So
even though we allocated a "new" anon_vma, it might still be busy.

Yes? No?

That thing really is too subtle for words. But if that's actually what you
are alluding to, then doesn't that mean that we _really_ should be doing
that "spin_lock(&anon_vma->lock)" even for the first allocation, and that
the current code is broken? Because otherwise that other concurrent user
that found the stale vma through page_lock_anon_vma() will now try to
follow the linked list and _think_ it's stable (thanks to the lock), but
we're actually inserting entries into it without holding any locks at all.

But I'm hoping I actually am totally *not* understanding what you meant,
and am actually just terminally confused.

Hugh, this is very much your code. Can you please tell me I'm really
confused here, and un-confuse me. Pretty please?

Linus