[PATCH] lib/test_vmalloc.c: avoid garbage in page array

[PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Lorenzo Stoakes]

It turns out that alloc_pages_bulk_array() does not treat the page_array
parameter as an output parameter, but rather reads the array and skips any
entries that have already been allocated.

This is somewhat unexpected and breaks this test, as we allocate the pages
array uninitialised on the assumption it will be overwritten.

As a result, the test was referencing uninitialised data and causing the
PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
and panic.

In addition, this is an array of pointers not of struct page objects, so we
need only allocate an array with elements of pointer size.

We solve both problems by simply using kcalloc() and referencing
sizeof(struct page *) rather than sizeof(struct page).

Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
 lib/test_vmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
index 9dd9745d365f..3718d9886407 100644
--- a/lib/test_vmalloc.c
+++ b/lib/test_vmalloc.c
@@ -369,7 +369,7 @@ vm_map_ram_test(void)
 	int i;
 
 	map_nr_pages = nr_pages > 0 ? nr_pages:1;
-	pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
+	pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);
 	if (!pages)
 		return -1;
 
-- 
2.40.1

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Uladzislau Rezki]

On Wed, May 24, 2023 at 10:24 AM Lorenzo Stoakes <lstoakes@gmail.com> wrote:
>
> It turns out that alloc_pages_bulk_array() does not treat the page_array
> parameter as an output parameter, but rather reads the array and skips any
> entries that have already been allocated.
>
> This is somewhat unexpected and breaks this test, as we allocate the pages
> array uninitialised on the assumption it will be overwritten.
>
> As a result, the test was referencing uninitialised data and causing the
> PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
> and panic.
>
> In addition, this is an array of pointers not of struct page objects, so we
> need only allocate an array with elements of pointer size.
>
> We solve both problems by simply using kcalloc() and referencing
> sizeof(struct page *) rather than sizeof(struct page).
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  lib/test_vmalloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> index 9dd9745d365f..3718d9886407 100644
> --- a/lib/test_vmalloc.c
> +++ b/lib/test_vmalloc.c
> @@ -369,7 +369,7 @@ vm_map_ram_test(void)
>         int i;
>
>         map_nr_pages = nr_pages > 0 ? nr_pages:1;
> -       pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
> +       pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);
>         if (!pages)
>                 return -1;
>
> --
> 2.40.1
>
Uh.. :)

Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Baoquan He]

On 05/24/23 at 09:24am, Lorenzo Stoakes wrote:
> It turns out that alloc_pages_bulk_array() does not treat the page_array
> parameter as an output parameter, but rather reads the array and skips any
> entries that have already been allocated.

I read __alloc_pages_bulk() carefully, it does store the allocated page
pointers into page_array[] and pass out, not just reads the array and
skips entry alreay allocated.

For the issue this patch is trying to fix, you mean __alloc_pages_bulk()
doesn't initialize page_array intentionally if it doesn't successfully
allocate desired number of pages. we may need add one sentence to notice
user that page_array need be initialized explicitly.

By the way, could you please tell in which line the test was referencing
uninitialized data and causing the PFN to not be valid and trigger the
WANR_ON? Please forgive my dumb head.
> 
> This is somewhat unexpected and breaks this test, as we allocate the pages
> array uninitialised on the assumption it will be overwritten.
> 
> As a result, the test was referencing uninitialised data and causing the
> PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
> and panic.
> 
> In addition, this is an array of pointers not of struct page objects, so we
> need only allocate an array with elements of pointer size.
> 
> We solve both problems by simply using kcalloc() and referencing
> sizeof(struct page *) rather than sizeof(struct page).
> 
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  lib/test_vmalloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> index 9dd9745d365f..3718d9886407 100644
> --- a/lib/test_vmalloc.c
> +++ b/lib/test_vmalloc.c
> @@ -369,7 +369,7 @@ vm_map_ram_test(void)
>  	int i;
>  
>  	map_nr_pages = nr_pages > 0 ? nr_pages:1;
> -	pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
> +	pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);
>  	if (!pages)
>  		return -1;
>  
> -- 
> 2.40.1
> 

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Lorenzo Stoakes]

