Message-ID: <CAHmME9qPdEwx9LQj_U2S+DRjp3qiOMjGieYehsHLYqhKjAbs0g@mail.gmail.com>
Date:   Fri, 3 Jun 2022 10:14:32 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Russell King <linux@...linux.org.uk>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Stephen Boyd <swboyd@...omium.org>,
        "# 3.4.x" <stable@...r.kernel.org>
Subject: Re: [PATCH] ARM: initialize jump labels before setup_machine_fdt()

Hi Ard,

On 6/3/22, Ard Biesheuvel <ardb@...nel.org> wrote:
> On Fri, 3 Jun 2022 at 09:51, Ard Biesheuvel <ardb@...nel.org> wrote:
>>
>> (+ Greg)
>>
>> On Fri, 3 Jun 2022 at 09:37, Jason A. Donenfeld <Jason@...c4.com> wrote:
>> >
>> > Hi Ard,
>> >
>> > On 6/3/22, Ard Biesheuvel <ardb@...nel.org> wrote:
>> > > On Thu, 2 Jun 2022 at 23:22, Jason A. Donenfeld <Jason@...c4.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@...omium.org>
>> > >> Cc: Catalin Marinas <catalin.marinas@....com>
>> > >> Cc: Ard Biesheuvel <ardb@...nel.org>
>> > >> Cc: stable@...r.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@...c4.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)
>

Oh, awesome, that's exactly what I was about to try when I got to my
laptop in a few hours. Do you want to send the v2 with that, or should
I do so after also testing it in a bit?

Jason

Powered by blists - more mailing lists