Re: [PATCH v2] fs/proc/task_mmu: fix missed VMAs when reading /proc/PID/maps and friends

[Suren Baghdasaryan]

On Tue, Oct 27, 2026 at 10:49 AM Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
>
> On Tue, Oct 27, 2026 at 08:34:10AM -0700, Suren Baghdasaryan wrote:
> > Overall the code looks correct bit I have a couple of questions below.
>
> Thanks!

Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

>
> >
> > > ---
> > > Note that I decided it's best to defer any VMA iterator stuff to a
> > > follow-up - with the abstractions added it's clear what's going on and
> > > easier for backporting.
> >
> > Ok. But once we get VMA iterator validation routine implemented, we
> > won't need vma_rcu_iter and its helpers, I think. Are you ok with the
> > fact that most of this change will have to be removed later?
>
> No, I don't think this is correct.
>
> The iterator can't validate itself - the failure case is invisible to it,
> something changed in the tree and it has no way of knowing, it has to be
> prompted to re-init etc.
>
> I tried a version of the code with vma_iter_next_before() and it really didn't
> provide much delta.
>
> So I think any version of this has to have the rvmi stuff.

Hmm. I though the fact that we store from_addr and can compare it with
mas->last could help us detect VMA skips but I'll have to think some
more about this.

>
> In any case, I am happy to replace all this with whatever makes most sense
> going forwards.
>
> Key here is to fix the bug + get it backported :) [ before moving on to the
> N other AI-reported bugs ;) ]

Understood. Yeah, I'm not going to nitpick here as you have your hands
full. If we find a better way we can improve it later.

>
> > > +static inline void vma_rcu_iter_init(struct vma_rcu_iter *rvmi,
> > > +               struct mm_struct *mm, struct vma_iterator *vmi,
> > > +               unsigned long addr)
> > > +{
> > > +       vma_iter_init(vmi, mm, addr);
> > > +       rvmi->mm = mm;
> > > +       rvmi->vmi = vmi;
> > > +       rvmi->mm_wr_seq = UINT_MAX; /* Start odd so changed by default. */
> >
> > Why not initialize mm_wr_seq correctly using
> > mmap_lock_speculate_try_begin(mm, &rvmi->mm_wr_seq)? That way on the
> > first call to lock_next_vma() you won't be resetting vmi
> > unnecessarily.
>
> It doesn't really save anything, the init already sets you up in start state
> where you need to do a traversal anyway, and vma_next() will trigger a walk
> either way.

You might end up with an extra vma_iter_set() call but it's not critical at all.

>
> What's useful about it is it makes it such that only lock_next_vma() does the
> speculation (and thus sampling of mm_wr_seq).

OTOH you won't be exposing these odd/even seqcount details. Anyway,
I'm fine with what you have.

>
> > > @@ -471,14 +505,9 @@ struct vm_area_struct *lock_next_vma(struct mm_struct *mm,
> > >          * installed ahead of it. Otherwise we might observe a gap that should
> > >          * not be there.
> > >          */
> > > -       if (from_addr < vma->vm_start) {
> > > -               /* Verify only if the address space might have changed since vma lookup. */
> > > -               if (!mmap_unlocked || mmap_lock_speculate_retry(mm, mm_wr_seq)) {
> > > -                       vma_iter_set(vmi, from_addr);
> > > -                       if (vma != vma_next(vmi))
> > > -                               goto fallback_unlock;
> > > -               }
> > > -       }
> > > +       if (from_addr < vma->vm_start &&
> > > +           vma_rcu_iter_may_have_skipped(rvmi, from_addr, vma->vm_start - 1))
> > > +               goto fallback_unlock;
> >
> > Before this change we fell back only if the new search yielded a
> > different VMA (the vma != vma_next(vmi) condition) while now we
> > fallback unconditionally. Note that the VMA that we are rechecking is
> > already locked, so it can't change. Was the previous logic invalid?
>
> It's not unconditional - it requires that we have progressed and we may have
> skipped a VMA change.
>
> The 'we have progressed' bit was there before (from_addr < vma->vm_start), and
> vma_rcu_iter_may_have_skipped() wraps the previous check plus actually walks to
> check if something is there to traverse,

Ok, I see. vma_rcu_iter_may_have_skipped() effectively replaces (vma
!= vma_next(vmi)).

>
> So this is strictly an improvement.
>
> And yes the previous logic was invalid - the issue is with an unlocked VMA
> in the range [from_addr, vma->vm_start - 1].
>
> It's really the gap that's the problem AND a race against node replacement,
> which means that 'X' is skipped by vma_next().
>
> vma_next() is a walk to from_addr, then runs mas_next_slot() which advances
> to the next entry. If a gap, then the walk assigns mas->last to the end of
> the gap, and if a node is killed before that happens, due to a VMA
> expanding _backwards_ like:
>
> vma_iter_set(vmi, from_addr), then mas_walk() from mas_find():
>
>         from_addr
>            |
>            v
>  |---------|      |-----|   |---------|
>  |    A    |      |  X  |   |    B    |
>  |---------|      |-----|   |---------|
>            ^     ^
>            |     |
>      mas->index  mas->last
>
> The walk lands in the gap. mas_next_slot() saves mas->last as its
> rewalk point.
>
> X expands backwards, the node containing the gap is replaced:
>
>         from_addr
>            |
>            v
>  |---------|  |---------|   |---------|
>  |    A    |  |    X    |   |    B    |
>  |---------|  |---------|   |---------|
>                  ^
>                  |
>              mas->last
>
> mas_next_slot() finds the node dead and calls mas_rewalk(mas->last),
> which now lands inside X:
>
>         from_addr
>            |
>            v
>  |---------|  |---------|   |---------|
>  |    A    |  |    X    |   |    B    |
>  |---------|  |---------|   |---------|
>                ^        ^
>                |        |
>          mas->index  mas->last
>
> mas_next_slot() then advances past the entry it is on, which is X:
>
>         from_addr           vmi
>            |                |
>            v                v
>  |---------|  |---------|   |---------|
>  |    A    |  |    X    |   |    B    |
>  |---------|  |---------|   |---------|
>
> So vma_next() returns B, having stepped over X. We lock B:
>
>         from_addr           vmi
>            |                |
>            v                v
>  |---------|  |---------|   |---------|
>  |    A    |  |    X    |   |    B    |
>  |---------|  |---------|   |---------|
>                               locked

Ok, so here we hit this "if (from_addr < vma->vm_start)" condition and
have to verify that we did not skip a VMA.

>
> Then if you get (really) unlucky, the same race happens again for the
> existing code:
>
> vma_iter_set(vmi, from_addr);
> if (vma != vma_next(vmi))
>     goto fallback_unlock;
>
> The walk lands in the gap before X, something else is merged into X from
> below, the rewalk lands inside X and the advance steps over it again:
>
>         from_addr           vmi
>            |                |
>            v                v
>  |---------| |----------|   |---------|
>  |    A    | |     X    |   |    B    |
>  |---------| |----------|   |---------|
>                               locked
>
> And suddenly vma == vma_next(vmi), no fallback, and X is never reported.

Got it. The same skip must happen the first time and then during our
verification. Thanks for the detailed explanation.

>
> With the new approach you simply can't miss it, as
> vma_rcu_iter_maybe_changed() will see a change could have happened, and
> vma_range_is_empty() tells you explicitly, with a brand new separate walk,
> whether e.g. X is there.

Yep, that function name is indeed very clear.

>
> It's finnicky, finnicky stuff, which is probably why AI found it :))
>
> --
> Cheers, Lorenzo