linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
@ 2022-06-07  8:29 Phil Elwell
  2022-06-07  8:30 ` Jason A. Donenfeld
  2022-06-07  8:43 ` Jason A. Donenfeld
  0 siblings, 2 replies; 22+ messages in thread
From: Phil Elwell @ 2022-06-07  8:29 UTC (permalink / raw)
  To: Jason A. Donenfeld, linux, rmk+kernel, linux-arm-kernel,
	linux-kernel, catalin.marinas
  Cc: Stephen Boyd, Ard Biesheuvel, stable

This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up 
on boot even before the earlycon output is available. Hacking jump_label_init to 
skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have 
consequences further down the line.

The stable branch may not be living up to its name, but I don't think this is a 
quick fix.

Phil

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:29 [PATCH v2] ARM: initialize jump labels before setup_machine_fdt() Phil Elwell
@ 2022-06-07  8:30 ` Jason A. Donenfeld
  2022-06-07  8:47   ` Phil Elwell
  2022-06-07  9:14   ` Russell King (Oracle)
  2022-06-07  8:43 ` Jason A. Donenfeld
  1 sibling, 2 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07  8:30 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Hi Phil,

Thanks for testing this. Can you let me know if v1 of this works?

https://lore.kernel.org/lkml/20220602212234.344394-1-Jason@zx2c4.com/

