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:   Tue, 15 Feb 2022 19:37:38 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Ross Philipson <ross.philipson@oracle.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        daniel.kiper@oracle.com, dpsmith@apertussolutions.com,
        tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com,
        luto@amacapital.net, kanth.ghatraju@oracle.com,
        trenchboot-devel@googlegroups.com
Subject: Re: [PATCH 1/2] x86/boot: Fix memremap of setup_indirect structures
Message-ID: <Ygvy8j9E7WPo6dx0@zn.tnic>
References: <1643303056-22272-1-git-send-email-ross.philipson@oracle.com>
 <1643303056-22272-2-git-send-email-ross.philipson@oracle.com>
 <YgabtFd4Nkpgi+oW@zn.tnic>
 <2e584fc4-163a-0af4-abe0-9c14996918f2@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <2e584fc4-163a-0af4-abe0-9c14996918f2@oracle.com>
Precedence: bulk

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?

> 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.

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

> 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?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette