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

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

[Liam R. Howlett]

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.

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.

Cc: Suren Baghdasaryan <surenb@google.com>
Cc: stable@vger.kernel.org
Fixes: 0b16f8bed19c ("mm: change vma_start_read() to drop RCU lock on failure")
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>
---
 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;
 		}
 
-- 
2.47.2

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

[Vlastimil Babka]

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

> ---
>  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;
>  		}
>  

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

[Suren Baghdasaryan]

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

> >
> > 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;
> >               }
> >
>

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

[Liam R. Howlett]

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

> 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;
> > >               }
> > >
> >

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

[Suren Baghdasaryan]

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.

>
> > 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;
> > > >               }
> > > >
> > >

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

[Liam R. Howlett]

* 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;
> > > > >               }
> > > > >
> > > >

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

[Andrew Morton]

On Tue, 11 Nov 2025 21:18:19 -0500 "Liam R. Howlett" <Liam.Howlett@oracle.com> wrote:

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

Done.

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

Yup.

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

[Lorenzo Stoakes]

+cc Paul for hare-brained idea

On Tue, Nov 11, 2025 at 04:56:05PM -0500, 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.
>
> 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.

Since we all missed it I do wonder if we need some super clear comment
saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
by doing 'blah'.

I think one source of confusion for me with maple tree operations is - what
to do if we are in a position where some kind of reset is needed?

So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
to me that we ought to set to the address.

I guess a mas_reset() would keep mas->index, last where they where which
also wouldn't be right would it?

In which case a mas_reset() is _also_ not a valid operation if invoked
after dropping/reacquiring the RCU lock right?

>
> 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.
>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: stable@vger.kernel.org
> Fixes: 0b16f8bed19c ("mm: change vma_start_read() to drop RCU lock on failure")
> 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>

The reasoning seems sensible & LGTM, so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>


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

I wonder if we could detect that the RCU lock was released (+ reacquired) in
mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?

Not sure if that's feasible, maybe Paul can comment? :)

I think Vlastimil made a similar kind of comment possibly off-list.

Would there be much overhead if we just did this:

retry:
	rcu_read_lock();
	mas_set(&mas, address);
	vma = mas_walk(&mas);

The retry path will be super rare, and I think the compiler should be smart
enough to not assign index, last twice and this would protect us.

Then we could have some function like:

mas_walk_from(&mas, address);

That did this.

Or, since we _literally_ only use mas for this one walk, have a simple
function like:

	/**
	 * ...
	 * Performs a single walk of a maple tree to the specified address,
	 * returning a pointer to an entry if found, or NULL if not.
	 * ...
	 */
	static void *mt_walk(struct maple_tree *mt, unsigned long address)
	{
		MA_STATE(mas, mt, address, adderss);

		lockdep_assert_in_rcu_read_lock();
		return mas_walk(&mas);
	}

That literally just does the walk as a one-off?

>  			goto retry;
>  		}
>
> --
> 2.47.2
>

Cheers, Lorenzo

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

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [251112 10:06]:
> +cc Paul for hare-brained idea
> 
> On Tue, Nov 11, 2025 at 04:56:05PM -0500, 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.
> >
> > 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.
> 
> Since we all missed it I do wonder if we need some super clear comment
> saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> by doing 'blah'.
> 
> I think one source of confusion for me with maple tree operations is - what
> to do if we are in a position where some kind of reset is needed?
> 
> So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> to me that we ought to set to the address.
> 
> I guess a mas_reset() would keep mas->index, last where they where which
> also wouldn't be right would it?

mas->index and mas->last are updated to the values of the entry you
found.  So if you ran a mas_find(), the operation will continue until
the limit is hit, or if you did a next/prev the address would be lost.

This is why I say mas_set() is safer, because it will ensure we return
to the same situation we started from, regardless of the operation.


> 
> In which case a mas_reset() is _also_ not a valid operation if invoked
> after dropping/reacquiring the RCU lock right?

In this case we did a mas_walk(), so a mas_reset() would be fine here.

> 
> >
> > 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.
> >
> > Cc: Suren Baghdasaryan <surenb@google.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 0b16f8bed19c ("mm: change vma_start_read() to drop RCU lock on failure")
> > 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>
> 
> The reasoning seems sensible & LGTM, so:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 
> 
> > ---
> >  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);
> 
> I wonder if we could detect that the RCU lock was released (+ reacquired) in
> mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> 
> Not sure if that's feasible, maybe Paul can comment? :)
> 
> I think Vlastimil made a similar kind of comment possibly off-list.
> 
> Would there be much overhead if we just did this:
> 
> retry:
> 	rcu_read_lock();
> 	mas_set(&mas, address);
> 	vma = mas_walk(&mas);
> 
> The retry path will be super rare, and I think the compiler should be smart
> enough to not assign index, last twice and this would protect us.

This is what existed before the 0b16f8bed19c change, which was
introduced to try and avoid exactly these issues.

I think there's no real way to avoid the complications of an rcu data
structure.  We've tried to make the interface as simple as possible, and
in doing so, have hidden the implementation details of what happens in
the 'state' - which is where all these troubles arise.

I can add more documentation around the locking and maple state,
hopefully people will find it useful and not just exist to go out of
date.

