lkml.org 
[lkml]   [2022]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures
From
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.
>

\
 
 \ /
  Last update: 2022-02-22 22:03    [W:1.249 / U:1.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site