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 10:10:56 +0100
From:   Lorenzo Stoakes <lstoakes@gmail.com>
To:     Baoquan He <bhe@redhat.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: <368bbc1d-d810-4bc4-8091-7ed55631344f@lucifer.local>
References: <20230524082424.10022-1-lstoakes@gmail.com>
 <ZG/4gVO9XPXccR5+@MiWiFi-R3L-srv>
 <b87ff2af-c89d-4ddd-8992-2ffb337fbe0c@lucifer.local>
 <ZHB0UTEYUMZVa23V@MiWiFi-R3L-srv>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZHB0UTEYUMZVa23V@MiWiFi-R3L-srv>
Precedence: bulk

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.