> 
> Then we could have some function like:
> 
> mas_walk_from(&mas, address);
> 
> That did this.
> 
> Or, since we _literally_ only use mas for this one walk, have a simple
> function like:
> 
> 	/**
> 	 * ...
> 	 * Performs a single walk of a maple tree to the specified address,
> 	 * returning a pointer to an entry if found, or NULL if not.
> 	 * ...
> 	 */
> 	static void *mt_walk(struct maple_tree *mt, unsigned long address)
> 	{
> 		MA_STATE(mas, mt, address, adderss);
> 
> 		lockdep_assert_in_rcu_read_lock();
> 		return mas_walk(&mas);
> 	}
> 
> That literally just does the walk as a one-off?


You have described mtree_load().  The mas_ interfaces are designed for
people to handle the locks themselves.  The mtree_ interface handles the
locking for people.

I don't think we are using the simple interface because we are using the
rcu read lock for the vma as well.

If you want to use the simple mtree_load() interface here, I think we'll
need two rcu_read_lock()/unlock() calls (nesting is supported fwiu).  I
don't think we want to nest the locks in this call path though.

...

Thanks,
Liam

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

[Lorenzo Stoakes]

On Wed, Nov 12, 2025 at 11:10:30AM -0500, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [251112 10:06]:
> > +cc Paul for hare-brained idea
> >
> > On Tue, Nov 11, 2025 at 04:56:05PM -0500, 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.
> > >
> > > 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.
> >
> > Since we all missed it I do wonder if we need some super clear comment
> > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > by doing 'blah'.
> >
> > I think one source of confusion for me with maple tree operations is - what
> > to do if we are in a position where some kind of reset is needed?
> >
> > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> > to me that we ought to set to the address.
> >
> > I guess a mas_reset() would keep mas->index, last where they where which
> > also wouldn't be right would it?
>
> mas->index and mas->last are updated to the values of the entry you
> found.  So if you ran a mas_find(), the operation will continue until
> the limit is hit, or if you did a next/prev the address would be lost.

I guess in _this_ specific case since we're specifically looking for a VMA
at the address encoded in the index we'd be ok?

>
> This is why I say mas_set() is safer, because it will ensure we return
> to the same situation we started from, regardless of the operation.

Right yes.

>
>
> >
> > In which case a mas_reset() is _also_ not a valid operation if invoked
> > after dropping/reacquiring the RCU lock right?
>
> In this case we did a mas_walk(), so a mas_reset() would be fine here.

Right yeah.

>
> >
> > >
> > > 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.
> > >
> > > Cc: Suren Baghdasaryan <surenb@google.com>
> > > Cc: stable@vger.kernel.org
> > > Fixes: 0b16f8bed19c ("mm: change vma_start_read() to drop RCU lock on failure")
> > > 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>
> >
> > The reasoning seems sensible & LGTM, so:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> >
> > > ---
> > >  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);
> >
> > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> >
> > Not sure if that's feasible, maybe Paul can comment? :)
> >
> > I think Vlastimil made a similar kind of comment possibly off-list.
> >
> > Would there be much overhead if we just did this:
> >
> > retry:
> > 	rcu_read_lock();
> > 	mas_set(&mas, address);
> > 	vma = mas_walk(&mas);
> >
> > The retry path will be super rare, and I think the compiler should be smart
> > enough to not assign index, last twice and this would protect us.
>
> This is what existed before the 0b16f8bed19c change, which was
> introduced to try and avoid exactly these issues.
>
> I think there's no real way to avoid the complications of an rcu data
> structure.  We've tried to make the interface as simple as possible, and
> in doing so, have hidden the implementation details of what happens in
> the 'state' - which is where all these troubles arise.
>
> I can add more documentation around the locking and maple state,
> hopefully people will find it useful and not just exist to go out of
> date.

Discussed more in reply to Matthew.

Yeah I agree that we're doing a trade-off here between abstraction and
performance overall.

>
> >
> > Then we could have some function like:
> >
> > mas_walk_from(&mas, address);
> >
> > That did this.
> >
> > Or, since we _literally_ only use mas for this one walk, have a simple
> > function like:
> >
> > 	/**
> > 	 * ...
> > 	 * Performs a single walk of a maple tree to the specified address,
> > 	 * returning a pointer to an entry if found, or NULL if not.
> > 	 * ...
> > 	 */
> > 	static void *mt_walk(struct maple_tree *mt, unsigned long address)
> > 	{
> > 		MA_STATE(mas, mt, address, adderss);
> >
> > 		lockdep_assert_in_rcu_read_lock();
> > 		return mas_walk(&mas);
> > 	}
> >
> > That literally just does the walk as a one-off?
>
>
> You have described mtree_load().  The mas_ interfaces are designed for
> people to handle the locks themselves.  The mtree_ interface handles the
> locking for people.
>
> I don't think we are using the simple interface because we are using the
> rcu read lock for the vma as well.

Yup I don't think we can use this as-is.

What I'm proposing Ig uess would be __mtree_load() or something with the
locking taken out.

I have suggested an alternative approach in reply to Matthew anyway.

>
> If you want to use the simple mtree_load() interface here, I think we'll
> need two rcu_read_lock()/unlock() calls (nesting is supported fwiu).  I
> don't think we want to nest the locks in this call path though.

