Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures

[Ross Philipson <ross.philipson@oracle.com>]

On 2/15/22 13:37, Borislav Petkov wrote:
> On Tue, Feb 15, 2022 at 06:34:43AM -0500, Ross Philipson wrote:
>> It can if you run out of slots in the fixed map.
> 
> Right. Or if any of the checks in __early_ioremap() fail. But those
> would at least warn.
> 
>> The only reason I did not check it for NULL was because it was not
>> checked elsewhere for NULL.
> 
> Elsewhere in the tree or elsewhere in this file or in the setup_indirect
> adding code?

In the ioremap.c module, the check for NULL is only missing in the
functions we updated but the lack of a check was already there before
these changes went in.

In the setup.c and e820.c modules, the check for NULL is missing in the
functions we updated but the lack of a check was already there before
these changes went in in those functions. The lack of early_memremap()
NULL checks can also be found in other functions in those modules.

> 
>> I guess there are two questions:
>>
>> 1. Should I also fix it elsewhere in the code I am touching?
> 
> Yes pls.
> 
>> 2. What should I do on an allocation failure? In a routine like this it
>> seems to be a critical early boot failure.
> 
> How so?
> 
> I'd expect in the case of e820__reserve_setup_data(), for example, to
> not call e820__range_update* and not have those indirect ranges present
> in the e820 map. What the user intended might not work but it'll at
> least boot instead of floating dead in the water.
> 
> And similar approach in the other places you're touching.

Yes I can see how to handle the failures. I will fix the code to do the
appropriate thing given what each of the functions is doing.

Fixing it in other functions and possibly elsewhere in the arch/x86 code
seems to be out of scope for this patch set. We could send separate
patches and hunt down other places this check is missing.

> 
> You could even issue a warning or so so that users at least know what's
> going on. I'd say...

Yea I can pr_warn when the issue occurs.

> 
>> I guess the original intention might have been to let it just blow up
>> since there is no recovery but that is just conjecture...
> 
> The original intention?

It was just idle speculation, just ignore this.

Thanks
Ross

> 
> Thx.
>