Re: [PATCH] mm/mmap_lock: Reset maple state on lock_vma_under_rcu() retry

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Suren Baghdasaryan <surenb@google.com> [251111 19:45]:
> On Tue, Nov 11, 2025 at 4:20 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Suren Baghdasaryan <surenb@google.com> [251111 19:11]:
> > > On Tue, Nov 11, 2025 at 2:18 PM Vlastimil Babka <vbabka@suse.cz> wrote:
> > > >
> > > > On 11/11/25 22:56, Liam R. Howlett wrote:
> > > > > The retry in lock_vma_under_rcu() drops the rcu read lock before
> > > > > reacquiring the lock and trying again.  This may cause a use-after-free
> > > > > if the maple node the maple state was using was freed.
> > >
> > > Ah, good catch. I didn't realize the state is RCU protected.
> > >
> > > > >
> > > > > The maple state is protected by the rcu read lock.  When the lock is
> > > > > dropped, the state cannot be reused as it tracks pointers to objects
> > > > > that may be freed during the time where the lock was not held.
> > > > >
> > > > > Any time the rcu read lock is dropped, the maple state must be
> > > > > invalidated.  Resetting the address and state to MA_START is the safest
> > > > > course of action, which will result in the next operation starting from
> > > > > the top of the tree.
> > > > >
> > > > > Prior to commit 0b16f8bed19c ("mm: change vma_start_read() to drop RCU
> > > > > lock on failure"), the rcu read lock was dropped and NULL was returned,
> > > > > so the retry would not have happened.  However, now that the read lock
> > > > > is dropped regardless of the return, we may use a freed maple tree node
> > > > > cached in the maple state on retry.
> > >
> > > Hmm. The above paragraph does not sound right to me, unless I
> > > completely misunderstood it. Before 0b16f8bed19c we would keep RCU
> > > lock up until the end of lock_vma_under_rcu(),
> >
> > Ah.. usually, yes?  But.. if (unlikely(vma->vm_mm != mm)), then we'd
> > drop and reacquire the rcu read lock, but return NULL.  This was fine
> > because we wouldn't return -EAGIAN and so the read lock was toggled..
> > but it didn't matter since we wouldn't reuse the maple state.
> >
> > I wanted to make it clear that the dropping/reacquiring of the rcu lock
> > prior to 0b16f8bed19c does not mean we have to backport the fix
> > further.. which I failed to do, I guess.
> 
> Ah, ok, now I get it. You were talking about vma_start_read() and RCU
> being dropped there... Would this explanation be a bit better?
> 
> Prior to commit 0b16f8bed19c ("mm: change vma_start_read() to drop RCU
> lock on failure"), vma_start_read() would drop rcu read lock and
> return NULL, so the retry would not have happened. However, now that
> vma_start_read() drops rcu read lock on failure followed by a retry,
> we may end up using a freed maple tree node cached in the maple state.

Yes, sounds good.

Andrew, can you make this change and also drop Cc stable tag?

This needs to be a hot fix, as Vlastimil said earlier.

> 
> >
> > > so retries could still
> > > happen but we were not dropping the RCU lock while doing that. After
> > > 0b16f8bed19c we drop RCU lock if vma_start_read() fails, so retrying
> > > after such failure becomes unsafe. So, if you agree with me assessment
> > > then I suggest changing it to:
> > >
> > > Prior to commit 0b16f8bed19c ("mm: change vma_start_read() to drop RCU
> > > lock on failure"), the retry after vma_start_read() failure was
> > > happening under the same RCU lock. However, now that the read lock is
> > > dropped on failure, we may use a freed maple tree node cached in the
> > > maple state on retry.
> >
> > This is also true, but fails to capture the fact that returning NULL
> > after toggling the lock prior to 0b16f8bed19c is okay.
> >
> > >
> > > > >
> > > > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > > > Cc: stable@vger.kernel.org
> > > > > Fixes: 0b16f8bed19c ("mm: change vma_start_read() to drop RCU lock on failure")
> > > >
> > > > The commit is 6.18-rc1 so we don't need Cc: stable, but it's a mm-hotfixes
> > > > material that must go to Linus before 6.18.
> > > >
> > > > > Reported-by: syzbot+131f9eb2b5807573275c@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=131f9eb2b5807573275c
> > > > > Signed-off-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> > > >
> > > > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > >
> > > With the changelog text sorted out.
> > >
> > > Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> > >
> > > Thanks!
> > >
> > > >
> > > > > ---
> > > > >  mm/mmap_lock.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > > > > index 39f341caf32c0..f2532af6208c0 100644
> > > > > --- a/mm/mmap_lock.c
> > > > > +++ b/mm/mmap_lock.c
> > > > > @@ -257,6 +257,7 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > > > >               if (PTR_ERR(vma) == -EAGAIN) {
> > > > >                       count_vm_vma_lock_event(VMA_LOCK_MISS);
> > > > >                       /* The area was replaced with another one */
> > > > > +                     mas_set(&mas, address);
> > > > >                       goto retry;
> > > > >               }
> > > > >
> > > >