And we don't want to just unconditionally release RCU read lock after we're
done either so I think that's out.

>
> ...
>
> Thanks,
> Liam

Cheers, Lorenzo

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

[Matthew Wilcox]

On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > 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.
> 
> Since we all missed it I do wonder if we need some super clear comment
> saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> by doing 'blah'.

I mean, this really isn't an RCU thing.  This is also bad:

	spin_lock(a);
	p = *q;
	spin_unlock(a);
	spin_lock(a);
	b = *p;

p could have been freed while you didn't hold lock a.  Detecting this
kind of thing needs compiler assistence (ie Rust) to let you know that
you don't have the right to do that any more.

> I think one source of confusion for me with maple tree operations is - what
> to do if we are in a position where some kind of reset is needed?
> 
> So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> to me that we ought to set to the address.

I think that's a separate problem.

> > +++ 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);
> 
> I wonder if we could detect that the RCU lock was released (+ reacquired) in
> mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?

Dropping and reacquiring the RCU read lock should have been a big red
flag.  I didn't have time to review the patches, but if I had, I would
have suggested passing the mas down to the routine that drops the rcu
read lock so it can be invalidated before dropping the readlock.

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

[Paul E. McKenney]

On Thu, Nov 13, 2025 at 12:04:19AM +0000, Matthew Wilcox wrote:
> On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > > 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.
> > 
> > Since we all missed it I do wonder if we need some super clear comment
> > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > by doing 'blah'.
> 
> I mean, this really isn't an RCU thing.  This is also bad:
> 
> 	spin_lock(a);
> 	p = *q;
> 	spin_unlock(a);
> 	spin_lock(a);
> 	b = *p;
> 
> p could have been freed while you didn't hold lock a.  Detecting this
> kind of thing needs compiler assistence (ie Rust) to let you know that
> you don't have the right to do that any more.

While in no way denigrating Rust's compile-time detection of this sort
of thing, use of KASAN combined with CONFIG_RCU_STRICT_GRACE_PERIOD=y
(which restricts you to four CPUs) can sometimes help.

> > I think one source of confusion for me with maple tree operations is - what
> > to do if we are in a position where some kind of reset is needed?
> > 
> > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> > to me that we ought to set to the address.
> 
> I think that's a separate problem.
> 
> > > +++ 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);
> > 
> > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> 
> Dropping and reacquiring the RCU read lock should have been a big red
> flag.  I didn't have time to review the patches, but if I had, I would
> have suggested passing the mas down to the routine that drops the rcu
> read lock so it can be invalidated before dropping the readlock.

There has been some academic efforts to check for RCU-protected pointers
leaking from one RCU read-side critical section to another, but nothing
useful has come from this.  :-/

But rcu_pointer_handoff() and unrcu_pointer() are intended not only for
documentation, but also to suppress the inevitable false positives should
anyone figure out how to detect leaking of RCU-protected pointers.

							Thanx, Paul

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

[Lorenzo Stoakes]

On Wed, Nov 12, 2025 at 05:27:22PM -0800, Paul E. McKenney wrote:
> On Thu, Nov 13, 2025 at 12:04:19AM +0000, Matthew Wilcox wrote:
> > On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > > > 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.
> > >
> > > Since we all missed it I do wonder if we need some super clear comment
> > > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > > by doing 'blah'.
> >
> > I mean, this really isn't an RCU thing.  This is also bad:
> >
> > 	spin_lock(a);
> > 	p = *q;
> > 	spin_unlock(a);
> > 	spin_lock(a);
> > 	b = *p;
> >
> > p could have been freed while you didn't hold lock a.  Detecting this
> > kind of thing needs compiler assistence (ie Rust) to let you know that
> > you don't have the right to do that any more.
>
> While in no way denigrating Rust's compile-time detection of this sort
> of thing, use of KASAN combined with CONFIG_RCU_STRICT_GRACE_PERIOD=y
> (which restricts you to four CPUs) can sometimes help.
>
> > > I think one source of confusion for me with maple tree operations is - what
> > > to do if we are in a position where some kind of reset is needed?
> > >
> > > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> > > to me that we ought to set to the address.
> >
> > I think that's a separate problem.
> >
> > > > +++ 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);
> > >
> > > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> > > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> >
> > Dropping and reacquiring the RCU read lock should have been a big red
> > flag.  I didn't have time to review the patches, but if I had, I would
> > have suggested passing the mas down to the routine that drops the rcu
> > read lock so it can be invalidated before dropping the readlock.
>
> There has been some academic efforts to check for RCU-protected pointers
> leaking from one RCU read-side critical section to another, but nothing
> useful has come from this.  :-/

Ugh a pity. I was hoping we could do (in debug mode only obv) something
absolutely roughly like:

On init:

mas->rcu_critical_section = rcu_get_critical_section_blah();

...

On walk:

	VM_WARN_ON(rcu_critical_section_blah() != mas->rcu_critical_section);

But sounds like that isn't feasible.

I always like the idea of us having debug stuff that helps highlight dumb
mistakes very quickly, no matter how silly they might be :)

>
> But rcu_pointer_handoff() and unrcu_pointer() are intended not only for
> documentation, but also to suppress the inevitable false positives should
> anyone figure out how to detect leaking of RCU-protected pointers.
>
> 							Thanx, Paul

