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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

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!

>
> > ---
> > 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.

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 ;) ]

> > +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.

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).

> > @@ -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,

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

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.

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.

It's finnicky, finnicky stuff, which is probably why AI found it :))

--
Cheers, Lorenzo