On Wed, Jul 26, 2023 at 11:42 PM Jann Horn <[email protected]> wrote:
> A read of vma->anon_vma under mmap_lock in read mode (in particular in
> anon_vma_prepare()) can race with a concurrent update under mmap_lock
> in read mode plus pagetable lock (in __prepare_anon_vma()).
> However, the only allowed concurrent update is one that changes
> vma->anon_vma from NULL to a non-NULL pointer; once vma->anon_vma has
> been set to a non-NULL value, it will keep that value as long as the
> mmap lock is held in read mode.
[...]
> @@ -1072,7 +1071,15 @@ static int anon_vma_compatible(struct vm_area_struct *a, struct vm_area_struct *
> static struct anon_vma *reusable_anon_vma(struct vm_area_struct *old, struct vm_area_struct *a, struct vm_area_struct *b)
> {
> if (anon_vma_compatible(a, b)) {
> - struct anon_vma *anon_vma = READ_ONCE(old->anon_vma);
> + /*
> + * Pairs with smp_store_release() in __anon_vma_prepare().
> + *
> + * We could get away with a READ_ONCE() here, but
> + * smp_load_acquire() ensures that the following
> + * list_is_singular() check on old->anon_vma_chain doesn't race
> + * with __anon_vma_prepare().
Of course I only realize directly after sending this patch that this
comment only holds...
> + */
> + struct anon_vma *anon_vma = smp_load_acquire(&old->anon_vma);
>
> if (anon_vma && list_is_singular(&old->anon_vma_chain))
> return anon_vma;
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 0c0d8857dfce..83bc4267269f 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -210,8 +210,9 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
> anon_vma_lock_write(anon_vma);
> /* page_table_lock to protect against threads */
> spin_lock(&mm->page_table_lock);
> + /* no need for smp_load_acquire() here, the lock prevents concurrency */
> if (likely(!vma->anon_vma)) {
> - vma->anon_vma = anon_vma;
> + smp_store_release(&vma->anon_vma, anon_vma);
> anon_vma_chain_link(vma, avc, anon_vma);
... if we move the smp_store_release() down by one line here.
> anon_vma->num_active_vmas++;
> allocated = NULL;
On Wed, 26 Jul 2023 at 14:51, Jann Horn <[email protected]> wrote:
>
> Of course I only realize directly after sending this patch that this
> comment only holds... [..]
> ... if we move the smp_store_release() down by one line here.
So I've applied PATCH 1/2 as obvious, but am holding off on this one.
Partly because of the other discussion about memory ordering, but also
due to how the now sometimes much more complex-looking conditionals
could be made a bit visually simpler.
For example the patch does
- if (!vma || !(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma)
+ /* see anon_vma_prepare() */
+ if (!vma || !(vma->vm_flags & VM_MERGEABLE) ||
+ !smp_load_acquire(&vma->anon_vma))
return NULL;
which is a understandably mindless "just add the smp_load_acquire()",
but it is also just unnecessarily hard to read.
And the comment placement is a bit misleading too, since it really
only refers to one part of the expression. And that's very obvious if
you know what it's about, but the whole point of that comment would be
that you don't necessarily know why the code is doing what it is
doing.
IOW, that would be much more legible just split up as
if (!vma || !(vma->vm_flags & VM_MERGEABLE))
return NULL;
/* see anon_vma_prepare() */
if (!smp_load_acquire(&vma->anon_vma))
return NULL;
I feel.
Also, I'm now wondering about the ordering of that
> smp_store_release(&vma->anon_vma, anon_vma);
> anon_vma_chain_link(vma, avc, anon_vma);
sequence. Maybe we *do* want the anon_vma pointer to be set first, so
that once it's on that anon_vma chain (and is visible in the anon_vma
interval tree) it always has a proper anon_vma pointer.
*OR* maybe the rule needs to be that any other concurrent user that
sees 'anon_vma' as being valid also can rely on the links being set
up?
I *suspect* the ordering doesn't actually matter, but it just makes me
wonder more about this area.
Linus