Re: [patch] mm: fix anon_vma races

Peter Zijlstra
2008-10-17T23:13:41Z

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?

Hmm, I guess not, since we won't set the page->mapping until well after
the wmb.. still, tricky.

> allocated = NULL;
> }
> spin_unlock(&mm->page_table_lock);
> @@ -91,6 +98,15 @@ int anon_vma_prepare(struct vm_area_stru
> spin_unlock(&locked->lock);
> if (unlikely(allocated))
> anon_vma_free(allocated);
> + } else {
> + /*
> + * This smp_read_barrier_depends is required to order the data
> + * dependent loads of fields in anon_vma, with the load of the
> + * anon_vma pointer vma->anon_vma. This complements the above
> + * smp_wmb, and prevents a CPU from loading uninitialized
> + * contents of anon_vma.
> + */
> + smp_read_barrier_depends();
> }
> return 0;
> }