Cheers, Lorenzo

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

[Vlastimil Babka]

(sorry for the late reply)

On 11/13/25 12:05, Lorenzo Stoakes wrote:
>> > > I think one source of confusion for me with maple tree operations is - what
>> > > to do if we are in a position where some kind of reset is needed?
>> > >
>> > > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
>> > > to me that we ought to set to the address.
>> >
>> > I think that's a separate problem.
>> >
>> > > > +++ 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);
>> > >
>> > > I wonder if we could detect that the RCU lock was released (+ reacquired) in
>> > > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
>> >
>> > Dropping and reacquiring the RCU read lock should have been a big red
>> > flag.  I didn't have time to review the patches, but if I had, I would
>> > have suggested passing the mas down to the routine that drops the rcu
>> > read lock so it can be invalidated before dropping the readlock.
>>
>> There has been some academic efforts to check for RCU-protected pointers
>> leaking from one RCU read-side critical section to another, but nothing
>> useful has come from this.  :-/
> 
> Ugh a pity. I was hoping we could do (in debug mode only obv) something
> absolutely roughly like:
> 
> On init:
> 
> mas->rcu_critical_section = rcu_get_critical_section_blah();

AFAICT, get_state_synchronize_rcu()?

> ...
> 
> On walk:
> 
> 	VM_WARN_ON(rcu_critical_section_blah() != mas->rcu_critical_section);

And here, poll_state_synchronize_rcu()?

It wouldn't detect directly that we dropped and reacquired the rcu read
lock, but with enough testing, that would at some point translate to a new
grace period between the first and second read lock, and we'd catch it then?

> But sounds like that isn't feasible.
I don't think what Paul says means your suggestion is not feasible. I think
he says there are no known ways to do this checking automagicallt. But your
suggestion is doing it manually for a specific case. I guess it depends on
how many maple tree functions we'd have to change and how ugly it would be.

> I always like the idea of us having debug stuff that helps highlight dumb
> mistakes very quickly, no matter how silly they might be :)
> 
>>
>> But rcu_pointer_handoff() and unrcu_pointer() are intended not only for
>> documentation, but also to suppress the inevitable false positives should
>> anyone figure out how to detect leaking of RCU-protected pointers.
>>
>> 							Thanx, Paul
> 
> Cheers, Lorenzo

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

[Paul E. McKenney]

On Fri, Nov 21, 2025 at 10:08:19AM +0100, Vlastimil Babka wrote:
> (sorry for the late reply)

And me for missing this!

> On 11/13/25 12:05, Lorenzo Stoakes wrote:
> >> > > I think one source of confusion for me with maple tree operations is - what
> >> > > to do if we are in a position where some kind of reset is needed?
> >> > >
> >> > > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> >> > > to me that we ought to set to the address.
> >> >
> >> > I think that's a separate problem.
> >> >
> >> > > > +++ 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);
> >> > >
> >> > > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> >> > > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> >> >
> >> > Dropping and reacquiring the RCU read lock should have been a big red
> >> > flag.  I didn't have time to review the patches, but if I had, I would
> >> > have suggested passing the mas down to the routine that drops the rcu
> >> > read lock so it can be invalidated before dropping the readlock.
> >>
> >> There has been some academic efforts to check for RCU-protected pointers
> >> leaking from one RCU read-side critical section to another, but nothing
> >> useful has come from this.  :-/
> > 
> > Ugh a pity. I was hoping we could do (in debug mode only obv) something
> > absolutely roughly like:
> > 
> > On init:
> > 
> > mas->rcu_critical_section = rcu_get_critical_section_blah();
> 
> AFAICT, get_state_synchronize_rcu()?

This would get a grace-period counter, and I believe that Lorenzo wants
a per-task count of RCU read-side critical sections.  In theory, this
is easy, but it does add overhead.  And in practice, there are a lot
of different types of RCU readers, and keeping them all straight would
be quite challenging.  But if you only care about a debug-only facility
for preemptible RCU's rcu_read_lock() and rcu_read_unlock(), something
could be done.

Besides, I didn't understand what Vlastimil was getting at...

> > ...
> > 
> > On walk:
> > 
> > 	VM_WARN_ON(rcu_critical_section_blah() != mas->rcu_critical_section);
> 
> And here, poll_state_synchronize_rcu()?
> 
> It wouldn't detect directly that we dropped and reacquired the rcu read
> lock, but with enough testing, that would at some point translate to a new
> grace period between the first and second read lock, and we'd catch it then?

Ah, good point.  And CONFIG_RCU_STRICT_GRACE_PERIOD=y would make it more
likely to happen.  As would booting with rcupdate.rcu_expedited=1.

> > But sounds like that isn't feasible.
> I don't think what Paul says means your suggestion is not feasible. I think
> he says there are no known ways to do this checking automagicallt. But your
> suggestion is doing it manually for a specific case. I guess it depends on
> how many maple tree functions we'd have to change and how ugly it would be.

All good points!

							Thanx, Paul

