Messages in this thread | | | Date | Tue, 22 Feb 2022 16:01:32 -0500 | Subject | Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures | From | Ross Philipson <> |
| |
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. >
| |