On Fri, May 26, 2023 at 08:08:33AM +0800, Baoquan He wrote:
> On 05/24/23 at 09:24am, Lorenzo Stoakes wrote:
> > It turns out that alloc_pages_bulk_array() does not treat the page_array
> > parameter as an output parameter, but rather reads the array and skips any
> > entries that have already been allocated.
>
> I read __alloc_pages_bulk() carefully, it does store the allocated page
> pointers into page_array[] and pass out, not just reads the array and
> skips entry alreay allocated.

Umm, the function literally opens with:-

	/*
	 * Skip populated array elements to determine if any pages need
	 * to be allocated before disabling IRQs.
	 */
	while (page_array && nr_populated < nr_pages && page_array[nr_populated])
		nr_populated++;

And then later:-

		/* Skip existing pages */
		if (page_array && page_array[nr_populated]) {
			nr_populated++;
			continue;
		}

This explicitly skips populated array entries and reads page_array to see
if entries already exist, and literally documents this in the comments
above each line, exactly as I describe.

>
> For the issue this patch is trying to fix, you mean __alloc_pages_bulk()
> doesn't initialize page_array intentionally if it doesn't successfully
> allocate desired number of pages. we may need add one sentence to notice
> user that page_array need be initialized explicitly.

It isn't 'trying' to fix it, it fixes it. I have this reproing locally.

What you're stating about 'successfully allocate desired number of pages'
is irrelevant, we literally check the number of allocated pages in the
caller.

No sentences need to be added, I explicitly state that the issue is due to
the array being uninitialised, the summary lines talks about reading
garbage.

>
> By the way, could you please tell in which line the test was referencing
> uninitialized data and causing the PFN to not be valid and trigger the
> WANR_ON? Please forgive my dumb head.

Well, I showed you the lines above where __alloc_bulk_array() is accessing
uninitialised data by reading page_array[].

But ultimately this is called from vm_map_ram_test() in lib/test_vmalloc.c:-

	for (i = 0; i < test_loop_count; i++) {
		v_ptr = vm_map_ram(pages, map_nr_pages, NUMA_NO_NODE);
^--- triggers warning because we can't map the invalid PFN
		*v_ptr = 'a';
^--- NULL pointer deref
		vm_unmap_ram(v_ptr, map_nr_pages);
	}

The warning is triggered in:-

vm_map_ram()
vmap_pages_range()
vmap_pages_range_noflush()
__vmap_pages_range_noflush()
vmap_pages_p4d_range()
vmap_pages_pud_range()
vmap_pages_pmd_range()
vmap_pages_pte_range()

In:-

		if (WARN_ON(!pfn_valid(page_to_pfn(page))))
			return -EINVAL;

The PFN is invalid because I happen to have garbage in an array entry such
that page_to_pfn(garbage) >= max_pfn.

> >
> > This is somewhat unexpected and breaks this test, as we allocate the pages
> > array uninitialised on the assumption it will be overwritten.
> >
> > As a result, the test was referencing uninitialised data and causing the
> > PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
> > and panic.
> >
> > In addition, this is an array of pointers not of struct page objects, so we
> > need only allocate an array with elements of pointer size.
> >
> > We solve both problems by simply using kcalloc() and referencing
> > sizeof(struct page *) rather than sizeof(struct page).
> >
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > ---
> >  lib/test_vmalloc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> > index 9dd9745d365f..3718d9886407 100644
> > --- a/lib/test_vmalloc.c
> > +++ b/lib/test_vmalloc.c
> > @@ -369,7 +369,7 @@ vm_map_ram_test(void)
> >  	int i;
> >
> >  	map_nr_pages = nr_pages > 0 ? nr_pages:1;
> > -	pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
> > +	pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);
> >  	if (!pages)
> >  		return -1;
> >
> > --
> > 2.40.1
> >
>

A broader problem we might want to think about is how little anybody is
running this test in order that it wasn't picked up before now... obviously
there's an element of luck as to whether the page_array happens to be
zeroed or not, but you'd think it'd be garbage filled at least a reasonable
amount of the time.

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Baoquan He]