> > I always like the idea of us having debug stuff that helps highlight dumb
> > mistakes very quickly, no matter how silly they might be :)
> > 
> >>
> >> But rcu_pointer_handoff() and unrcu_pointer() are intended not only for
> >> documentation, but also to suppress the inevitable false positives should
> >> anyone figure out how to detect leaking of RCU-protected pointers.
> >>
> >> 							Thanx, Paul
> > 
> > Cheers, Lorenzo
> 

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

[Lorenzo Stoakes]

On Thu, Nov 13, 2025 at 12:04:19AM +0000, Matthew Wilcox wrote:
> On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > > 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.
> >
> > Since we all missed it I do wonder if we need some super clear comment
> > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > by doing 'blah'.
>
> I mean, this really isn't an RCU thing.  This is also bad:
>
> 	spin_lock(a);
> 	p = *q;
> 	spin_unlock(a);
> 	spin_lock(a);
> 	b = *p;
>
> p could have been freed while you didn't hold lock a.  Detecting this
> kind of thing needs compiler assistence (ie Rust) to let you know that
> you don't have the right to do that any more.

Right but in your example the use of the pointers is _realy clear_. In the
mas situation, the pointers are embedded in the helper struct, there's a
state machine, etc. so it's harder to catch this.

There's already a state machine embedded in it, and I think the confusing
bit, at least for me, was a line of thinking like - 'oh there's all this
logic that figures out what's going on and if there's an error rewalks and
etc. - so it'll handle this case too'.

Obviously, very much wrong.

Generally I wonder if, when dealing with VMAs, we shouldn't just use the
VMA iterator anyway? Whenever I see 'naked' mas stuff I'm always a little
confused as to why.


>
> > I think one source of confusion for me with maple tree operations is - what
> > to do if we are in a position where some kind of reset is needed?
> >
> > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> > to me that we ought to set to the address.
>
> I think that's a separate problem.

Sure but I think there's a broader issue around confusion arising around
mas state and when we need to do one thing or another, there were a number
of issues that arose in the past where people got confused about what to do
with vma iterator state.

I think it's a difficult problem - we're both trying to abstract stuff
here but also retain performance, which is a trade-off.

>
> > > +++ 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);
> >
> > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
>
> Dropping and reacquiring the RCU read lock should have been a big red
> flag.  I didn't have time to review the patches, but if I had, I would

I think if you have 3 mm developers who all work with VMAs all the time
missing this, that's a signal that something is confusing here :)

So the issue is we all thought dropping the RCU lock would be OK, and
mas_walk(...) would 'somehow' do the right thing. See above for why I think
perhaps that happened.

> have suggested passing the mas down to the routine that drops the rcu
> read lock so it can be invalidated before dropping the readlock.
>

This would require changing vma_start_read(), which is called by both
lock_vma_under_rcu() and lock_next_vma().

We could make them consistent and have lock_vma_under_rcu() do something
like:

	VMA_ITERATOR(vmi, mm, address);

	...

	rcu_read_lock();
	vma = vma_start_read(&vmi);

And have vma_start_read() handle the:

	if (!vma) {
		rcu_read_unlock();
		goto inval;
	}

Case we have in lock_vma_under_rcu() now.

We'd need to keep:

	vma = vma_next(vmi);
	if (!vma)
		return NULL;

In lock_next_vma().

Then you could have:

err:
	/* Reset so state is valid if reused. */
	vmi_iter_reset(vmi);
	rcu_read_unlock();

In vma_start_read().

Assuming any/all of this is correct :)

I _think_ based on what Liam said in other sub-thread the reset should work
here (perhaps not quite maximally efficient).

If we risk perhaps relying on the optimiser to help us or hope no real perf
impact perhaps we could do both by also having the 'set address' bit happen
in lock_vma_under_rcu() also e.g.:


	VMA_ITERATOR(vmi, mm, address);

	...

retry:
	rcu_read_lock();
	vma_iter_set(&vmi, address);
	vma = vma_start_read(&vmi);

Let me know if any of this is sane... :)

Cheers, Lorenzo

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

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [251113 05:45]:
> On Thu, Nov 13, 2025 at 12:04:19AM +0000, Matthew Wilcox wrote:
> > On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > > > 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.
> > >
> > > Since we all missed it I do wonder if we need some super clear comment
> > > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > > by doing 'blah'.
> >
> > I mean, this really isn't an RCU thing.  This is also bad:
> >
> > 	spin_lock(a);
> > 	p = *q;
> > 	spin_unlock(a);
> > 	spin_lock(a);
> > 	b = *p;
> >
> > p could have been freed while you didn't hold lock a.  Detecting this
> > kind of thing needs compiler assistence (ie Rust) to let you know that
> > you don't have the right to do that any more.
> 
> Right but in your example the use of the pointers is _realy clear_. In the
> mas situation, the pointers are embedded in the helper struct, there's a
> state machine, etc. so it's harder to catch this.

We could modify the above example to use a helper struct and the same
problem would arise...

> 
> There's already a state machine embedded in it, and I think the confusing
> bit, at least for me, was a line of thinking like - 'oh there's all this
> logic that figures out what's going on and if there's an error rewalks and
> etc. - so it'll handle this case too'.
> 
> Obviously, very much wrong.
> 
> Generally I wonder if, when dealing with VMAs, we shouldn't just use the
> VMA iterator anyway? Whenever I see 'naked' mas stuff I'm always a little
> confused as to why.

