On Fri, 3 Jun 2022 at 09:51, Ard Biesheuvel ardb@kernel.org wrote:
(+ Greg)
On Fri, 3 Jun 2022 at 09:37, Jason A. Donenfeld Jason@zx2c4.com wrote:
Hi Ard,
On 6/3/22, Ard Biesheuvel ardb@kernel.org wrote:
On Thu, 2 Jun 2022 at 23:22, Jason A. Donenfeld Jason@zx2c4.com wrote:
Stephen reported that a static key warning splat appears during early boot on arm64 systems that credit randomness from device trees that contain an "rng-seed" property, because setup_machine_fdt() is called before jump_label_init() during setup_arch(), which was fixed by 73e2d827a501 ("arm64: Initialize jump labels before setup_machine_fdt()").
Upon cursory inspection, the same basic issue appears to apply to arm32 as well. In this case, we reorder setup_arch() to do things in the same order as is now the case on arm64.
Reported-by: Stephen Boyd swboyd@chromium.org Cc: Catalin Marinas catalin.marinas@arm.com Cc: Ard Biesheuvel ardb@kernel.org Cc: stable@vger.kernel.org Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Wouldn't it be better to defer the static_branch_enable(&crng_is_ready) call to later in the boot (e.g., using an initcall()), rather than going around 'fixing' fragile, working early boot code across multiple architectures?
Yes, maybe. It's just more book keeping that's potentially unnecessary, which would be nice to avoid. I wrote a patch for this before, but it wasn't beautiful. And Catalin got a pretty easy arm64 patch queued up sufficiently fast that I figured this was better.
The problem is that your original patch was already backported as far back as 5.10, and so this fix will need to be as well.
Playing with the code that runs before the call to setup_machine_fdt() is risky because it implies that issues that are introduced are likely to limit the ability of the system to generate diagnostic output of any kind, given that the device tree is what describes the topology of the system to the kernel. Before that, there is no serial or graphical console, and the only way to figure out what goes on is to connect a JTAG debugger and single step through the code or dump the contents of __log_buf[].
I like the /dev/random work you have been doing but as you know, I was skeptical about the need to backport all of that work to -stable, and it appears my skepticism may have been justified.
The patch in question is an unquantified performance optimization, which means it does not meet the stable-kernel-rules.rst criteria, but it was backported nonetheless. Now, we are in a situation where we must refactor very early boot code to address a regression introduced by that backport.
Signed-off-by: Jason A. Donenfeld Jason@zx2c4.com
arch/arm/kernel/setup.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c index 1e8a50a97edf..ef40d9f5d5a7 100644 --- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -1097,10 +1097,15 @@ void __init setup_arch(char **cmdline_p) const struct machine_desc *mdesc = NULL; void *atags_vaddr = NULL;
setup_initial_init_mm(_text, _etext, _edata, _end);setup_processor();early_fixmap_init();early_ioremap_init();jump_label_init();Is it really necessary to reorder all these calls? What does jump_label_init() actually need?
I'm not quite sure, but it matched how arm64 does things now. Was hoping somebody with deep arm32 knowledge (e.g. you or rmk) would be able to eyeball that to confirm.
As far as I can tell, the early patching code on ARM does not rely on the early fixmap code. Did you try just moving jump_label_init() earlier in the function?
The below seems to work too:
--- a/arch/arm/kernel/setup.c +++ b/arch/arm/kernel/setup.c @@ -1101,6 +1101,7 @@ void __init setup_arch(char **cmdline_p) atags_vaddr = FDT_VIRT_BASE(__atags_pointer);
setup_processor(); + jump_label_init(); if (atags_vaddr) { mdesc = setup_machine_fdt(atags_vaddr); if (mdesc)