On 05/26/23 at 08:13am, Lorenzo Stoakes wrote:
> On Fri, May 26, 2023 at 08:08:33AM +0800, Baoquan He wrote:
> > On 05/24/23 at 09:24am, Lorenzo Stoakes wrote:
> > > It turns out that alloc_pages_bulk_array() does not treat the page_array
> > > parameter as an output parameter, but rather reads the array and skips any
> > > entries that have already been allocated.
> >
> > I read __alloc_pages_bulk() carefully, it does store the allocated page
> > pointers into page_array[] and pass out, not just reads the array and
> > skips entry alreay allocated.
> 
> Umm, the function literally opens with:-
> 
> 	/*
> 	 * Skip populated array elements to determine if any pages need
> 	 * to be allocated before disabling IRQs.
> 	 */
> 	while (page_array && nr_populated < nr_pages && page_array[nr_populated])
> 		nr_populated++;

OK, suppose page_array[] alreasy has three pages populated, if not
initialized and there's garbage data in page_array[], it could have
nr_populated > 3 finally? This is really risky.

> 
> And then later:-
> 
> 		/* Skip existing pages */
> 		if (page_array && page_array[nr_populated]) {
> 			nr_populated++;
> 			continue;
> 		}

This is interesting, I thought this place of nr_populated checking and
updating is meaningless, in fact it's skipping the element with vlaue
in the middle of page_array. I realize this when I recheck the code when
replying to your mail. Not sure if we should restrict that, or it's
really existing reasonablly.

[x][x][x][][][][x][x][][]
x marks the element pointing to page.

> 
> This explicitly skips populated array entries and reads page_array to see
> if entries already exist, and literally documents this in the comments
> above each line, exactly as I describe.

OK, I misread your words in log. While page_array[] is still output
parameter, just not pure output parameter? Not sure if I understand
output parameter correctly.

> 
> >
> > For the issue this patch is trying to fix, you mean __alloc_pages_bulk()
> > doesn't initialize page_array intentionally if it doesn't successfully
> > allocate desired number of pages. we may need add one sentence to notice
> > user that page_array need be initialized explicitly.
> 
> It isn't 'trying' to fix it, it fixes it. I have this reproing locally.

Right, my wrong expression.

> 
> What you're stating about 'successfully allocate desired number of pages'
> is irrelevant, we literally check the number of allocated pages in the
> caller.
> 
> No sentences need to be added, I explicitly state that the issue is due to
> the array being uninitialised, the summary lines talks about reading
> garbage.

Well, I meant adding sentence above __alloc_pages_bulk() to tell:
page_array[] could have garbage data stored if you don't initialize
it explicitly before calling __alloc_pages_bulk();

This could happen in other place if they don't use kcalloc(),
kmalloc(GFP_ZERO) or something like this to allocate page_array[]?

> 
> >
> > By the way, could you please tell in which line the test was referencing
> > uninitialized data and causing the PFN to not be valid and trigger the
> > WANR_ON? Please forgive my dumb head.
> 
> Well, I showed you the lines above where __alloc_bulk_array() is accessing
> uninitialised data by reading page_array[].

I see now, thanks for these details.

> 
> But ultimately this is called from vm_map_ram_test() in lib/test_vmalloc.c:-
> 
> 	for (i = 0; i < test_loop_count; i++) {
> 		v_ptr = vm_map_ram(pages, map_nr_pages, NUMA_NO_NODE);
> ^--- triggers warning because we can't map the invalid PFN
> 		*v_ptr = 'a';
> ^--- NULL pointer deref
> 		vm_unmap_ram(v_ptr, map_nr_pages);
> 	}
> 
> The warning is triggered in:-
> 
> vm_map_ram()
> vmap_pages_range()
> vmap_pages_range_noflush()
> __vmap_pages_range_noflush()
> vmap_pages_p4d_range()
> vmap_pages_pud_range()
> vmap_pages_pmd_range()
> vmap_pages_pte_range()
> 
> In:-
> 
> 		if (WARN_ON(!pfn_valid(page_to_pfn(page))))
> 			return -EINVAL;
> 
> The PFN is invalid because I happen to have garbage in an array entry such
> that page_to_pfn(garbage) >= max_pfn.
> 
> > >
> > > This is somewhat unexpected and breaks this test, as we allocate the pages
> > > array uninitialised on the assumption it will be overwritten.
> > >
> > > As a result, the test was referencing uninitialised data and causing the
> > > PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
> > > and panic.
> > >
> > > In addition, this is an array of pointers not of struct page objects, so we
> > > need only allocate an array with elements of pointer size.
> > >
> > > We solve both problems by simply using kcalloc() and referencing
> > > sizeof(struct page *) rather than sizeof(struct page).
> > >
> > > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > > ---
> > >  lib/test_vmalloc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> > > index 9dd9745d365f..3718d9886407 100644
> > > --- a/lib/test_vmalloc.c
> > > +++ b/lib/test_vmalloc.c
> > > @@ -369,7 +369,7 @@ vm_map_ram_test(void)
> > >  	int i;
> > >
> > >  	map_nr_pages = nr_pages > 0 ? nr_pages:1;
> > > -	pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
> > > +	pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);
> > >  	if (!pages)
> > >  		return -1;
> > >
> > > --
> > > 2.40.1
> > >
> >
> 
> A broader problem we might want to think about is how little anybody is
> running this test in order that it wasn't picked up before now... obviously
> there's an element of luck as to whether the page_array happens to be
> zeroed or not, but you'd think it'd be garbage filled at least a reasonable
> amount of the time.

