Re: [patch] mm: fix anon_vma races

Christoph Lameter
2008-10-21T15:59:40Z

Nick Piggin wrote:

> if (!page_mapped(page))
> goto out;
>
> anon_vma = (struct anon_vma *) (anon_mapping - PAGE_MAPPING_ANON);

Isnt it possible for the anon_vma to be freed and reallocated for another use
after this statement and before taking the lock?

> spin_lock(&anon_vma->lock);

Then we may take the spinlock on another anon_vma structure not related to
this page.

> +
> + /*
> + * If the page is no longer mapped, we have no way to keep the anon_vma
> + * stable. It may be freed and even re-allocated for some other set of
> + * anonymous mappings at any point. Technically this should be OK, as
> + * we hold the spinlock, and should be able to tolerate finding
> + * unrelated vmas on our list. However we'd rather nip these in the bud
> + * here, for simplicity.
> + *
> + * If the page is mapped while we have the lock on the anon_vma, then
> + * we know anon_vma_unlink can't run and garbage collect the anon_vma:
> + * unmapping the page and decrementing its mapcount happens before
> + * unlinking the anon_vma; unlinking the anon_vma requires the
> + * anon_vma lock to be held. So this check ensures we have a stable
> + * anon_vma.
> + *
> + * Note: the page can still become unmapped, and the !page_mapped
> + * condition become true at any point. This check is definitely not
> + * preventing any such thing.
> + */

What is this then? An optimization?

> + if (unlikely(!page_mapped(page))) {
> + spin_unlock(&anon_vma->lock);
> + goto out;
> + }