I am not sure why this was left as maple state either.  But translating
it to the vma iterator would result in the same bug.  The locking story
would be the same.  There isn't much to the vma iterator, it will just
call the mas_ functions for you.

In other code, the maple state is used when we need to do special
operations that would be the single user of a vma iterator function.  I
suspect this was the case here at some point.

> 
> 
> >
> > > I think one source of confusion for me with maple tree operations is - what
> > > to do if we are in a position where some kind of reset is needed?
> > >
> > > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> > > to me that we ought to set to the address.
> >
> > I think that's a separate problem.
> 
> Sure but I think there's a broader issue around confusion arising around
> mas state and when we need to do one thing or another, there were a number
> of issues that arose in the past where people got confused about what to do
> with vma iterator state.
> 
> I think it's a difficult problem - we're both trying to abstract stuff
> here but also retain performance, which is a trade-off.
> 
> >
> > > > +++ 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);
> > >
> > > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> > > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> >
> > Dropping and reacquiring the RCU read lock should have been a big red
> > flag.  I didn't have time to review the patches, but if I had, I would
> 
> I think if you have 3 mm developers who all work with VMAs all the time
> missing this, that's a signal that something is confusing here :)
> 
> So the issue is we all thought dropping the RCU lock would be OK, and
> mas_walk(...) would 'somehow' do the right thing. See above for why I think
> perhaps that happened.

But again, I feel like we could replace the maple state with any helper
struct and this could also be missed.

I'm not sure there's an easy way to remove this class of errors without
changing the basic tooling to be rust or the like...

vma_start_read() is inherently complicated because of what it does
without taking the mmap lock.  Dealing with a potential failure/retry is
equally messy.

The locking is impossible to do in a clean way since one caller does not
take the rcu read lock itself, but may return without it held in many
scenarios.

> 
> > have suggested passing the mas down to the routine that drops the rcu
> > read lock so it can be invalidated before dropping the readlock.
> >
> 
> This would require changing vma_start_read(), which is called by both
> lock_vma_under_rcu() and lock_next_vma().
> 
> We could make them consistent and have lock_vma_under_rcu() do something
> like:
> 
> 	VMA_ITERATOR(vmi, mm, address);
> 
> 	...
> 
> 	rcu_read_lock();
> 	vma = vma_start_read(&vmi);
> 
> And have vma_start_read() handle the:
> 
> 	if (!vma) {
> 		rcu_read_unlock();
> 		goto inval;
> 	}
> 
> Case we have in lock_vma_under_rcu() now.
> 
> We'd need to keep:
> 
> 	vma = vma_next(vmi);
> 	if (!vma)
> 		return NULL;
> 
> In lock_next_vma().
> 
> Then you could have:
> 
> err:
> 	/* Reset so state is valid if reused. */
> 	vmi_iter_reset(vmi);
> 	rcu_read_unlock();
> 
> In vma_start_read().
> 
> Assuming any/all of this is correct :)
> 
> I _think_ based on what Liam said in other sub-thread the reset should work
> here (perhaps not quite maximally efficient).

No, don't do that.  If you want to go this route, use vma_iter_set() in
the error label to set the address.  Which means that we'll need to pass
the vma iterator and the address into vma_star_read() from both callers.

And may as well add this in vma_start_read()..

err_unstable:
 	vma_iter_set(&vmi, address);

> 
> If we risk perhaps relying on the optimiser to help us or hope no real perf
> impact perhaps we could do both by also having the 'set address' bit happen
> in lock_vma_under_rcu() also e.g.:
> 
> 
> 	VMA_ITERATOR(vmi, mm, address);
> 
> 	...
> 
> retry:
> 	rcu_read_lock();
> 	vma_iter_set(&vmi, address);
> 	vma = vma_start_read(&vmi);

lock_next_vma() also calls vma_iter_set() in the -EAGAIN case, so
passing both through might make more sense.

> 
> Let me know if any of this is sane... :)

The locking on this function makes it virtually impossible to reuse for
anything beyond the two users it has today.  Passing the iterator down
might remind people of what to do if the function itself changes.  It
does seem like the right way of handling this, since we can't clean up
the locking.

Thanks,
Liam

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

[Lorenzo Stoakes]

On Thu, Nov 13, 2025 at 12:28:58PM -0500, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [251113 05:45]:
> > On Thu, Nov 13, 2025 at 12:04:19AM +0000, Matthew Wilcox wrote:
> > > On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > > > > 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.
> > > >
> > > > Since we all missed it I do wonder if we need some super clear comment
> > > > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > > > by doing 'blah'.
> > >
> > > I mean, this really isn't an RCU thing.  This is also bad:
> > >
> > > 	spin_lock(a);
> > > 	p = *q;
> > > 	spin_unlock(a);
> > > 	spin_lock(a);
> > > 	b = *p;
> > >
> > > p could have been freed while you didn't hold lock a.  Detecting this
> > > kind of thing needs compiler assistence (ie Rust) to let you know that
> > > you don't have the right to do that any more.
> >
> > Right but in your example the use of the pointers is _realy clear_. In the
> > mas situation, the pointers are embedded in the helper struct, there's a
> > state machine, etc. so it's harder to catch this.
>
> We could modify the above example to use a helper struct and the same
> problem would arise...

