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.