Hmm, that's why we may need notice people that there's risk in
__alloc_pages_bulk() if page_array[] is not initialized and the garbage
could be mistaken as a effective page pointer. My personal opinion.
People may argue it's caller's responsibility to do that.

Thanks
Baoquan

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Lorenzo Stoakes]

On Fri, May 26, 2023 at 04:56:49PM +0800, Baoquan He wrote:
> > Umm, the function literally opens with:-
> >
> > 	/*
> > 	 * Skip populated array elements to determine if any pages need
> > 	 * to be allocated before disabling IRQs.
> > 	 */
> > 	while (page_array && nr_populated < nr_pages && page_array[nr_populated])
> > 		nr_populated++;
>
> OK, suppose page_array[] alreasy has three pages populated, if not
> initialized and there's garbage data in page_array[], it could have
> nr_populated > 3 finally? This is really risky.
>
> >
> > And then later:-
> >
> > 		/* Skip existing pages */
> > 		if (page_array && page_array[nr_populated]) {
> > 			nr_populated++;
> > 			continue;
> > 		}
>
> This is interesting, I thought this place of nr_populated checking and
> updating is meaningless, in fact it's skipping the element with vlaue
> in the middle of page_array. I realize this when I recheck the code when
> replying to your mail. Not sure if we should restrict that, or it's
> really existing reasonablly.
>
> [x][x][x][][][][x][x][][]
> x marks the element pointing to page.

All of this is fine, the caller is expected to provide a zeroed array or an
array that contains existing elements. We only need to use it with a zeroed
array. We zero the array. Problem solved. Other users use the 'already
allocated pages' functionality, we don't care.

>
> >
> > This explicitly skips populated array entries and reads page_array to see
> > if entries already exist, and literally documents this in the comments
> > above each line, exactly as I describe.
>
> OK, I misread your words in log. While page_array[] is still output
> parameter, just not pure output parameter? Not sure if I understand
> output parameter correctly.

Well, output implies output i.e. writing to something. If you also read it,
it's not just an output parameter is it?

I don't really want to get into semantics here, the point is the test's
expectation is that it'd be write-only and it's not so we have to zero the
array, that's it.

>
> Well, I meant adding sentence above __alloc_pages_bulk() to tell:
> page_array[] could have garbage data stored if you don't initialize
> it explicitly before calling __alloc_pages_bulk();
>

As I said I literally state in multiple places this is about needing to
initialise the array:-

    lib/test_vmalloc.c: avoid garbage in page array

...

    This is somewhat unexpected and breaks this test, as we allocate the pages
    array uninitialised on the assumption it will be overwritten.

...

    We solve both problems by simply using kcalloc() and referencing
    sizeof(struct page *) rather than sizeof(struct page).

So I completely disagree we need to add anything more.

> This could happen in other place if they don't use kcalloc(),
> kmalloc(GFP_ZERO) or something like this to allocate page_array[]?

We don't care? I'm fixing a test here not auditing __alloc_bulk_array().

