Re: [patch] mm: fix anon_vma races

Linus Torvalds
2008-10-17T23:53:39Z

On Sat, 18 Oct 2008, Peter Zijlstra wrote:
> On Thu, 2008-10-16 at 06:10 +0200, Nick Piggin wrote:
>
> > Signed-off-by: Nick Piggin <npiggin@suse.de>
> > ---
> > Index: linux-2.6/mm/rmap.c
> > ===================================================================
> > --- linux-2.6.orig/mm/rmap.c
> > +++ linux-2.6/mm/rmap.c
> > @@ -81,8 +81,15 @@ int anon_vma_prepare(struct vm_area_stru
> > /* page_table_lock to protect against threads */
> > spin_lock(&mm->page_table_lock);
> > if (likely(!vma->anon_vma)) {
> > - vma->anon_vma = anon_vma;
> > list_add_tail(&vma->anon_vma_node, &anon_vma->head);
> > + /*
> > + * This smp_wmb() is required to order all previous
> > + * stores to initialize the anon_vma (by the slab
> > + * ctor) and add this vma, with the store to make it
> > + * visible to other CPUs via vma->anon_vma.
> > + */
> > + smp_wmb();
> > + vma->anon_vma = anon_vma;
>
> I'm not getting why you explicitly move the list_add_tail() before the
> wmb, doesn't the list also expose the anon_vma to other cpus?

I do think the anon_vma locking might be good to look over. It is very
non-obvious. Especially the initial create is really really quite suspect.
I suspect we should start out with the anon-vma locked, and do the

spin_unlock(&anon_vma->lock);

unconditionally in anon_vma_prepare(), and just simplify it. As it is,
newly allocated anon_vma's get exposed in unlocked state while we're still
working on them.

But I think that what Nick did is correct - we always start traversal
through anon_vma->head, so no, the "list_add_tail()" won't expose it to
anybody else, because nobody else has seen the anon_vma().

That said, that's really too damn subtle. We shouldn't rely on memory
ordering for the list handling, when the list handling is _supposed_ to be
using that anon_vma->lock thing.

Linus