(I'll also fashion a revert for this part of stable.)

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:29 [PATCH v2] ARM: initialize jump labels before setup_machine_fdt() Phil Elwell
  2022-06-07  8:30 ` Jason A. Donenfeld
@ 2022-06-07  8:43 ` Jason A. Donenfeld
  2022-06-07  9:15   ` Phil Elwell
  1 sibling, 1 reply; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07  8:43 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Hi Phil,

On Tue, Jun 7, 2022 at 10:29 AM Phil Elwell <phil@raspberrypi.com> wrote:
>
> This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up
> on boot even before the earlycon output is available. Hacking jump_label_init to
> skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have
> consequences further down the line.

Also, reading this a few times, I'm not 100% sure I understand what
you did to hack around this and why that works. Think you could paste
your hackpatch just out of interest to the discussion (but obviously
not to be applied)?

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:30 ` Jason A. Donenfeld
@ 2022-06-07  8:47   ` Phil Elwell
  2022-06-07  8:51     ` Jason A. Donenfeld
  2022-06-07  9:10     ` Greg KH
  2022-06-07  9:14   ` Russell King (Oracle)
  1 sibling, 2 replies; 22+ messages in thread
From: Phil Elwell @ 2022-06-07  8:47 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Hi Jason,

On 07/06/2022 09:30, Jason A. Donenfeld wrote:
> Hi Phil,
> 
> Thanks for testing this. Can you let me know if v1 of this works?
> 
> https://lore.kernel.org/lkml/20220602212234.344394-1-Jason@zx2c4.com/
> 
> (I'll also fashion a revert for this part of stable.)
> 
> Jason

Thanks for the quick response, but that doesn't work for me either. Let me say 
again that I'm on a downstream kernel (rpi-5.15.y) so this may not be a 
universal problem, but merging either of these fixing patches would be fatal for us.

Phil

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:47   ` Phil Elwell
@ 2022-06-07  8:51     ` Jason A. Donenfeld
  2022-06-07 10:06       ` Catalin Marinas
  2022-06-07  9:10     ` Greg KH
  1 sibling, 1 reply; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07  8:51 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Hi Phil,

On Tue, Jun 7, 2022 at 10:47 AM Phil Elwell <phil@raspberrypi.com> wrote:
> Thanks for the quick response, but that doesn't work for me either. Let me say
> again that I'm on a downstream kernel (rpi-5.15.y) so this may not be a
> universal problem, but merging either of these fixing patches would be fatal for us.

Alright, thanks. And I'm guessing you don't currently have a problem
*without* either of the fixing patches, because your device tree
doesn't use rng-seed. Is that right?

In anycase, I sent in a revert to get all the static branch stuff out
of stable -- https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/
-- so the "urgency" of this should decrease and we can fix this as
normal during the 5.19 cycle. But with that said, I do want to get
this fixed as soon as possible still. I'll be back at my desk tonight
and will finally have access to real hardware again. Are you hitting
this by loading a 32bit kernel on a raspi4? Or running a 32bit kernel
on the raspi1? Or some other combo?

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:47   ` Phil Elwell
  2022-06-07  8:51     ` Jason A. Donenfeld
@ 2022-06-07  9:10     ` Greg KH
  2022-06-07 11:04       ` Phil Elwell
  1 sibling, 1 reply; 22+ messages in thread
From: Greg KH @ 2022-06-07  9:10 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Jason A. Donenfeld, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Catalin Marinas, Stephen Boyd,
	Ard Biesheuvel, stable

On Tue, Jun 07, 2022 at 09:47:30AM +0100, Phil Elwell wrote:
> Hi Jason,
> 
> On 07/06/2022 09:30, Jason A. Donenfeld wrote:
> > Hi Phil,
> > 
> > Thanks for testing this. Can you let me know if v1 of this works?
> > 
> > https://lore.kernel.org/lkml/20220602212234.344394-1-Jason@zx2c4.com/
> > 
> > (I'll also fashion a revert for this part of stable.)
> > 
> > Jason
> 
> Thanks for the quick response, but that doesn't work for me either. Let me
> say again that I'm on a downstream kernel (rpi-5.15.y) so this may not be a
> universal problem, but merging either of these fixing patches would be fatal
> for us.

I have reports of a "clean" 5.15.45 working just fine on a rpi.
Anything special in your tree that isn't upstream yet that might be
conflicting with this?  Any chance you can try a kernel.org release
instead?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:30 ` Jason A. Donenfeld
  2022-06-07  8:47   ` Phil Elwell
@ 2022-06-07  9:14   ` Russell King (Oracle)
  2022-06-07  9:16     ` Jason A. Donenfeld
  1 sibling, 1 reply; 22+ messages in thread
From: Russell King (Oracle) @ 2022-06-07  9:14 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Phil Elwell, linux-arm-kernel, LKML, Catalin Marinas,
	Stephen Boyd, Ard Biesheuvel, stable

On Tue, Jun 07, 2022 at 10:30:49AM +0200, Jason A. Donenfeld wrote:
> Hi Phil,
> 
> Thanks for testing this. Can you let me know if v1 of this works?
> 
> https://lore.kernel.org/lkml/20220602212234.344394-1-Jason@zx2c4.com/
> 
> (I'll also fashion a revert for this part of stable.)

As the arm32 version hasn't been merged yet, how is it in stable already?
If it is in stable, isn't that a yet another violation of the stable
kernel rules?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:43 ` Jason A. Donenfeld
@ 2022-06-07  9:15   ` Phil Elwell
  2022-06-07 15:14     ` Jason A. Donenfeld
  0 siblings, 1 reply; 22+ messages in thread
From: Phil Elwell @ 2022-06-07  9:15 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

On 07/06/2022 09:43, Jason A. Donenfeld wrote:
> Hi Phil,
> 
> On Tue, Jun 7, 2022 at 10:29 AM Phil Elwell <phil@raspberrypi.com> wrote:
>>
>> This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up
>> on boot even before the earlycon output is available. Hacking jump_label_init to
>> skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have
>> consequences further down the line.
> 
> Also, reading this a few times, I'm not 100% sure I understand what
> you did to hack around this and why that works. Think you could paste
> your hackpatch just out of interest to the discussion (but obviously
> not to be applied)?

This is the minimal version of my workaround patch that at least allows the 
board to boot. Bear in mind that it was written with no previous knowledge of 
jump labels and was arrived at by iteratively bisecting the list of jump_labels 
until the first dangerous one was found, then later working out that there was 
only one.

diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index b156e152d6b48..7b053521f7216 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -466,6 +466,7 @@ void __init jump_label_init(void)
         struct jump_entry *iter_stop = __stop___jump_table;
         struct static_key *key = NULL;
         struct jump_entry *iter;
+       int iter_count = 0;

         /*
          * Since we are initializing the static_key.enabled field with
@@ -499,7 +500,9 @@ void __init jump_label_init(void)
                         continue;

                 key = iterk;
-               static_key_set_entries(key, iter);
+               iter_count++;
+               if (iter_count != 1083)
+                       static_key_set_entries(key, iter);
         }
         static_key_initialized = true;
         jump_label_unlock();

I'm sure this could be rewritten in a less fragile manner making reference to 
crng_is_ready directly, but this is where I got to yesterday.

Ha! I just proved the patch's fragility by switching from a Pi 2 to a Pi 4,
so the saner version is:

diff --git a/drivers/char/random.c b/drivers/char/random.c
index ca17a658c2147..ca7c8a67b8314 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -79,7 +79,7 @@ static enum {
         CRNG_EARLY = 1, /* At least POOL_EARLY_BITS collected */
         CRNG_READY = 2  /* Fully initialized with POOL_READY_BITS collected */
  } crng_init __read_mostly = CRNG_EMPTY;
-static DEFINE_STATIC_KEY_FALSE(crng_is_ready);
+DEFINE_STATIC_KEY_FALSE(crng_is_ready);
  #define crng_ready() (static_branch_likely(&crng_is_ready) || crng_init >= 
CRNG_READY)
  /* Various types of waiters for crng_init->CRNG_READY transition. */
  static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);


diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index b156e152d6b48..b79de9da036fd 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -484,6 +484,7 @@ void __init jump_label_init(void)
         jump_label_sort_entries(iter_start, iter_stop);

         for (iter = iter_start; iter < iter_stop; iter++) {
+               extern struct static_key_false crng_is_ready;
                 struct static_key *iterk;
                 bool in_init;

@@ -499,7 +500,8 @@ void __init jump_label_init(void)
                         continue;

                 key = iterk;
-               static_key_set_entries(key, iter);
+               if (key != &crng_is_ready.key)
+                       static_key_set_entries(key, iter);
         }
         static_key_initialized = true;
         jump_label_unlock();

And to answer your questions from the other thread:

* Without any fixing patches we see the warning messages because we are using 
rng-seed in DT.

* I've seen the problem on a Pi 2 and a Pi 4 running 32-bit kernels.

Phil

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  9:14   ` Russell King (Oracle)
@ 2022-06-07  9:16     ` Jason A. Donenfeld
  0 siblings, 0 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07  9:16 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Phil Elwell, linux-arm-kernel, LKML, Catalin Marinas,
	Stephen Boyd, Ard Biesheuvel, stable

On 6/7/22, Russell King (Oracle) <linux@armlinux.org.uk> wrote:
> On Tue, Jun 07, 2022 at 10:30:49AM +0200, Jason A. Donenfeld wrote:
>> Hi Phil,
>>
>> Thanks for testing this. Can you let me know if v1 of this works?
>>
>> https://lore.kernel.org/lkml/20220602212234.344394-1-Jason@zx2c4.com/
>>
>> (I'll also fashion a revert for this part of stable.)
>
> As the arm32 version hasn't been merged yet, how is it in stable already?
> If it is in stable, isn't that a yet another violation of the stable
> kernel rules?

You misunderstood my ambiguous sentence, sorry. I meant a revert of
the thing in stable that makes this discussion here a stable
discussion rather than a 5.19 discussion. No arm32 stuff has been
committed anywhere.

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  8:51     ` Jason A. Donenfeld
@ 2022-06-07 10:06       ` Catalin Marinas
  2022-06-07 10:11         ` Jason A. Donenfeld
  2022-06-08  8:20         ` Jason A. Donenfeld
  0 siblings, 2 replies; 22+ messages in thread
From: Catalin Marinas @ 2022-06-07 10:06 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Phil Elwell, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Stephen Boyd, Ard Biesheuvel, stable

Hi Jason,

On Tue, Jun 07, 2022 at 10:51:41AM +0200, Jason A. Donenfeld wrote:
> On Tue, Jun 7, 2022 at 10:47 AM Phil Elwell <phil@raspberrypi.com> wrote:
> > Thanks for the quick response, but that doesn't work for me either. Let me say
> > again that I'm on a downstream kernel (rpi-5.15.y) so this may not be a
> > universal problem, but merging either of these fixing patches would be fatal for us.
> 
> Alright, thanks. And I'm guessing you don't currently have a problem
> *without* either of the fixing patches, because your device tree
> doesn't use rng-seed. Is that right?
> 
> In anycase, I sent in a revert to get all the static branch stuff out
> of stable -- https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/
> -- so the "urgency" of this should decrease and we can fix this as
> normal during the 5.19 cycle.

Since the above revert got queued in -stable, I assume you don't need
commit 73e2d827a501 ("arm64: Initialize jump labels before
setup_machine_fdt()") in stable either.

Do you plan to fix the crng_ready() static branch differently? If you
do, I'd like to revert the corresponding arm64 commit as well. It seems
to be harmless but I'd rather not keep it if no longer needed. So please
keep me updated whatever you decide.

Thanks.

-- 
Catalin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 10:06       ` Catalin Marinas
@ 2022-06-07 10:11         ` Jason A. Donenfeld
  2022-06-08  8:20         ` Jason A. Donenfeld
  1 sibling, 0 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07 10:11 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Phil Elwell, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Stephen Boyd, Ard Biesheuvel, stable

Hi Catalin,

On Tue, Jun 07, 2022 at 11:06:56AM +0100, Catalin Marinas wrote:
> Hi Jason,
> 
> On Tue, Jun 07, 2022 at 10:51:41AM +0200, Jason A. Donenfeld wrote:
> > On Tue, Jun 7, 2022 at 10:47 AM Phil Elwell <phil@raspberrypi.com> wrote:
> > > Thanks for the quick response, but that doesn't work for me either. Let me say
> > > again that I'm on a downstream kernel (rpi-5.15.y) so this may not be a
> > > universal problem, but merging either of these fixing patches would be fatal for us.
> > 
> > Alright, thanks. And I'm guessing you don't currently have a problem
> > *without* either of the fixing patches, because your device tree
> > doesn't use rng-seed. Is that right?
> > 
> > In anycase, I sent in a revert to get all the static branch stuff out
> > of stable -- https://lore.kernel.org/stable/20220607084005.666059-1-Jason@zx2c4.com/
> > -- so the "urgency" of this should decrease and we can fix this as
> > normal during the 5.19 cycle.
> 
> Since the above revert got queued in -stable, I assume you don't need
> commit 73e2d827a501 ("arm64: Initialize jump labels before
> setup_machine_fdt()") in stable either.

I made the point here:
https://lore.kernel.org/stable/Yp8i9DH57dRGfTNf@kroah.com/T/#m8f33bc14b677980abe690e5c7a4909b5902010cc

> Do you plan to fix the crng_ready() static branch differently? If you
> do, I'd like to revert the corresponding arm64 commit as well. It seems
> to be harmless but I'd rather not keep it if no longer needed. So please
> keep me updated whatever you decide.

I sent a "backup commit" for that here: https://lore.kernel.org/all/20220607100210.683136-1-Jason@zx2c4.com/
But I would like a few days to see if there's some trivial way of not
needing that on arm32. If it turns out to be easy, then I'd prefer the
direct fix akin to the arm64 one. If it turns out to be not easy, then
I'll merge the backup commit. I'll keep you posted (and I assume anyway
you'll see the arm32 attempts progress or fail here, also).

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  9:10     ` Greg KH
@ 2022-06-07 11:04       ` Phil Elwell
  2022-06-07 11:09         ` Jason A. Donenfeld
  0 siblings, 1 reply; 22+ messages in thread
From: Phil Elwell @ 2022-06-07 11:04 UTC (permalink / raw)
  To: Greg KH
  Cc: Jason A. Donenfeld, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Catalin Marinas, Stephen Boyd,
	Ard Biesheuvel, stable

Hi Greg,

On 07/06/2022 10:10, Greg KH wrote:
> On Tue, Jun 07, 2022 at 09:47:30AM +0100, Phil Elwell wrote:
>> Hi Jason,
>>
>> On 07/06/2022 09:30, Jason A. Donenfeld wrote:
>>> Hi Phil,
>>>
>>> Thanks for testing this. Can you let me know if v1 of this works?
>>>
>>> https://lore.kernel.org/lkml/20220602212234.344394-1-Jason@zx2c4.com/
>>>
>>> (I'll also fashion a revert for this part of stable.)
>>>
>>> Jason
>>
>> Thanks for the quick response, but that doesn't work for me either. Let me
>> say again that I'm on a downstream kernel (rpi-5.15.y) so this may not be a
>> universal problem, but merging either of these fixing patches would be fatal
>> for us.
> 
> I have reports of a "clean" 5.15.45 working just fine on a rpi.
> Anything special in your tree that isn't upstream yet that might be
> conflicting with this?  Any chance you can try a kernel.org release
> instead?

A clean 5.15.45 boots cleanly, whereas a downstream kernel shows the static key 
warning (but it does go on to boot). The significant difference is that our 
defconfigs set CONFIG_RANDOM_TRUST_BOOTLOADER=y - defining that on top of 
multi_v7_defconfig demonstrates the issue on a clean 5.15.45. Conversely, not 
setting that option in a downstream kernel build avoids the warning, presumably 
because it takes much longer to accumulate the required entropy.

Phil

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 11:04       ` Phil Elwell
@ 2022-06-07 11:09         ` Jason A. Donenfeld
  0 siblings, 0 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07 11:09 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Greg KH, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Catalin Marinas, Stephen Boyd,
	Ard Biesheuvel, stable

Hi Phil,

On Tue, Jun 07, 2022 at 12:04:13PM +0100, Phil Elwell wrote:
> A clean 5.15.45 boots cleanly, whereas a downstream kernel shows the static key 
> warning (but it does go on to boot). The significant difference is that our 
> defconfigs set CONFIG_RANDOM_TRUST_BOOTLOADER=y - defining that on top of 
> multi_v7_defconfig demonstrates the issue on a clean 5.15.45. Conversely, not 
> setting that option in a downstream kernel build avoids the warning

Ah, that makes sense. Note that I've got a patch out for changing that
defconfig as well to be Y, which means the CI will catch this sort of
thing in the future.

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07  9:15   ` Phil Elwell
@ 2022-06-07 15:14     ` Jason A. Donenfeld
  2022-06-07 15:35       ` Phil Elwell
  0 siblings, 1 reply; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07 15:14 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Hey again,

On Tue, Jun 07, 2022 at 10:15:27AM +0100, Phil Elwell wrote:
> On 07/06/2022 09:43, Jason A. Donenfeld wrote:
> > Hi Phil,
> > 
> > On Tue, Jun 7, 2022 at 10:29 AM Phil Elwell <phil@raspberrypi.com> wrote:
> >>
> >> This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up
> >> on boot even before the earlycon output is available. Hacking jump_label_init to
> >> skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have
> >> consequences further down the line.
> > 
> > Also, reading this a few times, I'm not 100% sure I understand what
> > you did to hack around this and why that works. Think you could paste
> > your hackpatch just out of interest to the discussion (but obviously
> > not to be applied)?
> 
> This is the minimal version of my workaround patch that at least allows the 
> board to boot. Bear in mind that it was written with no previous knowledge of 
> jump labels and was arrived at by iteratively bisecting the list of jump_labels 
> until the first dangerous one was found, then later working out that there was 
> only one.

Looks like this patch fails due to CONFIG_STRICT_KERNEL_RWX.
Investigating deeper now, but that for starters seems to be the
differentiating factor between my prior test rig and one that reproduces
the error. I assume your raspi also sets CONFIG_STRICT_KERNEL_RWX.

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 15:14     ` Jason A. Donenfeld
@ 2022-06-07 15:35       ` Phil Elwell
  2022-06-07 15:44         ` Jason A. Donenfeld
  0 siblings, 1 reply; 22+ messages in thread
From: Phil Elwell @ 2022-06-07 15:35 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Jason,

On 07/06/2022 16:14, Jason A. Donenfeld wrote:
> Hey again,
> 
> On Tue, Jun 07, 2022 at 10:15:27AM +0100, Phil Elwell wrote:
>> On 07/06/2022 09:43, Jason A. Donenfeld wrote:
>>> Hi Phil,
>>>
>>> On Tue, Jun 7, 2022 at 10:29 AM Phil Elwell <phil@raspberrypi.com> wrote:
>>>>
>>>> This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up
>>>> on boot even before the earlycon output is available. Hacking jump_label_init to
>>>> skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have
>>>> consequences further down the line.
>>>
>>> Also, reading this a few times, I'm not 100% sure I understand what
>>> you did to hack around this and why that works. Think you could paste
>>> your hackpatch just out of interest to the discussion (but obviously
>>> not to be applied)?
>>
>> This is the minimal version of my workaround patch that at least allows the
>> board to boot. Bear in mind that it was written with no previous knowledge of
>> jump labels and was arrived at by iteratively bisecting the list of jump_labels
>> until the first dangerous one was found, then later working out that there was
>> only one.
> 
> Looks like this patch fails due to CONFIG_STRICT_KERNEL_RWX.
> Investigating deeper now, but that for starters seems to be the
> differentiating factor between my prior test rig and one that reproduces
> the error. I assume your raspi also sets CONFIG_STRICT_KERNEL_RWX.

Yes, it does, as does multi_v7_defconfig.

Phil

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 15:35       ` Phil Elwell
@ 2022-06-07 15:44         ` Jason A. Donenfeld
  2022-06-07 15:51           ` Phil Elwell
  2022-06-07 19:30           ` [PATCH v3] " Jason A. Donenfeld
  0 siblings, 2 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07 15:44 UTC (permalink / raw)
  To: Phil Elwell
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

Hi Phil,

On Tue, Jun 07, 2022 at 04:35:32PM +0100, Phil Elwell wrote:
> Jason,
> 
> On 07/06/2022 16:14, Jason A. Donenfeld wrote:
> > Hey again,
> > 
> > On Tue, Jun 07, 2022 at 10:15:27AM +0100, Phil Elwell wrote:
> >> On 07/06/2022 09:43, Jason A. Donenfeld wrote:
> >>> Hi Phil,
> >>>
> >>> On Tue, Jun 7, 2022 at 10:29 AM Phil Elwell <phil@raspberrypi.com> wrote:
> >>>>
> >>>> This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up
> >>>> on boot even before the earlycon output is available. Hacking jump_label_init to
> >>>> skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have
> >>>> consequences further down the line.
> >>>
> >>> Also, reading this a few times, I'm not 100% sure I understand what
> >>> you did to hack around this and why that works. Think you could paste
> >>> your hackpatch just out of interest to the discussion (but obviously
> >>> not to be applied)?
> >>
> >> This is the minimal version of my workaround patch that at least allows the
> >> board to boot. Bear in mind that it was written with no previous knowledge of
> >> jump labels and was arrived at by iteratively bisecting the list of jump_labels
> >> until the first dangerous one was found, then later working out that there was
> >> only one.
> > 
> > Looks like this patch fails due to CONFIG_STRICT_KERNEL_RWX.
> > Investigating deeper now, but that for starters seems to be the
> > differentiating factor between my prior test rig and one that reproduces
> > the error. I assume your raspi also sets CONFIG_STRICT_KERNEL_RWX.
> 
> Yes, it does, as does multi_v7_defconfig.

Oh good. Adjusting my CI now to have that.

Having tickled arch/arm/ a little bit now, this is looking sort of
complicated. So I think I might be leaning toward giving up and just
rolling with <https://git.zx2c4.com/linux-rng/commit/?id=78f79dda>.

Unless of course somebody has some ARM chops and can think of a quick
easy fix.

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 15:44         ` Jason A. Donenfeld
@ 2022-06-07 15:51           ` Phil Elwell
  2022-06-07 19:30           ` [PATCH v3] " Jason A. Donenfeld
  1 sibling, 0 replies; 22+ messages in thread
From: Phil Elwell @ 2022-06-07 15:51 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Russell King - ARM Linux, Russell King, linux-arm-kernel, LKML,
	Catalin Marinas, Stephen Boyd, Ard Biesheuvel, stable

On 07/06/2022 16:44, Jason A. Donenfeld wrote:
> Hi Phil,
> 
> On Tue, Jun 07, 2022 at 04:35:32PM +0100, Phil Elwell wrote:
>> Jason,
>>
>> On 07/06/2022 16:14, Jason A. Donenfeld wrote:
>>> Hey again,
>>>
>>> On Tue, Jun 07, 2022 at 10:15:27AM +0100, Phil Elwell wrote:
>>>> On 07/06/2022 09:43, Jason A. Donenfeld wrote:
>>>>> Hi Phil,
>>>>>
>>>>> On Tue, Jun 7, 2022 at 10:29 AM Phil Elwell <phil@raspberrypi.com> wrote:
>>>>>>
>>>>>> This patch is fatal for me in the downstream Raspberry Pi kernel - it locks up
>>>>>> on boot even before the earlycon output is available. Hacking jump_label_init to
>>>>>> skip the jump_entry for "crng_is_ready" allows it to boot, but is likely to have
>>>>>> consequences further down the line.
>>>>>
>>>>> Also, reading this a few times, I'm not 100% sure I understand what
>>>>> you did to hack around this and why that works. Think you could paste
>>>>> your hackpatch just out of interest to the discussion (but obviously
>>>>> not to be applied)?
>>>>
>>>> This is the minimal version of my workaround patch that at least allows the
>>>> board to boot. Bear in mind that it was written with no previous knowledge of
>>>> jump labels and was arrived at by iteratively bisecting the list of jump_labels
>>>> until the first dangerous one was found, then later working out that there was
>>>> only one.
>>>
>>> Looks like this patch fails due to CONFIG_STRICT_KERNEL_RWX.
>>> Investigating deeper now, but that for starters seems to be the
>>> differentiating factor between my prior test rig and one that reproduces
>>> the error. I assume your raspi also sets CONFIG_STRICT_KERNEL_RWX.
>>
>> Yes, it does, as does multi_v7_defconfig.
> 
> Oh good. Adjusting my CI now to have that.
> 
> Having tickled arch/arm/ a little bit now, this is looking sort of
> complicated. So I think I might be leaning toward giving up and just
> rolling with <https://git.zx2c4.com/linux-rng/commit/?id=78f79dda>.
> 
> Unless of course somebody has some ARM chops and can think of a quick
> easy fix.

I've not looked at the performance implications (if any), but in terms of when the RNG initilialization completes and the lack of a WARN I'm happy with that patch, a.k.a. [1].

Phil

[1] https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 15:44         ` Jason A. Donenfeld
  2022-06-07 15:51           ` Phil Elwell
@ 2022-06-07 19:30           ` Jason A. Donenfeld
  2022-06-08  8:18             ` Jason A. Donenfeld
  1 sibling, 1 reply; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-07 19:30 UTC (permalink / raw)
  To: linux-arm-kernel, LKML
  Cc: Jason A. Donenfeld, Stephen Boyd, Phil Elwell, Ard Biesheuvel,
	Russel King, Catalin Marinas

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()").

The same basic issue applies to arm32 as well. So this commit adds a
call to jump_label_init() just before setup_machine_fdt(). Since the
page maps haven't been set yet, this also requires us to use the early
patching code in the jump label code.

Reported-by: Stephen Boyd <swboyd@chromium.org>
Reported-by: Phil Elwell <phil@raspberrypi.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Russel King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/arm/kernel/jump_label.c | 3 ++-
 arch/arm/kernel/setup.c      | 1 +
 arch/arm/mm/mmu.c            | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c
index 303b3ab87f7e..8f8c5312f917 100644
--- a/arch/arm/kernel/jump_label.c
+++ b/arch/arm/kernel/jump_label.c
@@ -8,6 +8,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
 					enum jump_label_type type,
 					bool is_static)
 {
+	extern bool early_mm_initialized;
 	void *addr = (void *)entry->code;
 	unsigned int insn;
 
@@ -16,7 +17,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
 	else
 		insn = arm_gen_nop();
 
-	if (is_static)
+	if (is_static || !early_mm_initialized)
 		__patch_text_early(addr, insn);
 	else
 		patch_text(addr, insn);
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 1e8a50a97edf..3ff80b1ee0b5 100644
--- 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)
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 5e2be37a198e..3f63a5581966 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -1754,10 +1754,13 @@ void __init paging_init(const struct machine_desc *mdesc)
 	__flush_dcache_page(NULL, empty_zero_page);
 }
 
+bool early_mm_initialized;
+
 void __init early_mm_init(const struct machine_desc *mdesc)
 {
 	build_mem_type_table();
 	early_paging_init(mdesc);
+	early_mm_initialized = true;
 }
 
 void set_pte_at(struct mm_struct *mm, unsigned long addr,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v3] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 19:30           ` [PATCH v3] " Jason A. Donenfeld
@ 2022-06-08  8:18             ` Jason A. Donenfeld
  0 siblings, 0 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-08  8:18 UTC (permalink / raw)
  To: linux-arm-kernel, LKML
  Cc: Stephen Boyd, Phil Elwell, Ard Biesheuvel, Russel King, Catalin Marinas

This patch isn't needed in the end. An equivalent patch is needed on
xtensa, powerpc, arc, mips, arm32, arm64, riscv. That's a bit much and
points to a larger issue. So I'll fix this the ugly way in the
random.c code :(.

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-07 10:06       ` Catalin Marinas
  2022-06-07 10:11         ` Jason A. Donenfeld
@ 2022-06-08  8:20         ` Jason A. Donenfeld
  2022-06-08  9:16           ` Catalin Marinas
  1 sibling, 1 reply; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-08  8:20 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Phil Elwell, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Stephen Boyd, Ard Biesheuvel, stable

Hi Catalin,

On Tue, Jun 7, 2022 at 12:07 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> Do you plan to fix the crng_ready() static branch differently? If you
> do, I'd like to revert the corresponding arm64 commit as well. It seems
> to be harmless but I'd rather not keep it if no longer needed. So please
> keep me updated whatever you decide.

Feel free to revert. I'm aborting this line of inquiry and will just
fix it downstream in random.c.

Jason

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
  2022-06-08  8:20         ` Jason A. Donenfeld
@ 2022-06-08  9:16           ` Catalin Marinas
  0 siblings, 0 replies; 22+ messages in thread
From: Catalin Marinas @ 2022-06-08  9:16 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Phil Elwell, Russell King - ARM Linux, Russell King,
	linux-arm-kernel, LKML, Stephen Boyd, Ard Biesheuvel, stable

Hi Jason,

On Wed, Jun 08, 2022 at 10:20:21AM +0200, Jason A. Donenfeld wrote:
> On Tue, Jun 7, 2022 at 12:07 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > Do you plan to fix the crng_ready() static branch differently? If you
> > do, I'd like to revert the corresponding arm64 commit as well. It seems
> > to be harmless but I'd rather not keep it if no longer needed. So please
> > keep me updated whatever you decide.
> 
> Feel free to revert. I'm aborting this line of inquiry and will just
> fix it downstream in random.c.

Thanks for the update.

-- 
Catalin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2] ARM: initialize jump labels before setup_machine_fdt()
@ 2022-06-03 12:15 Jason A. Donenfeld
  0 siblings, 0 replies; 22+ messages in thread
From: Jason A. Donenfeld @ 2022-06-03 12:15 UTC (permalink / raw)
  To: linux, rmk+kernel, linux-arm-kernel, linux-kernel, catalin.marinas
  Cc: Jason A. Donenfeld, Stephen Boyd, Ard Biesheuvel, stable

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. So this commit adds a call to jump_label_init() just before
setup_machine_fdt().

Reported-by: Stephen Boyd <swboyd@chromium.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: stable@vger.kernel.org
Fixes: f5bda35fba61 ("random: use static branch for crng_ready()")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/arm/kernel/setup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 1e8a50a97edf..3ff80b1ee0b5 100644
--- 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)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-06-08  9:49 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-07  8:29 [PATCH v2] ARM: initialize jump labels before setup_machine_fdt() Phil Elwell
2022-06-07  8:30 ` Jason A. Donenfeld
2022-06-07  8:47   ` Phil Elwell
2022-06-07  8:51     ` Jason A. Donenfeld
2022-06-07 10:06       ` Catalin Marinas
2022-06-07 10:11         ` Jason A. Donenfeld
2022-06-08  8:20         ` Jason A. Donenfeld
2022-06-08  9:16           ` Catalin Marinas
2022-06-07  9:10     ` Greg KH
2022-06-07 11:04       ` Phil Elwell
2022-06-07 11:09         ` Jason A. Donenfeld
2022-06-07  9:14   ` Russell King (Oracle)
2022-06-07  9:16     ` Jason A. Donenfeld
2022-06-07  8:43 ` Jason A. Donenfeld
2022-06-07  9:15   ` Phil Elwell
2022-06-07 15:14     ` Jason A. Donenfeld
2022-06-07 15:35       ` Phil Elwell
2022-06-07 15:44         ` Jason A. Donenfeld
2022-06-07 15:51           ` Phil Elwell
2022-06-07 19:30           ` [PATCH v3] " Jason A. Donenfeld
2022-06-08  8:18             ` Jason A. Donenfeld
  -- strict thread matches above, loose matches on Subject: below --
2022-06-03 12:15 [PATCH v2] " Jason A. Donenfeld

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).