I disagree.

It's a helper struct with a state machine, manipulated by API functions. In fact
it turns out we _can_ recover this state even after dropping/reacquiring the
lock by calling the appropriate API functions to do so.

You manipulate this state via mas_xxx() commands, and in fact we resolve the
lock issue by issuing the correct one.

So it's a problem of abstraction I think.

HOWEVER, clearly the crux of the problem as you say elsewhere is that we are
using the 'advanced' API and handling our own lock, which leaves us open to
mistakes like this.

My thought process here is around 'can we avoid a bunch of mm developers all
making the same mistake again'.

In this case I mean - it's a unique situation, in some already _very_ hairy VMA
lock code, that used to be much simpler (*grumble grumble*). We're paying the
price for rolling our own mechanism here in general.

But I think more broadly, perhaps there's things we can do here to help. You
need to be able to go on vacation without having to worry about what mistakes we
might make with this stuff :P

>
> >
> > There's already a state machine embedded in it, and I think the confusing
> > bit, at least for me, was a line of thinking like - 'oh there's all this
> > logic that figures out what's going on and if there's an error rewalks and
> > etc. - so it'll handle this case too'.
> >
> > Obviously, very much wrong.
> >
> > Generally I wonder if, when dealing with VMAs, we shouldn't just use the
> > VMA iterator anyway? Whenever I see 'naked' mas stuff I'm always a little
> > confused as to why.
>
> I am not sure why this was left as maple state either.  But translating
> it to the vma iterator would result in the same bug.  The locking story
> would be the same.  There isn't much to the vma iterator, it will just
> call the mas_ functions for you.

Yes I understand it wouldn't fix the bug :) I'm saying this as an aside, and it
leads into the suggestion I make below.

>
> In other code, the maple state is used when we need to do special
> operations that would be the single user of a vma iterator function.  I
> suspect this was the case here at some point.

Right yes. And perhaps so.

>
> >
> >
> > >
> > > > I think one source of confusion for me with maple tree operations is - what
> > > > to do if we are in a position where some kind of reset is needed?
> > > >
> > > > So even if I'd realised 'aha we need to reset this' it wouldn't be obvious
> > > > to me that we ought to set to the address.
> > >
> > > I think that's a separate problem.
> >
> > Sure but I think there's a broader issue around confusion arising around
> > mas state and when we need to do one thing or another, there were a number
> > of issues that arose in the past where people got confused about what to do
> > with vma iterator state.
> >
> > I think it's a difficult problem - we're both trying to abstract stuff
> > here but also retain performance, which is a trade-off.
> >
> > >
> > > > > +++ 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);
> > > >
> > > > I wonder if we could detect that the RCU lock was released (+ reacquired) in
> > > > mas_walk() in a debug mode, like CONFIG_VM_DEBUG_MAPLE_TREE?
> > >
> > > Dropping and reacquiring the RCU read lock should have been a big red
> > > flag.  I didn't have time to review the patches, but if I had, I would
> >
> > I think if you have 3 mm developers who all work with VMAs all the time
> > missing this, that's a signal that something is confusing here :)
> >
> > So the issue is we all thought dropping the RCU lock would be OK, and
> > mas_walk(...) would 'somehow' do the right thing. See above for why I think
> > perhaps that happened.
>
> But again, I feel like we could replace the maple state with any helper
> struct and this could also be missed.

I disagree for the reasons stated above.

>
> I'm not sure there's an easy way to remove this class of errors without
> changing the basic tooling to be rust or the like...

Well I like to be optimistic that we can find ways forward without that.

>
> vma_start_read() is inherently complicated because of what it does
> without taking the mmap lock.  Dealing with a potential failure/retry is
> equally messy.

Yes I agree.

>
> The locking is impossible to do in a clean way since one caller does not
> take the rcu read lock itself, but may return without it held in many
> scenarios.

Yes absolutely. I am not necessarily in love with how complicated we've made all
of this and I am not sure it was justified, but unfortunately I didn't pay
enough attention to the VMA lock seqcount rework.

>
> >
> > > have suggested passing the mas down to the routine that drops the rcu
> > > read lock so it can be invalidated before dropping the readlock.
> > >
> >
> > This would require changing vma_start_read(), which is called by both
> > lock_vma_under_rcu() and lock_next_vma().
> >
> > We could make them consistent and have lock_vma_under_rcu() do something
> > like:
> >
> > 	VMA_ITERATOR(vmi, mm, address);
> >
> > 	...
> >
> > 	rcu_read_lock();
> > 	vma = vma_start_read(&vmi);
> >
> > And have vma_start_read() handle the:
> >
> > 	if (!vma) {
> > 		rcu_read_unlock();
> > 		goto inval;
> > 	}
> >
> > Case we have in lock_vma_under_rcu() now.
> >
> > We'd need to keep:
> >
> > 	vma = vma_next(vmi);
> > 	if (!vma)
> > 		return NULL;
> >
> > In lock_next_vma().
> >
> > Then you could have:
> >
> > err:
> > 	/* Reset so state is valid if reused. */
> > 	vmi_iter_reset(vmi);
> > 	rcu_read_unlock();
> >
> > In vma_start_read().
> >
> > Assuming any/all of this is correct :)
> >
> > I _think_ based on what Liam said in other sub-thread the reset should work
> > here (perhaps not quite maximally efficient).
>
> No, don't do that.  If you want to go this route, use vma_iter_set() in
> the error label to set the address.  Which means that we'll need to pass
> the vma iterator and the address into vma_star_read() from both callers.

