Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 May 2023 08:08:33 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Christoph Hellwig <hch@infradead.org>,
        Uladzislau Rezki <urezki@gmail.com>
Subject: Re: [PATCH] lib/test_vmalloc.c: avoid garbage in page array
Message-ID: <ZG/4gVO9XPXccR5+@MiWiFi-R3L-srv>
References: <20230524082424.10022-1-lstoakes@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230524082424.10022-1-lstoakes@gmail.com>
Precedence: bulk

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
>