[PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lance Yang]

From: Lance Yang <lance.yang@linux.dev>

The prev pointer was uninitialized, which could lead to undefined behavior
where its address is taken and passed to the visit() callback without being
assigned a value.

Initializing it to NULL makes the code safer and prevents potential bugs
if a future callback function attempts to read from it.

Signed-off-by: Lance Yang <lance.yang@linux.dev>
---
 mm/madvise.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 267d8e4adf31..c87325000303 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
 				   struct vm_area_struct **prev, unsigned long start,
 				   unsigned long end, void *arg))
 {
+	struct vm_area_struct *prev = NULL;
 	struct vm_area_struct *vma;
-	struct vm_area_struct *prev;
-	unsigned long tmp;
 	int unmapped_error = 0;
+	unsigned long tmp;
 	int error;
 
 	/*
-- 
2.49.0

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Barry Song]

On Tue, Jun 17, 2025 at 2:05 PM Lance Yang <ioworker0@gmail.com> wrote:
>
> From: Lance Yang <lance.yang@linux.dev>
>
> The prev pointer was uninitialized, which could lead to undefined behavior
> where its address is taken and passed to the visit() callback without being
> assigned a value.
>
> Initializing it to NULL makes the code safer and prevents potential bugs
> if a future callback function attempts to read from it.

Is there any read-before-write case here? I haven't found one.

It also looks like we're assuming that *prev == NULL implies
a specific condition:

*prev = NULL;   /* tell sys_madvise we drop mmap_lock */

*prev = NULL; /* mmap_lock has been dropped, prev is stale */

>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
>  mm/madvise.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 267d8e4adf31..c87325000303 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>                                    struct vm_area_struct **prev, unsigned long start,
>                                    unsigned long end, void *arg))
>  {
> +       struct vm_area_struct *prev = NULL;
>         struct vm_area_struct *vma;
> -       struct vm_area_struct *prev;
> -       unsigned long tmp;
>         int unmapped_error = 0;
> +       unsigned long tmp;
>         int error;
>
>         /*
> --
> 2.49.0
>

Thanks
Barry

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lance Yang]

On 2025/6/17 10:24, Barry Song wrote:
> On Tue, Jun 17, 2025 at 2:05 PM Lance Yang <ioworker0@gmail.com> wrote:
>>
>> From: Lance Yang <lance.yang@linux.dev>
>>
>> The prev pointer was uninitialized, which could lead to undefined behavior
>> where its address is taken and passed to the visit() callback without being
>> assigned a value.
>>
>> Initializing it to NULL makes the code safer and prevents potential bugs
>> if a future callback function attempts to read from it.
> 
> Is there any read-before-write case here? I haven't found one.


It appears that the following is a call chain showing the read-before-write
of prev:

-> madvise_vma_anon_name(..., struct vm_area_struct **prev, ...)
         Receives the address of madvise_walk_vmas's prev.
         Passes this pointer directly to madvise_update_vma.
         Note that prev is not updated before visit() is called
         if !(start > vma->vm_start) in the slow path.

         -> madvise_update_vma(..., struct vm_area_struct **prev, ...)
                 It calls the next function with *prev.

                 -> vma_modify_flags_name(..., *prev, ...)
                         Stores the value of madvise_walk_vmas's prev in 
vmg.prev
                         using the VMG_VMA_STATE macro.

                         -> vma_modify(struct vma_merge_struct *vmg)
                                 Receives the vmg struct.
                                 Passes vmg to vma_merge_existing_range.

                                 -> vma_merge_existing_range(struct 
vma_merge_struct *vmg)
                                         Retrieves the value: struct 
vm_area_struct *prev = vmg->prev;
                                         The value is now used in a 
conditional check:
                                         VM_WARN_ON_VMG(prev && start <= 
prev->vm_start, vmg)
                                         If prev was uninitialized, this 
would cause a crash.

Thanks,
Lance

> 
> It also looks like we're assuming that *prev == NULL implies
> a specific condition:
> 
> *prev = NULL;   /* tell sys_madvise we drop mmap_lock */
> 
> *prev = NULL; /* mmap_lock has been dropped, prev is stale */
> 
>>
>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>> ---
>>   mm/madvise.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 267d8e4adf31..c87325000303 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>>                                     struct vm_area_struct **prev, unsigned long start,
>>                                     unsigned long end, void *arg))
>>   {
>> +       struct vm_area_struct *prev = NULL;
>>          struct vm_area_struct *vma;
>> -       struct vm_area_struct *prev;
>> -       unsigned long tmp;
>>          int unmapped_error = 0;
>> +       unsigned long tmp;
>>          int error;
>>
>>          /*
>> --
>> 2.49.0
>>
> 
> Thanks
> Barry

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Barry Song]

On Tue, Jun 17, 2025 at 4:57 PM Lance Yang <lance.yang@linux.dev> wrote:
>
>
>
> On 2025/6/17 10:24, Barry Song wrote:
> > On Tue, Jun 17, 2025 at 2:05 PM Lance Yang <ioworker0@gmail.com> wrote:
> >>
> >> From: Lance Yang <lance.yang@linux.dev>
> >>
> >> The prev pointer was uninitialized, which could lead to undefined behavior
> >> where its address is taken and passed to the visit() callback without being
> >> assigned a value.
> >>
> >> Initializing it to NULL makes the code safer and prevents potential bugs
> >> if a future callback function attempts to read from it.
> >
> > Is there any read-before-write case here? I haven't found one.
>
>
> It appears that the following is a call chain showing the read-before-write
> of prev:
>
> -> madvise_vma_anon_name(..., struct vm_area_struct **prev, ...)
>          Receives the address of madvise_walk_vmas's prev.
>          Passes this pointer directly to madvise_update_vma.
>          Note that prev is not updated before visit() is called
>          if !(start > vma->vm_start) in the slow path.
>
>          -> madvise_update_vma(..., struct vm_area_struct **prev, ...)
>                  It calls the next function with *prev.
>
>                  -> vma_modify_flags_name(..., *prev, ...)
>                          Stores the value of madvise_walk_vmas's prev in
> vmg.prev
>                          using the VMG_VMA_STATE macro.
>
>                          -> vma_modify(struct vma_merge_struct *vmg)
>                                  Receives the vmg struct.
>                                  Passes vmg to vma_merge_existing_range.
>
>                                  -> vma_merge_existing_range(struct
> vma_merge_struct *vmg)
>                                          Retrieves the value: struct
> vm_area_struct *prev = vmg->prev;
>                                          The value is now used in a
> conditional check:
>                                          VM_WARN_ON_VMG(prev && start <=
> prev->vm_start, vmg)
>                                          If prev was uninitialized, this
> would cause a crash.

Thanks!

Do you have a reproducer? I'd like to try.

>
> Thanks,
> Lance
>
> >
> > It also looks like we're assuming that *prev == NULL implies
> > a specific condition:
> >
> > *prev = NULL;   /* tell sys_madvise we drop mmap_lock */
> >
> > *prev = NULL; /* mmap_lock has been dropped, prev is stale */
> >
> >>
> >> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> >> ---
> >>   mm/madvise.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/madvise.c b/mm/madvise.c
> >> index 267d8e4adf31..c87325000303 100644
> >> --- a/mm/madvise.c
> >> +++ b/mm/madvise.c
> >> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> >>                                     struct vm_area_struct **prev, unsigned long start,
> >>                                     unsigned long end, void *arg))
> >>   {
> >> +       struct vm_area_struct *prev = NULL;
> >>          struct vm_area_struct *vma;
> >> -       struct vm_area_struct *prev;
> >> -       unsigned long tmp;
> >>          int unmapped_error = 0;
> >> +       unsigned long tmp;
> >>          int error;
> >>
> >>          /*
> >> --
> >> 2.49.0
> >>
> >

Thanks
Barry

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lance Yang]

On 2025/6/17 13:19, Barry Song wrote:
> On Tue, Jun 17, 2025 at 4:57 PM Lance Yang <lance.yang@linux.dev> wrote:
>>
>>
>>
>> On 2025/6/17 10:24, Barry Song wrote:
>>> On Tue, Jun 17, 2025 at 2:05 PM Lance Yang <ioworker0@gmail.com> wrote:
>>>>
>>>> From: Lance Yang <lance.yang@linux.dev>
>>>>
>>>> The prev pointer was uninitialized, which could lead to undefined behavior
>>>> where its address is taken and passed to the visit() callback without being
>>>> assigned a value.
>>>>
>>>> Initializing it to NULL makes the code safer and prevents potential bugs
>>>> if a future callback function attempts to read from it.
>>>
>>> Is there any read-before-write case here? I haven't found one.
>>
>>
>> It appears that the following is a call chain showing the read-before-write
>> of prev:
>>
>> -> madvise_vma_anon_name(..., struct vm_area_struct **prev, ...)
>>           Receives the address of madvise_walk_vmas's prev.
>>           Passes this pointer directly to madvise_update_vma.
>>           Note that prev is not updated before visit() is called
>>           if !(start > vma->vm_start) in the slow path.
>>
>>           -> madvise_update_vma(..., struct vm_area_struct **prev, ...)
>>                   It calls the next function with *prev.
>>
>>                   -> vma_modify_flags_name(..., *prev, ...)
>>                           Stores the value of madvise_walk_vmas's prev in
>> vmg.prev
>>                           using the VMG_VMA_STATE macro.
>>
>>                           -> vma_modify(struct vma_merge_struct *vmg)
>>                                   Receives the vmg struct.
>>                                   Passes vmg to vma_merge_existing_range.
>>
>>                                   -> vma_merge_existing_range(struct
>> vma_merge_struct *vmg)
>>                                           Retrieves the value: struct
>> vm_area_struct *prev = vmg->prev;
>>                                           The value is now used in a
>> conditional check:
>>                                           VM_WARN_ON_VMG(prev && start <=
>> prev->vm_start, vmg)
>>                                           If prev was uninitialized, this
>> would cause a crash.
> 
> Thanks!
> 
> Do you have a reproducer? I'd like to try.

Not yet ;)

It was found during code review to prevent potential bugs, and the
initialization itself is harmless.

Thanks,
Lance

> 
>>
>> Thanks,
>> Lance
>>
>>>
>>> It also looks like we're assuming that *prev == NULL implies
>>> a specific condition:
>>>
>>> *prev = NULL;   /* tell sys_madvise we drop mmap_lock */
>>>
>>> *prev = NULL; /* mmap_lock has been dropped, prev is stale */
>>>
>>>>
>>>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>>>> ---
>>>>    mm/madvise.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/mm/madvise.c b/mm/madvise.c
>>>> index 267d8e4adf31..c87325000303 100644
>>>> --- a/mm/madvise.c
>>>> +++ b/mm/madvise.c
>>>> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>>>>                                      struct vm_area_struct **prev, unsigned long start,
>>>>                                      unsigned long end, void *arg))
>>>>    {
>>>> +       struct vm_area_struct *prev = NULL;
>>>>           struct vm_area_struct *vma;
>>>> -       struct vm_area_struct *prev;
>>>> -       unsigned long tmp;
>>>>           int unmapped_error = 0;
>>>> +       unsigned long tmp;
>>>>           int error;
>>>>
>>>>           /*
>>>> --
>>>> 2.49.0
>>>>
>>>
> 
> Thanks
> Barry

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[David Hildenbrand]

On 17.06.25 04:05, Lance Yang wrote:
> From: Lance Yang <lance.yang@linux.dev>
> 
> The prev pointer was uninitialized, which could lead to undefined behavior
> where its address is taken and passed to the visit() callback without being
> assigned a value.

So, we are passing the pointer value to visit(), which is not undefined 
behavior.

The issue would be if anybody takes a look at the value stored at that 
pointer. Because, already passing an uninitialized value to a 
(non-inlined) function is undefined behavior according to C.

In madvise_update_vma()->vma_modify_flags_name() we do exactly that, 
correct?

	vma = vma_modify_flags_name(&vmi, *prev, ...

We should use Fixes: then.


Acked-by: David Hildenbrand <david@redhat.com>

> 
> Initializing it to NULL makes the code safer and prevents potential bugs
> if a future callback function attempts to read from it.
> 
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
>   mm/madvise.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 267d8e4adf31..c87325000303 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>   				   struct vm_area_struct **prev, unsigned long start,
>   				   unsigned long end, void *arg))
>   {
> +	struct vm_area_struct *prev = NULL;
>   	struct vm_area_struct *vma;
> -	struct vm_area_struct *prev;
> -	unsigned long tmp;
>   	int unmapped_error = 0;
> +	unsigned long tmp;
>   	int error;
>   
>   	/*


-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lance Yang]

On 2025/6/17 15:54, David Hildenbrand wrote:
> On 17.06.25 04:05, Lance Yang wrote:
>> From: Lance Yang <lance.yang@linux.dev>
>>
>> The prev pointer was uninitialized, which could lead to undefined 
>> behavior
>> where its address is taken and passed to the visit() callback without 
>> being
>> assigned a value.
> 
> So, we are passing the pointer value to visit(), which is not undefined 
> behavior.
> 
> The issue would be if anybody takes a look at the value stored at that 
> pointer. Because, already passing an uninitialized value to a (non- 
> inlined) function is undefined behavior according to C.

Yes, that is precisely is what I am concerned about ;)

> 
> In madvise_update_vma()->vma_modify_flags_name() we do exactly that, 
> correct?
> 
>      vma = vma_modify_flags_name(&vmi, *prev, ...
> 
> We should use Fixes: then.

Exactly, I missed that. Will add the "Fixes" tag and send out a new version.

> 
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thanks!
Lance

> 
>>
>> Initializing it to NULL makes the code safer and prevents potential bugs
>> if a future callback function attempts to read from it.
>>
>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>> ---
>>   mm/madvise.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 267d8e4adf31..c87325000303 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, 
>> unsigned long start,
>>                      struct vm_area_struct **prev, unsigned long start,
>>                      unsigned long end, void *arg))
>>   {
>> +    struct vm_area_struct *prev = NULL;
>>       struct vm_area_struct *vma;
>> -    struct vm_area_struct *prev;
>> -    unsigned long tmp;
>>       int unmapped_error = 0;
>> +    unsigned long tmp;
>>       int error;
>>       /*
> 
> 

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 09:54:29AM +0200, David Hildenbrand wrote:
> On 17.06.25 04:05, Lance Yang wrote:
> > From: Lance Yang <lance.yang@linux.dev>
> >
> > The prev pointer was uninitialized, which could lead to undefined behavior
> > where its address is taken and passed to the visit() callback without being
> > assigned a value.
>
> So, we are passing the pointer value to visit(), which is not undefined
> behavior.
>
> The issue would be if anybody takes a look at the value stored at that
> pointer. Because, already passing an uninitialized value to a (non-inlined)
> function is undefined behavior according to C.
>
> In madvise_update_vma()->vma_modify_flags_name() we do exactly that,
> correct?

Err the parameter there is struct vm_area_struct **prev...

We deref to the prev ptr which is unassigned yes but the pointer to the pointer isn't...

>
> 	vma = vma_modify_flags_name(&vmi, *prev, ...
>
> We should use Fixes: then.

So no we shouldn't...

>
>
> Acked-by: David Hildenbrand <david@redhat.com>

Sure? :)

>
> >
> > Initializing it to NULL makes the code safer and prevents potential bugs
> > if a future callback function attempts to read from it.
> >
> > Signed-off-by: Lance Yang <lance.yang@linux.dev>
> > ---
> >   mm/madvise.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 267d8e4adf31..c87325000303 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> >   				   struct vm_area_struct **prev, unsigned long start,
> >   				   unsigned long end, void *arg))
> >   {
> > +	struct vm_area_struct *prev = NULL;
> >   	struct vm_area_struct *vma;
> > -	struct vm_area_struct *prev;
> > -	unsigned long tmp;
> >   	int unmapped_error = 0;
> > +	unsigned long tmp;
> >   	int error;
> >   	/*
>
>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[David Hildenbrand]

On 17.06.25 10:21, Lorenzo Stoakes wrote:
> On Tue, Jun 17, 2025 at 09:54:29AM +0200, David Hildenbrand wrote:
>> On 17.06.25 04:05, Lance Yang wrote:
>>> From: Lance Yang <lance.yang@linux.dev>
>>>
>>> The prev pointer was uninitialized, which could lead to undefined behavior
>>> where its address is taken and passed to the visit() callback without being
>>> assigned a value.
>>
>> So, we are passing the pointer value to visit(), which is not undefined
>> behavior.
>>
>> The issue would be if anybody takes a look at the value stored at that
>> pointer. Because, already passing an uninitialized value to a (non-inlined)
>> function is undefined behavior according to C.
>>
>> In madvise_update_vma()->vma_modify_flags_name() we do exactly that,
>> correct?
> 
> Err the parameter there is struct vm_area_struct **prev...
> 
> We deref to the prev ptr which is unassigned yes but the pointer to the pointer isn't...
> 

struct vm_area_struct *prev;

is uninitialized.

We pass &prev -> prevp, which now points at something uninitialized.

Doing "*prevp =" is fine, because we will initialize.

Doing "= *prep" is not fine, because the value was not initialized.

>>
>> 	vma = vma_modify_flags_name(&vmi, *prev, ...
>>
>> We should use Fixes: then.
> 
> So no we shouldn't...
> 
>>
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
> 
> Sure? :)

Unless I am missing something important, yes :)

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 10:28:58AM +0200, David Hildenbrand wrote:
> On 17.06.25 10:21, Lorenzo Stoakes wrote:
> > On Tue, Jun 17, 2025 at 09:54:29AM +0200, David Hildenbrand wrote:
> > > On 17.06.25 04:05, Lance Yang wrote:
> > > > From: Lance Yang <lance.yang@linux.dev>
> > > >
> > > > The prev pointer was uninitialized, which could lead to undefined behavior
> > > > where its address is taken and passed to the visit() callback without being
> > > > assigned a value.
> > >
> > > So, we are passing the pointer value to visit(), which is not undefined
> > > behavior.
> > >
> > > The issue would be if anybody takes a look at the value stored at that
> > > pointer. Because, already passing an uninitialized value to a (non-inlined)
> > > function is undefined behavior according to C.
> > >
> > > In madvise_update_vma()->vma_modify_flags_name() we do exactly that,
> > > correct?
> >
> > Err the parameter there is struct vm_area_struct **prev...
> >
> > We deref to the prev ptr which is unassigned yes but the pointer to the pointer isn't...
> >
>
> struct vm_area_struct *prev;
>
> is uninitialized.
>
> We pass &prev -> prevp, which now points at something uninitialized.
>
> Doing "*prevp =" is fine, because we will initialize.
>
> Doing "= *prep" is not fine, because the value was not initialized.

Yep sorry pre-coffee.

>
> > >
> > > 	vma = vma_modify_flags_name(&vmi, *prev, ...
> > >
> > > We should use Fixes: then.
> >
> > So no we shouldn't...
> >
> > >
> > >
> > > Acked-by: David Hildenbrand <david@redhat.com>
> >
> > Sure? :)
>
> Unless I am missing something important, yes :)

This solution isn't correct as prev == NULL when prev != NULL is wholly
incorrect.

We need a better solution.

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[David Hildenbrand]

>>>> 	vma = vma_modify_flags_name(&vmi, *prev, ...
>>>>
>>>> We should use Fixes: then.
>>>
>>> So no we shouldn't...
>>>
>>>>
>>>>
>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>
>>> Sure? :)
>>
>> Unless I am missing something important, yes :)
> 
> This solution isn't correct as prev == NULL when prev != NULL is wholly
> incorrect.

I am not able to understand what you mean :)

I assume you mean, that we reach a point down in the callchain, where 
"prev" is supposed to be set to something proper, but it would be "NULL".

That would indeed require a different fix.

I wonder why we didn't trigger this case so far?

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 10:38:07AM +0200, David Hildenbrand wrote:
> > > > > 	vma = vma_modify_flags_name(&vmi, *prev, ...
> > > > >
> > > > > We should use Fixes: then.
> > > >
> > > > So no we shouldn't...
> > > >
> > > > >
> > > > >
> > > > > Acked-by: David Hildenbrand <david@redhat.com>
> > > >
> > > > Sure? :)
> > >
> > > Unless I am missing something important, yes :)
> >
> > This solution isn't correct as prev == NULL when prev != NULL is wholly
> > incorrect.
>
> I am not able to understand what you mean :)
>
> I assume you mean, that we reach a point down in the callchain, where "prev"
> is supposed to be set to something proper, but it would be "NULL".

I mean if you tell merge code 'hey the previous VMA is NULL' (same thing as
saying 'hey this is the first VMA in the address space) and it isn't, bad things
will happen (TM).

>
> That would indeed require a different fix.

Yes this patch is wrong, sorry.

>
> I wonder why we didn't trigger this case so far?

It's because it only happens since Barry's per-VMA lock logic...

	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
		vma = try_vma_read_lock(mm, madv_behavior, start, end);
		if (vma) {
			error = visit(vma, &prev, start, end, arg);
			vma_end_read(vma);
			return error;
		}
	}

Otherwise, we look up the find_vma_prev():

	vma = find_vma_prev(mm, start, &prev);

In madvise_dontneed_free() we always set *prev = vma _first_.

Let me suggest the better fix to Lance higher in thread so he sees :)

Not sure if a fixes is valid here given this isn't mainline yet, more so this
should be squashed with barry's series?

>
> --
> Cheers,
>
> David / dhildenb
>
>

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[David Hildenbrand]

On 17.06.25 10:50, Lorenzo Stoakes wrote:
> On Tue, Jun 17, 2025 at 10:38:07AM +0200, David Hildenbrand wrote:
>>>>>> 	vma = vma_modify_flags_name(&vmi, *prev, ...
>>>>>>
>>>>>> We should use Fixes: then.
>>>>>
>>>>> So no we shouldn't...
>>>>>
>>>>>>
>>>>>>
>>>>>> Acked-by: David Hildenbrand <david@redhat.com>
>>>>>
>>>>> Sure? :)
>>>>
>>>> Unless I am missing something important, yes :)
>>>
>>> This solution isn't correct as prev == NULL when prev != NULL is wholly
>>> incorrect.
>>
>> I am not able to understand what you mean :)
>>
>> I assume you mean, that we reach a point down in the callchain, where "prev"
>> is supposed to be set to something proper, but it would be "NULL".
> 
> I mean if you tell merge code 'hey the previous VMA is NULL' (same thing as
> saying 'hey this is the first VMA in the address space) and it isn't, bad things
> will happen (TM).
> 
>>
>> That would indeed require a different fix.
> 
> Yes this patch is wrong, sorry.
> 
>>
>> I wonder why we didn't trigger this case so far?
> 
> It's because it only happens since Barry's per-VMA lock logic...
> 
> 	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
> 		vma = try_vma_read_lock(mm, madv_behavior, start, end);
> 		if (vma) {
> 			error = visit(vma, &prev, start, end, arg);
> 			vma_end_read(vma);
> 			return error;
> 		}
> 	}
> 
> Otherwise, we look up the find_vma_prev():
> 
> 	vma = find_vma_prev(mm, start, &prev);
> 
> In madvise_dontneed_free() we always set *prev = vma _first_.
> 
> Let me suggest the better fix to Lance higher in thread so he sees :)
> 
> Not sure if a fixes is valid here given this isn't mainline yet, more so this
> should be squashed with barry's series?

If it's not in mm-stable yet, it can still be squashed, yes.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 09:54:29AM +0200, David Hildenbrand wrote:
> On 17.06.25 04:05, Lance Yang wrote:
> > From: Lance Yang <lance.yang@linux.dev>
> >
> > The prev pointer was uninitialized, which could lead to undefined behavior
> > where its address is taken and passed to the visit() callback without being
> > assigned a value.
>
> So, we are passing the pointer value to visit(), which is not undefined
> behavior.
>
> The issue would be if anybody takes a look at the value stored at that
> pointer. Because, already passing an uninitialized value to a (non-inlined)
> function is undefined behavior according to C.
>
> In madvise_update_vma()->vma_modify_flags_name() we do exactly that,
> correct?
>
> 	vma = vma_modify_flags_name(&vmi, *prev, ...
>
> We should use Fixes: then.

A note if people were hoping to blame 94d7d9233951, well before that patch we
had:

-	*prev = vma_merge(&vmi, mm, *prev, start, end, new_flags,
-			  vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
-			  vma->vm_userfaultfd_ctx, anon_name);

Note the *prev...

>
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> >
> > Initializing it to NULL makes the code safer and prevents potential bugs
> > if a future callback function attempts to read from it.
> >
> > Signed-off-by: Lance Yang <lance.yang@linux.dev>
> > ---
> >   mm/madvise.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 267d8e4adf31..c87325000303 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> >   				   struct vm_area_struct **prev, unsigned long start,
> >   				   unsigned long end, void *arg))
> >   {
> > +	struct vm_area_struct *prev = NULL;
> >   	struct vm_area_struct *vma;
> > -	struct vm_area_struct *prev;
> > -	unsigned long tmp;
> >   	int unmapped_error = 0;
> > +	unsigned long tmp;
> >   	int error;
> >   	/*
>
>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 09:43:06AM +0100, Lorenzo Stoakes wrote:
> On Tue, Jun 17, 2025 at 09:54:29AM +0200, David Hildenbrand wrote:
> > On 17.06.25 04:05, Lance Yang wrote:
> > > From: Lance Yang <lance.yang@linux.dev>
> > >
> > > The prev pointer was uninitialized, which could lead to undefined behavior
> > > where its address is taken and passed to the visit() callback without being
> > > assigned a value.
> >
> > So, we are passing the pointer value to visit(), which is not undefined
> > behavior.
> >
> > The issue would be if anybody takes a look at the value stored at that
> > pointer. Because, already passing an uninitialized value to a (non-inlined)
> > function is undefined behavior according to C.
> >
> > In madvise_update_vma()->vma_modify_flags_name() we do exactly that,
> > correct?
> >
> > 	vma = vma_modify_flags_name(&vmi, *prev, ...
> >
> > We should use Fixes: then.
>
> A note if people were hoping to blame 94d7d9233951, well before that patch we
> had:
>
> -	*prev = vma_merge(&vmi, mm, *prev, start, end, new_flags,
> -			  vma->anon_vma, vma->vm_file, pgoff, vma_policy(vma),
> -			  vma->vm_userfaultfd_ctx, anon_name);
>
> Note the *prev...

Oops I didn't mean to send this one ;)

>
> >
> >
> > Acked-by: David Hildenbrand <david@redhat.com>
> >
> > >
> > > Initializing it to NULL makes the code safer and prevents potential bugs
> > > if a future callback function attempts to read from it.
> > >
> > > Signed-off-by: Lance Yang <lance.yang@linux.dev>
> > > ---
> > >   mm/madvise.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 267d8e4adf31..c87325000303 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> > >   				   struct vm_area_struct **prev, unsigned long start,
> > >   				   unsigned long end, void *arg))
> > >   {
> > > +	struct vm_area_struct *prev = NULL;
> > >   	struct vm_area_struct *vma;
> > > -	struct vm_area_struct *prev;
> > > -	unsigned long tmp;
> > >   	int unmapped_error = 0;
> > > +	unsigned long tmp;
> > >   	int error;
> > >   	/*
> >
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 10:05:43AM +0800, Lance Yang wrote:
> From: Lance Yang <lance.yang@linux.dev>
>
> The prev pointer was uninitialized, which could lead to undefined behavior
> where its address is taken and passed to the visit() callback without being
> assigned a value.
>
> Initializing it to NULL makes the code safer and prevents potential bugs
> if a future callback function attempts to read from it.

Well, no it doesn't, it prevents only one (easily caught by kasan setc.)
class of bug - that is accessing uninitialised data.

But is prev always NULL? It definitely isn't. So we're now basically
introducing a new kind of bug, one that won't get picked up anywhere near
as easily.

And yes this whole thing sucks.

We can't be arbitrarily walking the maple tree to get the actual prev
either as it's inefficient.

>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
>  mm/madvise.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 267d8e4adf31..c87325000303 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>  				   struct vm_area_struct **prev, unsigned long start,
>  				   unsigned long end, void *arg))

Is this patch broken? This seems to be chopping bits off, or maybe it's
because the @@ line shows the start of the decl?

Makes it look like prev is being shadowed...

Actual decl:

static
int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
		      unsigned long end, struct madvise_behavior *madv_behavior,
		      void *arg,
		      int (*visit)(struct vm_area_struct *vma,
				   struct vm_area_struct **prev, unsigned long start,
				   unsigned long end, void *arg))

>  {
> +	struct vm_area_struct *prev = NULL;
>  	struct vm_area_struct *vma;
> -	struct vm_area_struct *prev;

As far as I know there's not a case where this matters.

But it does suck to be passing around a pointer to uninitialised state.

> -	unsigned long tmp;
>  	int unmapped_error = 0;
> +	unsigned long tmp;

Not sure why you're fiddling with this?

>  	int error;
>
>  	/*
> --
> 2.49.0
>

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

Lace - To simplify and not get bogged down in sub-threads am replying at the top
level.

TL;DR this fix is incorrect, but the issue is correct :)

So the patch at [0] introduced by Barry changed things in a way that _appears_
broken but in fact aren't, however we should do something about this, obviously.

That patch added:

	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
		vma = try_vma_read_lock(mm, madv_behavior, start, end);
		if (vma) {
			error = visit(vma, &prev, start, end, arg);
			vma_end_read(vma);
			return error;
		}
	}

And the problem is, in this case, we don't initialise prev.

In all other cases, we do (under mmap lock):

	vma = find_vma_prev(mm, start, &prev);
	if (vma && start > vma->vm_start)
		prev = vma;

The reason this isn't a problem is that the only madvise operation that
currently supports this, madvise_dontneed_free() will initialise *prev = vma.

BUT we really shouldn't be relying on this, so I attach a fixpatch.

Given Barry's patch isn't mainline yet, I think this should just be squashed
into that as a fix?

It kind of sucks to do this, but it resolves any potential bug.

I think a follow up is needed, as there's an implicit assumption it seems that
prev is updated immediately for most callers, but of course anon_vma_name is a
special snowflake.

todos++;

Lance - I suggest you reply to Barry's series with the below fix, or I can if
you prefer?

[0]: https://lore.kernel.org/linux-mm/20250607220150.2980-1-21cnbao@gmail.com/

Thanks!

On Tue, Jun 17, 2025 at 10:05:43AM +0800, Lance Yang wrote:
> From: Lance Yang <lance.yang@linux.dev>
>
> The prev pointer was uninitialized, which could lead to undefined behavior
> where its address is taken and passed to the visit() callback without being
> assigned a value.
>
> Initializing it to NULL makes the code safer and prevents potential bugs
> if a future callback function attempts to read from it.
>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
>  mm/madvise.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 267d8e4adf31..c87325000303 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>  				   struct vm_area_struct **prev, unsigned long start,
>  				   unsigned long end, void *arg))
>  {
> +	struct vm_area_struct *prev = NULL;
>  	struct vm_area_struct *vma;
> -	struct vm_area_struct *prev;
> -	unsigned long tmp;
>  	int unmapped_error = 0;
> +	unsigned long tmp;
>  	int error;
>
>  	/*
> --
> 2.49.0
>

----8<----
From c8dc9f5b2929e389cac44b79201fff43e0ab8195 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Tue, 17 Jun 2025 09:46:27 +0100
Subject: [PATCH] fix

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/madvise.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/madvise.c b/mm/madvise.c
index 267d8e4adf31..45ea4588e34e 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1549,6 +1549,7 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
 	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
 		vma = try_vma_read_lock(mm, madv_behavior, start, end);
 		if (vma) {
+			*prev = vma;
 			error = visit(vma, &prev, start, end, arg);
 			vma_end_read(vma);
 			return error;
--
2.49.0

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lance Yang]

On 2025/6/17 16:50, Lorenzo Stoakes wrote:
> Lace - To simplify and not get bogged down in sub-threads am replying at the top
> level.
> 
> TL;DR this fix is incorrect, but the issue is correct :)
> 
> So the patch at [0] introduced by Barry changed things in a way that _appears_
> broken but in fact aren't, however we should do something about this, obviously.
> 
> That patch added:
> 
> 	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
> 		vma = try_vma_read_lock(mm, madv_behavior, start, end);
> 		if (vma) {
> 			error = visit(vma, &prev, start, end, arg);
> 			vma_end_read(vma);
> 			return error;
> 		}
> 	}
> 
> And the problem is, in this case, we don't initialise prev.
> 
> In all other cases, we do (under mmap lock):
> 
> 	vma = find_vma_prev(mm, start, &prev);
> 	if (vma && start > vma->vm_start)
> 		prev = vma;
> 
> The reason this isn't a problem is that the only madvise operation that
> currently supports this, madvise_dontneed_free() will initialise *prev = vma.
> 
> BUT we really shouldn't be relying on this, so I attach a fixpatch.
> 
> Given Barry's patch isn't mainline yet, I think this should just be squashed
> into that as a fix?
> 
> It kind of sucks to do this, but it resolves any potential bug.
> 
> I think a follow up is needed, as there's an implicit assumption it seems that
> prev is updated immediately for most callers, but of course anon_vma_name is a
> special snowflake.
> 
> todos++;

Ah, please keep me in the loop ;)

> 
> Lance - I suggest you reply to Barry's series with the below fix, or I can if
> you prefer?

Sure, go ahead!

Thanks,
Lance

> 
> [0]: https://lore.kernel.org/linux-mm/20250607220150.2980-1-21cnbao@gmail.com/
> 
> Thanks!
> 
> On Tue, Jun 17, 2025 at 10:05:43AM +0800, Lance Yang wrote:
>> From: Lance Yang <lance.yang@linux.dev>
>>
>> The prev pointer was uninitialized, which could lead to undefined behavior
>> where its address is taken and passed to the visit() callback without being
>> assigned a value.
>>
>> Initializing it to NULL makes the code safer and prevents potential bugs
>> if a future callback function attempts to read from it.
>>
>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>> ---
>>   mm/madvise.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 267d8e4adf31..c87325000303 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>>   				   struct vm_area_struct **prev, unsigned long start,
>>   				   unsigned long end, void *arg))
>>   {
>> +	struct vm_area_struct *prev = NULL;
>>   	struct vm_area_struct *vma;
>> -	struct vm_area_struct *prev;
>> -	unsigned long tmp;
>>   	int unmapped_error = 0;
>> +	unsigned long tmp;
>>   	int error;
>>
>>   	/*
>> --
>> 2.49.0
>>
> 
> ----8<----
>  From c8dc9f5b2929e389cac44b79201fff43e0ab8195 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Date: Tue, 17 Jun 2025 09:46:27 +0100
> Subject: [PATCH] fix
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>   mm/madvise.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 267d8e4adf31..45ea4588e34e 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1549,6 +1549,7 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
>   	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
>   		vma = try_vma_read_lock(mm, madv_behavior, start, end);
>   		if (vma) {
> +			*prev = vma;
>   			error = visit(vma, &prev, start, end, arg);
>   			vma_end_read(vma);
>   			return error;
> --
> 2.49.0

Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk_vmas

[Lorenzo Stoakes]

On Tue, Jun 17, 2025 at 05:21:03PM +0800, Lance Yang wrote:
>
>
> On 2025/6/17 16:50, Lorenzo Stoakes wrote:
> > Lace - To simplify and not get bogged down in sub-threads am replying at the top
> > level.

Firstly let me profusely apologise for calling you 'Lace' LOL! ;)

Busy morning and typos are happening...

> >
> > TL;DR this fix is incorrect, but the issue is correct :)
> >
> > So the patch at [0] introduced by Barry changed things in a way that _appears_
> > broken but in fact aren't, however we should do something about this, obviously.
> >
> > That patch added:
> >
> > 	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
> > 		vma = try_vma_read_lock(mm, madv_behavior, start, end);
> > 		if (vma) {
> > 			error = visit(vma, &prev, start, end, arg);
> > 			vma_end_read(vma);
> > 			return error;
> > 		}
> > 	}
> >
> > And the problem is, in this case, we don't initialise prev.
> >
> > In all other cases, we do (under mmap lock):
> >
> > 	vma = find_vma_prev(mm, start, &prev);
> > 	if (vma && start > vma->vm_start)
> > 		prev = vma;
> >
> > The reason this isn't a problem is that the only madvise operation that
> > currently supports this, madvise_dontneed_free() will initialise *prev = vma.
> >
> > BUT we really shouldn't be relying on this, so I attach a fixpatch.
> >
> > Given Barry's patch isn't mainline yet, I think this should just be squashed
> > into that as a fix?
> >
> > It kind of sucks to do this, but it resolves any potential bug.
> >
> > I think a follow up is needed, as there's an implicit assumption it seems that
> > prev is updated immediately for most callers, but of course anon_vma_name is a
> > special snowflake.
> >
> > todos++;
>
> Ah, please keep me in the loop ;)

Will do! Thanks for finding this issue, is appreciated!

>
> >
> > Lance - I suggest you reply to Barry's series with the below fix, or I can if
> > you prefer?
>
> Sure, go ahead!

Thanks will chase up!

Cheers, Lorenzo

>
> Thanks,
> Lance
>
> >
> > [0]: https://lore.kernel.org/linux-mm/20250607220150.2980-1-21cnbao@gmail.com/
> >
> > Thanks!
> >
> > On Tue, Jun 17, 2025 at 10:05:43AM +0800, Lance Yang wrote:
> > > From: Lance Yang <lance.yang@linux.dev>
> > >
> > > The prev pointer was uninitialized, which could lead to undefined behavior
> > > where its address is taken and passed to the visit() callback without being
> > > assigned a value.
> > >
> > > Initializing it to NULL makes the code safer and prevents potential bugs
> > > if a future callback function attempts to read from it.
> > >
> > > Signed-off-by: Lance Yang <lance.yang@linux.dev>
> > > ---
> > >   mm/madvise.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 267d8e4adf31..c87325000303 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -1536,10 +1536,10 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> > >   				   struct vm_area_struct **prev, unsigned long start,
> > >   				   unsigned long end, void *arg))
> > >   {
> > > +	struct vm_area_struct *prev = NULL;
> > >   	struct vm_area_struct *vma;
> > > -	struct vm_area_struct *prev;
> > > -	unsigned long tmp;
> > >   	int unmapped_error = 0;
> > > +	unsigned long tmp;
> > >   	int error;
> > >
> > >   	/*
> > > --
> > > 2.49.0
> > >
> >
> > ----8<----
> >  From c8dc9f5b2929e389cac44b79201fff43e0ab8195 Mon Sep 17 00:00:00 2001
> > From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > Date: Tue, 17 Jun 2025 09:46:27 +0100
> > Subject: [PATCH] fix
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >   mm/madvise.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 267d8e4adf31..45ea4588e34e 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1549,6 +1549,7 @@ int madvise_walk_vmas(struct mm_struct *mm, unsigned long start,
> >   	if (madv_behavior && madv_behavior->lock_mode == MADVISE_VMA_READ_LOCK) {
> >   		vma = try_vma_read_lock(mm, madv_behavior, start, end);
> >   		if (vma) {
> > +			*prev = vma;
> >   			error = visit(vma, &prev, start, end, arg);
> >   			vma_end_read(vma);
> >   			return error;
> > --
> > 2.49.0
>