Well that's what I'm proposing we do re: passing in the vma iterator, so it
seems we're generally aligned on this, but sure we should use vma_iter_set(),
ack on that.

>
> And may as well add this in vma_start_read()..
>
> err_unstable:
>  	vma_iter_set(&vmi, address);

Ack.

>
> >
> > If we risk perhaps relying on the optimiser to help us or hope no real perf
> > impact perhaps we could do both by also having the 'set address' bit happen
> > in lock_vma_under_rcu() also e.g.:
> >
> >
> > 	VMA_ITERATOR(vmi, mm, address);
> >
> > 	...
> >
> > retry:
> > 	rcu_read_lock();
> > 	vma_iter_set(&vmi, address);
> > 	vma = vma_start_read(&vmi);
>
> lock_next_vma() also calls vma_iter_set() in the -EAGAIN case, so
> passing both through might make more sense.

Yes.

>
> >
> > Let me know if any of this is sane... :)
>
> The locking on this function makes it virtually impossible to reuse for
> anything beyond the two users it has today.  Passing the iterator down
> might remind people of what to do if the function itself changes.  It
> does seem like the right way of handling this, since we can't clean up
> the locking.

OK, I can put forward a patch for this if that'd be helpful!

>
> Thanks,
> Liam
>

Cheers, Lorenzo

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

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [251114 06:51]:
> On Thu, Nov 13, 2025 at 12:28:58PM -0500, Liam R. Howlett wrote:
> > * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [251113 05:45]:
> > > On Thu, Nov 13, 2025 at 12:04:19AM +0000, Matthew Wilcox wrote:
> > > > On Wed, Nov 12, 2025 at 03:06:38PM +0000, Lorenzo Stoakes wrote:
> > > > > > 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.
> > > > >
> > > > > Since we all missed it I do wonder if we need some super clear comment
> > > > > saying 'hey if you drop + re-acquire RCU lock you MUST revalidate mas state
> > > > > by doing 'blah'.
> > > >
> > > > I mean, this really isn't an RCU thing.  This is also bad:
> > > >
> > > > 	spin_lock(a);
> > > > 	p = *q;
> > > > 	spin_unlock(a);
> > > > 	spin_lock(a);
> > > > 	b = *p;
> > > >
> > > > p could have been freed while you didn't hold lock a.  Detecting this
> > > > kind of thing needs compiler assistence (ie Rust) to let you know that
> > > > you don't have the right to do that any more.
> > >
> > > Right but in your example the use of the pointers is _realy clear_. In the
> > > mas situation, the pointers are embedded in the helper struct, there's a
> > > state machine, etc. so it's harder to catch this.
> >
> > We could modify the above example to use a helper struct and the same
> > problem would arise...
> 
> I disagree.
> 
> It's a helper struct with a state machine, manipulated by API functions. In fact
> it turns out we _can_ recover this state even after dropping/reacquiring the
> lock by calling the appropriate API functions to do so.
> 
> You manipulate this state via mas_xxx() commands, and in fact we resolve the
> lock issue by issuing the correct one.
> 

The state is never recovered.. it's re-established entirely.

What is happening is, we are walking a tree data structure and keeping
tack of where we are by keeping a pointer to the node.  This node
remains stable as long as the rcu or write lock is held for the tree.

If you are not unlocking, you could see how keeping the node for
prev/next operations would be much faster.. it's just one pointer away.

When you drop whatever lock you are holding, that node may disappear,
which is what happened in this bug.

When you mas_reset() or mas_set() or mas_set_range(), then you are
setting the node in the maple state to MA_START.  Any operation you call
from then on will start over (get the root node and re-walk the tree).

So, calling the reset removes any potentially stale pointers but does
not restore any state.

mas_set()/mas_set_range() sets the index and last to what you are
looking for, which is part of the state.  The operations will set the
index/last to the range it finds on a search.  In the vma case, this
isn't very useful since we have vm_start/vm_end.

The state is re-established once you call the api to find something
again.

This is, imo, very close to having a vma in a helper struct, then
calling a function that drops the mmap lock, reacquires the lock, and
continues to use the vma.  The only way to restore the vma helper struct
to a safe state is to do the vma lookup again and replace the
(potentially) stale vma pointer.

If, say, for some reason, during copy_vma() we needed to drop the lock
after vma_merge_new_range().  We'd have to restore vmg->target to
whatever it was pointed to by vmg->start.. but vmg->start might not be
right if vmg->target was set to the previous vma.  We'd have to set
vmg->target = vma_lookup(vmg->orig_start) or such, then re-evaluate the
merging scenario.

I don't really see a difference in mas->node being invalid after a
locking dance vs vmg->target being invalid if there was a locking dance.
I also think it's fair to say that vma_merge_new_range() is an api that
copy_vma() is using.

I do see that hiding it in an API could be missed, but the API exists
because the mas struct is used in a lot of places that are in and around
locking like this.

I'll add to the documentation, but I suspect it won't really help.

...

Thanks,
Liam