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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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