> > A broader problem we might want to think about is how little anybody is
> > running this test in order that it wasn't picked up before now... obviously
> > there's an element of luck as to whether the page_array happens to be
> > zeroed or not, but you'd think it'd be garbage filled at least a reasonable
> > amount of the time.
>
> Hmm, that's why we may need notice people that there's risk in
> __alloc_pages_bulk() if page_array[] is not initialized and the garbage
> could be mistaken as a effective page pointer. My personal opinion.
> People may argue it's caller's responsibility to do that.
>
> Thanks
> Baoquan
>

That's just irrelevant to this change. You're also not replying to my point here
(that we're clearly not running this test very much).

I find this review super bikesheddy. Let's try not to bog things down in
lengthily discussions when literally all I am doing here is:-

1. Function expects a zeroed array
2. Change the code to zero the array
3. Change the array to be smaller since it only needs to store pointers

It's a two line change, can we try to be a little proportionate here?

You're not even giving me a tag because... I'm not auditing all uses of
__alloc_bulk_array() or something? Seriously.

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Baoquan He]

On 05/26/23 at 10:10am, Lorenzo Stoakes wrote:
> On Fri, May 26, 2023 at 04:56:49PM +0800, Baoquan He wrote:
> > > Umm, the function literally opens with:-
> > >
> > > 	/*
> > > 	 * Skip populated array elements to determine if any pages need
> > > 	 * to be allocated before disabling IRQs.
> > > 	 */
> > > 	while (page_array && nr_populated < nr_pages && page_array[nr_populated])
> > > 		nr_populated++;
> >
> > OK, suppose page_array[] alreasy has three pages populated, if not
> > initialized and there's garbage data in page_array[], it could have
> > nr_populated > 3 finally? This is really risky.
> >
> > >
> > > And then later:-
> > >
> > > 		/* Skip existing pages */
> > > 		if (page_array && page_array[nr_populated]) {
> > > 			nr_populated++;
> > > 			continue;
> > > 		}
> >
> > This is interesting, I thought this place of nr_populated checking and
> > updating is meaningless, in fact it's skipping the element with vlaue
> > in the middle of page_array. I realize this when I recheck the code when
> > replying to your mail. Not sure if we should restrict that, or it's
> > really existing reasonablly.
> >
> > [x][x][x][][][][x][x][][]
> > x marks the element pointing to page.
> 
> All of this is fine, the caller is expected to provide a zeroed array or an
> array that contains existing elements. We only need to use it with a zeroed
> array. We zero the array. Problem solved. Other users use the 'already
> allocated pages' functionality, we don't care.
> 
> >
> > >
> > > This explicitly skips populated array entries and reads page_array to see
> > > if entries already exist, and literally documents this in the comments
> > > above each line, exactly as I describe.
> >
> > OK, I misread your words in log. While page_array[] is still output
> > parameter, just not pure output parameter? Not sure if I understand
> > output parameter correctly.
> 
> Well, output implies output i.e. writing to something. If you also read it,
> it's not just an output parameter is it?
> 
> I don't really want to get into semantics here, the point is the test's
> expectation is that it'd be write-only and it's not so we have to zero the
> array, that's it.
> 
> >
> > Well, I meant adding sentence above __alloc_pages_bulk() to tell:
> > page_array[] could have garbage data stored if you don't initialize
> > it explicitly before calling __alloc_pages_bulk();
> >
> 
> As I said I literally state in multiple places this is about needing to
> initialise the array:-
> 
>     lib/test_vmalloc.c: avoid garbage in page array
> 
> ...
> 
>     This is somewhat unexpected and breaks this test, as we allocate the pages
>     array uninitialised on the assumption it will be overwritten.
> 
> ...
> 
>     We solve both problems by simply using kcalloc() and referencing
>     sizeof(struct page *) rather than sizeof(struct page).
> 
> So I completely disagree we need to add anything more.
> 
> > This could happen in other place if they don't use kcalloc(),
> > kmalloc(GFP_ZERO) or something like this to allocate page_array[]?
> 
> We don't care? I'm fixing a test here not auditing __alloc_bulk_array().
> 
> > > A broader problem we might want to think about is how little anybody is
> > > running this test in order that it wasn't picked up before now... obviously
> > > there's an element of luck as to whether the page_array happens to be
> > > zeroed or not, but you'd think it'd be garbage filled at least a reasonable
> > > amount of the time.
> >
> > Hmm, that's why we may need notice people that there's risk in
> > __alloc_pages_bulk() if page_array[] is not initialized and the garbage
> > could be mistaken as a effective page pointer. My personal opinion.
> > People may argue it's caller's responsibility to do that.
> >
> > Thanks
> > Baoquan
> >
> 
> That's just irrelevant to this change. You're also not replying to my point here
> (that we're clearly not running this test very much).
> 
> I find this review super bikesheddy. Let's try not to bog things down in
> lengthily discussions when literally all I am doing here is:-
> 
> 1. Function expects a zeroed array
> 2. Change the code to zero the array
> 3. Change the array to be smaller since it only needs to store pointers
> 
> It's a two line change, can we try to be a little proportionate here?

Yes, I digressed from the patch itself. At the beginning, I truly didn't
understand why __alloc_bulk_array() would break the test. Sorry for
wasting your time.

While I have to say this is a great fix. As you said, people usually don't
run this test much. I guess maintainer like Uladzislau or you could run
the test to see if the whole code is working well. However, correct
testing code is also very important. If people read testing code, she/he
will know how the interface is used. Sometime if I am not sure about
some code writing, as a vim+ctags+cscope user, I will git grep or use
cscope to search, the testing code is also a good example to refer to.

> 
> You're not even giving me a tag because... I'm not auditing all uses of
> __alloc_bulk_array() or something? Seriously.

I am usually passive when adding a tag as long as the patch has been
acked by other people, or have been picked up by maintainer. Especially
in MM and kdump I maintained, if Andrew has taken the patch, I will
not add tag because I don't want to take up his time to add that tag.
Doing this because Andrew has been helping to merge kexec/kdump patches,
and he creates a very open and harmonious environment for us to study
code and review patch. I always suggest any newcomer to start from MM
and post patch to MM because people from maintainers to reviewers are
very friendly and very proactive.

But now I realize being passive on adidng tag is not good. People could
misunderstand I oppose or still have concern. From now on, I will add
tag as long as I agree on the patch, then expand the discussion if
people would like to.

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Lorenzo Stoakes]

