linux-kernel - Re: [PATCH 1/1] mm/madvise: initialize prev pointer in madvise_walk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8aa7932b-e30d-4025-8503-726e01bd7539@lucifer.local>
Date: Tue, 17 Jun 2025 09:50:51 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Lance Yang <ioworker0@...il.com>, akpm@...ux-foundation.org,
        21cnbao@...il.com, Liam.Howlett@...cle.com, vbabka@...e.cz,
        jannh@...gle.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Lance Yang <lance.yang@...ux.dev>
Subject: Re: [PATCH 1/1] mm/madvise: initialize prev pointer in
 madvise_walk_vmas

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