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