On Sat, May 27, 2023 at 06:11:29PM +0800, Baoquan He wrote:
> Yes, I digressed from the patch itself. At the beginning, I truly didn't
> understand why __alloc_bulk_array() would break the test. Sorry for
> wasting your time.
>

It's fine, no apology needed, it's fine to discuss things but I think it's
also very important to be practical about timelines for these things too.

Thanks for the tag, much appreciated!

Cheers, Lorenzo

Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array

[Baoquan He]

On 05/24/23 at 09:24am, Lorenzo Stoakes wrote:
> It turns out that alloc_pages_bulk_array() does not treat the page_array
> parameter as an output parameter, but rather reads the array and skips any
> entries that have already been allocated.
> 
> This is somewhat unexpected and breaks this test, as we allocate the pages
> array uninitialised on the assumption it will be overwritten.
> 
> As a result, the test was referencing uninitialised data and causing the
> PFN to not be valid and thus a WARN_ON() followed by a null pointer deref
> and panic.
> 
> In addition, this is an array of pointers not of struct page objects, so we
> need only allocate an array with elements of pointer size.
> 
> We solve both problems by simply using kcalloc() and referencing
> sizeof(struct page *) rather than sizeof(struct page).
> 
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> ---
>  lib/test_vmalloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c
> index 9dd9745d365f..3718d9886407 100644
> --- a/lib/test_vmalloc.c
> +++ b/lib/test_vmalloc.c
> @@ -369,7 +369,7 @@ vm_map_ram_test(void)
>  	int i;
>  
>  	map_nr_pages = nr_pages > 0 ? nr_pages:1;
> -	pages = kmalloc(map_nr_pages * sizeof(struct page), GFP_KERNEL);
> +	pages = kcalloc(map_nr_pages, sizeof(struct page *), GFP_KERNEL);

This is a great fix, ack.

Reviewed-by: Baoquan He <bhe@redhat.com>

>  	if (!pages)
>  		return -1;
>  
> -- 
> 2.40.1
>