| Message ID | 20220607100210.683136-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | random: do not use jump labels before they are initialized | expand |
Hi Jason, On Tue, 7 Jun 2022 at 12:04, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > [ I would like to pursue fixing this more directly first before actually > merging this, but I thought I'd send this to the list now anyway as a > the "backup" plan. If I can't figure out how to make headway on the > main plan in the next few days, it'll be easy to just do this. ] > What more direct fix did you have in mind here? > Stephen reported that a static key warning splat appears during early > boot on systems that credit randomness from device trees that contain an > "rng-seed" property, because because setup_machine_fdt() is called > before jump_label_init() during setup_arch(): > > static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init() > WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8 > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff > pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : static_key_enable_cpuslocked+0xb0/0xb8 > lr : static_key_enable_cpuslocked+0xb0/0xb8 > sp : ffffffe51c393cf0 > x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10 > x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000 > x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000 > x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020 > x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708 > x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000 > x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000 > x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027 > x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05 > x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065 > Call trace: > static_key_enable_cpuslocked+0xb0/0xb8 > static_key_enable+0x2c/0x40 > crng_set_ready+0x24/0x30 > execute_in_process_context+0x80/0x90 > _credit_init_bits+0x100/0x154 > add_bootloader_randomness+0x64/0x78 > early_init_dt_scan_chosen+0x140/0x184 > early_init_dt_scan_nodes+0x28/0x4c > early_init_dt_scan+0x40/0x44 > setup_machine_fdt+0x7c/0x120 > setup_arch+0x74/0x1d8 > start_kernel+0x84/0x44c > __primary_switched+0xc0/0xc8 > ---[ end trace 0000000000000000 ]--- > random: crng init done > Machine model: Google Lazor (rev1 - 2) with LTE > > A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64: > Initialize jump labels before setup_machine_fdt()"). But it appears that > fixing it on other platforms might not be so trivial. Instead, defer the > setting of the static branch until later in the boot process. > > Fixes: f5bda35fba61 ("random: use static branch for crng_ready()") > Reported-by: Stephen Boyd <swboyd@chromium.org> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Phil Elwell <phil@raspberrypi.com> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/char/random.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 4862d4d3ec49..f9a020ec08b9 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits) > > if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { > crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ > - execute_in_process_context(crng_set_ready, &set_ready); > + if (static_key_initialized) > + execute_in_process_context(crng_set_ready, &set_ready); Can we just drop this entirely, and rely on the hunk below to set the static key? What justifies having two code paths that set the static key in different ways on different architectures? The use of the static key in general seems like a reasonable idea, even though it is not clear what we actually gain by it (it omits a single load from memory, right?) So I'd argue that the impact of deferring the static key assignment is so limited that there is really no reason for doing it this early, as this clearly has unanticipated side effects that are difficult to diagnose in some cases (i.e., boot crashes before the early console comes up) > wake_up_interruptible(&crng_init_wait); > kill_fasync(&fasync, SIGIO, POLL_IN); > pr_notice("crng init done\n"); > @@ -779,6 +780,14 @@ int __init random_init(const char *command_line) > unsigned int i, arch_bytes; > unsigned long entropy; > > + /* > + * If we were initialized by the bootloader before jump labels are > + * initialized, then we should enable the static branch here, where > + * it's guaranteed that jump labels have been initialized. > + */ > + if (!static_branch_likely(&crng_is_ready) && crng_init >= CRNG_READY) > + crng_set_ready(NULL); > + > #if defined(LATENT_ENTROPY_PLUGIN) > static const u8 compiletime_seed[BLAKE2S_BLOCK_SIZE] __initconst __latent_entropy; > _mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed)); > -- > 2.35.1 >
Hi Ard, On Tue, Jun 07, 2022 at 12:13:29PM +0200, Ard Biesheuvel wrote: > Hi Jason, > > On Tue, 7 Jun 2022 at 12:04, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > [ I would like to pursue fixing this more directly first before actually > > merging this, but I thought I'd send this to the list now anyway as a > > the "backup" plan. If I can't figure out how to make headway on the > > main plan in the next few days, it'll be easy to just do this. ] > > > > What more direct fix did you have in mind here? A non-broken version of https://lore.kernel.org/lkml/20220603121543.360283-1-Jason@zx2c4.com/ As I mentioned in https://lore.kernel.org/lkml/Yp8kQrBgE3WVqqC5@zx2c4.com/ , 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. > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > index 4862d4d3ec49..f9a020ec08b9 100644 > > --- a/drivers/char/random.c > > +++ b/drivers/char/random.c > > @@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits) > > > > if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { > > crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ > > - execute_in_process_context(crng_set_ready, &set_ready); > > + if (static_key_initialized) > > + execute_in_process_context(crng_set_ready, &set_ready); > > Can we just drop this entirely, and rely on the hunk below to set the > static key? What justifies having two code paths that set the static > key in different ways on different architectures? No, we can't. The hunk below (A) is called from init/main.c some time after jump_label_init(). The hunk above (B) is called whenever we reach the 256-bit mark. The order is (B)(A) on machines that get seeded from efi or device tree. The order is (A)(B) on all other machines, which reach the 256-bit mark at "some point"... could be after a second, a minute, whenever enough estimated entropy has been accounted. Jason
Hey again, On Tue, Jun 07, 2022 at 12:28:08PM +0200, Jason A. Donenfeld wrote: > Hi Ard, > > On Tue, Jun 07, 2022 at 12:13:29PM +0200, Ard Biesheuvel wrote: > > Hi Jason, > > > > On Tue, 7 Jun 2022 at 12:04, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > [ I would like to pursue fixing this more directly first before actually > > > merging this, but I thought I'd send this to the list now anyway as a > > > the "backup" plan. If I can't figure out how to make headway on the > > > main plan in the next few days, it'll be easy to just do this. ] > > > > > > > What more direct fix did you have in mind here? > > A non-broken version of https://lore.kernel.org/lkml/20220603121543.360283-1-Jason@zx2c4.com/ > > As I mentioned in https://lore.kernel.org/lkml/Yp8kQrBgE3WVqqC5@zx2c4.com/ , > > 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. > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > > index 4862d4d3ec49..f9a020ec08b9 100644 > > > --- a/drivers/char/random.c > > > +++ b/drivers/char/random.c > > > @@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits) > > > > > > if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { > > > crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ > > > - execute_in_process_context(crng_set_ready, &set_ready); > > > + if (static_key_initialized) > > > + execute_in_process_context(crng_set_ready, &set_ready); > > > > Can we just drop this entirely, and rely on the hunk below to set the > > static key? What justifies having two code paths that set the static > > key in different ways on different architectures? > > No, we can't. The hunk below (A) is called from init/main.c some time after > jump_label_init(). The hunk above (B) is called whenever we reach the > 256-bit mark. > > The order is (B)(A) on machines that get seeded from efi or device tree. > The order is (A)(B) on all other machines, which reach the 256-bit mark > at "some point"... could be after a second, a minute, whenever enough > estimated entropy has been accounted. Just thinking about this a bit more, I should note that this is not the first problem caused by EFI/DT doing its thing mega early in the boot process. Dominik and I fixed up what felt like endless bugs all of basically that same form back in January. Before it mostly had to do with workqueues not being available yet. Now it has to do with jump labels. So in thinking about how to fix this, the two approaches thus far discussed are: 1. initialize jump labels earlier, e.g. the arm64 patch (and proposed arm32 patch). 2. defer the static key enabling until later, e.g. this patch. As a third, I could just defer doing anything with the bootloader seed until random_init(). This might actually be the simplest solution... I'll sketch something out. A downside, which might be sort of significant, is that a few odd things actually use randomness before random_init() is called. So these would miss out on having that seed. I'll have to look what exactly to see if we're actually getting anything real out of that. Jason
On Tue, 7 Jun 2022 at 12:42, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hey again, > > On Tue, Jun 07, 2022 at 12:28:08PM +0200, Jason A. Donenfeld wrote: > > Hi Ard, > > > > On Tue, Jun 07, 2022 at 12:13:29PM +0200, Ard Biesheuvel wrote: > > > Hi Jason, > > > > > > On Tue, 7 Jun 2022 at 12:04, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > > > [ I would like to pursue fixing this more directly first before actually > > > > merging this, but I thought I'd send this to the list now anyway as a > > > > the "backup" plan. If I can't figure out how to make headway on the > > > > main plan in the next few days, it'll be easy to just do this. ] > > > > > > > > > > What more direct fix did you have in mind here? > > > > A non-broken version of https://lore.kernel.org/lkml/20220603121543.360283-1-Jason@zx2c4.com/ > > > > As I mentioned in https://lore.kernel.org/lkml/Yp8kQrBgE3WVqqC5@zx2c4.com/ , > > > > 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. > > > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > > > index 4862d4d3ec49..f9a020ec08b9 100644 > > > > --- a/drivers/char/random.c > > > > +++ b/drivers/char/random.c > > > > @@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits) > > > > > > > > if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { > > > > crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ > > > > - execute_in_process_context(crng_set_ready, &set_ready); > > > > + if (static_key_initialized) > > > > + execute_in_process_context(crng_set_ready, &set_ready); > > > > > > Can we just drop this entirely, and rely on the hunk below to set the > > > static key? What justifies having two code paths that set the static > > > key in different ways on different architectures? > > > > No, we can't. The hunk below (A) is called from init/main.c some time after > > jump_label_init(). The hunk above (B) is called whenever we reach the > > 256-bit mark. > > > > The order is (B)(A) on machines that get seeded from efi or device tree. > > The order is (A)(B) on all other machines, which reach the 256-bit mark > > at "some point"... could be after a second, a minute, whenever enough > > estimated entropy has been accounted. > > Just thinking about this a bit more, I should note that this is not the > first problem caused by EFI/DT doing its thing mega early in the boot > process. Dominik and I fixed up what felt like endless bugs all of > basically that same form back in January. Before it mostly had to do > with workqueues not being available yet. Now it has to do with jump > labels. > > So in thinking about how to fix this, the two approaches thus far > discussed are: > > 1. initialize jump labels earlier, e.g. the arm64 patch (and proposed > arm32 patch). > 2. defer the static key enabling until later, e.g. this patch. > > As a third, I could just defer doing anything with the bootloader seed > until random_init(). This might actually be the simplest solution... > I'll sketch something out. A downside, which might be sort of > significant, is that a few odd things actually use randomness before > random_init() is called. So these would miss out on having that seed. > I'll have to look what exactly to see if we're actually getting anything > real out of that. > This is kind of the point of using a firmware provided seed, i.e., that it is available much earlier than anything else. Could we do this to defer the static key manipulation? That way, the first call to crng_reseed() that occurs after the static keys API becomes available will set the static key, and patch itself away at the same time. ---------->8------------- diff --git a/drivers/char/random.c b/drivers/char/random.c index b691b9d59503..fad4e1a043ce 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -110,7 +110,8 @@ EXPORT_SYMBOL(rng_is_initialized); static void __cold crng_set_ready(struct work_struct *work) { - static_branch_enable(&crng_is_ready); + if (static_key_initialized) + static_branch_enable(&crng_is_ready); } /* Used by wait_for_random_bytes(), and considered an entropy collector, below. */ @@ -202,6 +203,7 @@ static void extract_entropy(void *buf, size_t len); /* This extracts a new crng key from the input pool. */ static void crng_reseed(void) { + static struct execute_work set_ready; unsigned long flags; unsigned long next_gen; u8 key[CHACHA_KEY_SIZE]; @@ -221,8 +223,10 @@ static void crng_reseed(void) ++next_gen; WRITE_ONCE(base_crng.generation, next_gen); WRITE_ONCE(base_crng.birth, jiffies); - if (!static_branch_likely(&crng_is_ready)) + if (!static_branch_likely(&crng_is_ready)) { + execute_in_process_context(crng_set_ready, &set_ready); crng_init = CRNG_READY; + } spin_unlock_irqrestore(&base_crng.lock, flags); memzero_explicit(key, sizeof(key)); } @@ -634,7 +638,6 @@ static void extract_entropy(void *buf, size_t len) static void __cold _credit_init_bits(size_t bits) { - static struct execute_work set_ready; unsigned int new, orig, add; unsigned long flags; @@ -650,7 +653,6 @@ static void __cold _credit_init_bits(size_t bits) if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ - execute_in_process_context(crng_set_ready, &set_ready); wake_up_interruptible(&crng_init_wait); kill_fasync(&fasync, SIGIO, POLL_IN); pr_notice("crng init done\n");
Hi Ard, On Tue, Jun 07, 2022 at 12:56:20PM +0200, Ard Biesheuvel wrote: > Could we do this to defer the static key manipulation? That way, the > first call to crng_reseed() that occurs after the static keys API > becomes available will set the static key, and patch itself away at > the same time. That's almost the same as the patch I just posted, except you pushed the logic down into crng_reseed() instead of credit_init_bits(). (A previous mini-project aimed to remove as much logic as possible from crng_reseed(), counting on those blocks in crng_init_bits() to only ever run once.) What this means is that the static key won't get changed until whenever the next reseeding is. I guess that's "fine" but I think I'd prefer to keep the entropy counting stuff as separate from the init bits stuff as possible. >> As a third, I could just defer doing anything with the bootloader seed >> until random_init(). This might actually be the simplest solution... >> I'll sketch something out. A downside, which might be sort of >> significant, is that a few odd things actually use randomness before >> random_init() is called. So these would miss out on having that seed. >> I'll have to look what exactly to see if we're actually getting anything >> real out of that. >> > > This is kind of the point of using a firmware provided seed, i.e., > that it is available much earlier than anything else. I'll send a patch for this anyway because I'm sort of curious now. Maybe it'll be a dead end, for the reason you mentioned, but I think I'll still try to evaluate it. Jason
On Tue, 7 Jun 2022 at 13:04, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Ard, > > On Tue, Jun 07, 2022 at 12:56:20PM +0200, Ard Biesheuvel wrote: > > Could we do this to defer the static key manipulation? That way, the > > first call to crng_reseed() that occurs after the static keys API > > becomes available will set the static key, and patch itself away at > > the same time. > > That's almost the same as the patch I just posted, except you > pushed the logic down into crng_reseed() instead of credit_init_bits(). Sure. > (A previous mini-project aimed to remove as much logic as possible from > crng_reseed(), counting on those blocks in crng_init_bits() to only ever > run once.) What this means is that the static key won't get changed > until whenever the next reseeding is. I guess that's "fine" but I think > I'd prefer to keep the entropy counting stuff as separate from the init > bits stuff as possible. > Fair enough. What I would like is to remove the need to play around with the placement of jump_label_init() across architectures. Jump labels are fundamentally a performance optimization, so unless you can explain how setting it as early as possible makes a material difference, performance or otherwise, I really think we should pursue a solution that does the static key manipulation at some later time. > >> As a third, I could just defer doing anything with the bootloader seed > >> until random_init(). This might actually be the simplest solution... > >> I'll sketch something out. A downside, which might be sort of > >> significant, is that a few odd things actually use randomness before > >> random_init() is called. So these would miss out on having that seed. > >> I'll have to look what exactly to see if we're actually getting anything > >> real out of that. > >> > > > > This is kind of the point of using a firmware provided seed, i.e., > > that it is available much earlier than anything else. > > I'll send a patch for this anyway because I'm sort of curious now. Maybe > it'll be a dead end, for the reason you mentioned, but I think I'll > still try to evaluate it. > Sure. Anything that can be deferred to an initcall() should be, as the early arch code is much too fragile to much around with.
Hi Ard, On Tue, Jun 07, 2022 at 01:10:52PM +0200, Ard Biesheuvel wrote: > Fair enough. What I would like is to remove the need to play around > with the placement of jump_label_init() across architectures. Jump > labels are fundamentally a performance optimization, so unless you can > explain how setting it as early as possible makes a material > difference, performance or otherwise, I really think we should pursue > a solution that does the static key manipulation at some later time. Alright. It sounds like Catalin also prefers the same. This seems simple enough with minimal downsides: https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/ So maybe we should just go that route. Jason
On Tue, 7 Jun 2022 at 13:35, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Ard, > > On Tue, Jun 07, 2022 at 01:10:52PM +0200, Ard Biesheuvel wrote: > > Fair enough. What I would like is to remove the need to play around > > with the placement of jump_label_init() across architectures. Jump > > labels are fundamentally a performance optimization, so unless you can > > explain how setting it as early as possible makes a material > > difference, performance or otherwise, I really think we should pursue > > a solution that does the static key manipulation at some later time. > > Alright. It sounds like Catalin also prefers the same. This seems simple > enough with minimal downsides: https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/ > That looks simple enough. Do we risk causing any boot stalls due to the crediting being deferred? Or new warnings about randomness being used before CRNG is ready? > So maybe we should just go that route. > It is not my preferred approach, but I can live with it.
Hi Ard, On Tue, Jun 07, 2022 at 02:03:28PM +0200, Ard Biesheuvel wrote: > On Tue, 7 Jun 2022 at 13:35, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > Hi Ard, > > > > On Tue, Jun 07, 2022 at 01:10:52PM +0200, Ard Biesheuvel wrote: > > > Fair enough. What I would like is to remove the need to play around > > > with the placement of jump_label_init() across architectures. Jump > > > labels are fundamentally a performance optimization, so unless you can > > > explain how setting it as early as possible makes a material > > > difference, performance or otherwise, I really think we should pursue > > > a solution that does the static key manipulation at some later time. > > > > Alright. It sounds like Catalin also prefers the same. This seems simple > > enough with minimal downsides: https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/ > > > > That looks simple enough. Do we risk causing any boot stalls due to > the crediting being deferred? Or new warnings about randomness being > used before CRNG is ready? We don't risk boot stalls. But there will be warnings for developers who have enabled the CONFIG_WARN_ALL_UNSEEDED_RANDOM debug option. > > So maybe we should just go that route. > > > > It is not my preferred approach, but I can live with it. I'm not sure what your preferred approach is at this point in time actually. I'll summarize all the approaches discussed so far: 1) Fix archs to initialize jump labels earlier: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=73e2d827a501 https://lore.kernel.org/lkml/20220603121543.360283-1-Jason@zx2c4.com/ 2) Defer mixing & crediting until random_init(): https://lore.kernel.org/lkml/20220607111514.755009-1-Jason@zx2c4.com/ 3) Defer crediting (but not mixing) until random_init(): https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/ 4) Defer changing the static branch (but neither mixing nor crediting) until random_init(): https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/ My first choice is (1) if it's feasible. (2) is not possible without introducing a copy, so that's out. What's your preferred approach? Or is there a number 5 you have in mind? Jason
On Tue, 7 Jun 2022 at 14:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Ard, > > On Tue, Jun 07, 2022 at 02:03:28PM +0200, Ard Biesheuvel wrote: > > On Tue, 7 Jun 2022 at 13:35, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > Hi Ard, > > > > > > On Tue, Jun 07, 2022 at 01:10:52PM +0200, Ard Biesheuvel wrote: > > > > Fair enough. What I would like is to remove the need to play around > > > > with the placement of jump_label_init() across architectures. Jump > > > > labels are fundamentally a performance optimization, so unless you can > > > > explain how setting it as early as possible makes a material > > > > difference, performance or otherwise, I really think we should pursue > > > > a solution that does the static key manipulation at some later time. > > > > > > Alright. It sounds like Catalin also prefers the same. This seems simple > > > enough with minimal downsides: https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/ > > > > > > > That looks simple enough. Do we risk causing any boot stalls due to > > the crediting being deferred? Or new warnings about randomness being > > used before CRNG is ready? > > We don't risk boot stalls. But there will be warnings for developers who > have enabled the CONFIG_WARN_ALL_UNSEEDED_RANDOM debug option. > > > > > So maybe we should just go that route. > > > > > > > It is not my preferred approach, but I can live with it. > > I'm not sure what your preferred approach is at this point in time > actually. I'll summarize all the approaches discussed so far: > > 1) Fix archs to initialize jump labels earlier: > https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=73e2d827a501 > https://lore.kernel.org/lkml/20220603121543.360283-1-Jason@zx2c4.com/ > > 2) Defer mixing & crediting until random_init(): > https://lore.kernel.org/lkml/20220607111514.755009-1-Jason@zx2c4.com/ > > 3) Defer crediting (but not mixing) until random_init(): > https://lore.kernel.org/lkml/20220607113238.769088-1-Jason@zx2c4.com/ > > 4) Defer changing the static branch (but neither mixing nor crediting) until random_init(): > https://lore.kernel.org/lkml/20220607100210.683136-1-Jason@zx2c4.com/ > > > My first choice is (1) if it's feasible. > > (2) is not possible without introducing a copy, so that's out. > > What's your preferred approach? Or is there a number 5 you have in mind? > Seems like we need a mutex instead of going back concurrently on two different threads :-) I'll shut up now and wait for your reply on the other thread.
On Tue, 7 Jun 2022 at 12:04, Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > [ I would like to pursue fixing this more directly first before actually > merging this, but I thought I'd send this to the list now anyway as a > the "backup" plan. If I can't figure out how to make headway on the > main plan in the next few days, it'll be easy to just do this. ] > > Stephen reported that a static key warning splat appears during early > boot on systems that credit randomness from device trees that contain an > "rng-seed" property, because because setup_machine_fdt() is called > before jump_label_init() during setup_arch(): > > static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init() > WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8 > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff > pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : static_key_enable_cpuslocked+0xb0/0xb8 > lr : static_key_enable_cpuslocked+0xb0/0xb8 > sp : ffffffe51c393cf0 > x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10 > x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000 > x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000 > x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020 > x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708 > x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000 > x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000 > x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027 > x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05 > x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065 > Call trace: > static_key_enable_cpuslocked+0xb0/0xb8 > static_key_enable+0x2c/0x40 > crng_set_ready+0x24/0x30 > execute_in_process_context+0x80/0x90 > _credit_init_bits+0x100/0x154 > add_bootloader_randomness+0x64/0x78 > early_init_dt_scan_chosen+0x140/0x184 > early_init_dt_scan_nodes+0x28/0x4c > early_init_dt_scan+0x40/0x44 > setup_machine_fdt+0x7c/0x120 > setup_arch+0x74/0x1d8 > start_kernel+0x84/0x44c > __primary_switched+0xc0/0xc8 > ---[ end trace 0000000000000000 ]--- > random: crng init done > Machine model: Google Lazor (rev1 - 2) with LTE > > A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64: > Initialize jump labels before setup_machine_fdt()"). But it appears that > fixing it on other platforms might not be so trivial. Instead, defer the > setting of the static branch until later in the boot process. > > Fixes: f5bda35fba61 ("random: use static branch for crng_ready()") > Reported-by: Stephen Boyd <swboyd@chromium.org> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Russell King <linux@armlinux.org.uk> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Phil Elwell <phil@raspberrypi.com> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Reviewed-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/char/random.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 4862d4d3ec49..f9a020ec08b9 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits) > > if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { > crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ > - execute_in_process_context(crng_set_ready, &set_ready); > + if (static_key_initialized) > + execute_in_process_context(crng_set_ready, &set_ready); > wake_up_interruptible(&crng_init_wait); > kill_fasync(&fasync, SIGIO, POLL_IN); > pr_notice("crng init done\n"); > @@ -779,6 +780,14 @@ int __init random_init(const char *command_line) > unsigned int i, arch_bytes; > unsigned long entropy; > > + /* > + * If we were initialized by the bootloader before jump labels are > + * initialized, then we should enable the static branch here, where > + * it's guaranteed that jump labels have been initialized. > + */ > + if (!static_branch_likely(&crng_is_ready) && crng_init >= CRNG_READY) > + crng_set_ready(NULL); > + > #if defined(LATENT_ENTROPY_PLUGIN) > static const u8 compiletime_seed[BLAKE2S_BLOCK_SIZE] __initconst __latent_entropy; > _mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed)); > -- > 2.35.1 >
diff --git a/drivers/char/random.c b/drivers/char/random.c index 4862d4d3ec49..f9a020ec08b9 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -650,7 +650,8 @@ static void __cold _credit_init_bits(size_t bits) if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) { crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */ - execute_in_process_context(crng_set_ready, &set_ready); + if (static_key_initialized) + execute_in_process_context(crng_set_ready, &set_ready); wake_up_interruptible(&crng_init_wait); kill_fasync(&fasync, SIGIO, POLL_IN); pr_notice("crng init done\n"); @@ -779,6 +780,14 @@ int __init random_init(const char *command_line) unsigned int i, arch_bytes; unsigned long entropy; + /* + * If we were initialized by the bootloader before jump labels are + * initialized, then we should enable the static branch here, where + * it's guaranteed that jump labels have been initialized. + */ + if (!static_branch_likely(&crng_is_ready) && crng_init >= CRNG_READY) + crng_set_ready(NULL); + #if defined(LATENT_ENTROPY_PLUGIN) static const u8 compiletime_seed[BLAKE2S_BLOCK_SIZE] __initconst __latent_entropy; _mix_pool_bytes(compiletime_seed, sizeof(compiletime_seed));
[ I would like to pursue fixing this more directly first before actually merging this, but I thought I'd send this to the list now anyway as a the "backup" plan. If I can't figure out how to make headway on the main plan in the next few days, it'll be easy to just do this. ] Stephen reported that a static key warning splat appears during early boot on systems that credit randomness from device trees that contain an "rng-seed" property, because because setup_machine_fdt() is called before jump_label_init() during setup_arch(): static_key_enable_cpuslocked(): static key '0xffffffe51c6fcfc0' used before call to jump_label_init() WARNING: CPU: 0 PID: 0 at kernel/jump_label.c:166 static_key_enable_cpuslocked+0xb0/0xb8 Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0+ #224 44b43e377bfc84bc99bb5ab885ff694984ee09ff pstate: 600001c9 (nZCv dAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : static_key_enable_cpuslocked+0xb0/0xb8 lr : static_key_enable_cpuslocked+0xb0/0xb8 sp : ffffffe51c393cf0 x29: ffffffe51c393cf0 x28: 000000008185054c x27: 00000000f1042f10 x26: 0000000000000000 x25: 00000000f10302b2 x24: 0000002513200000 x23: 0000002513200000 x22: ffffffe51c1c9000 x21: fffffffdfdc00000 x20: ffffffe51c2f0831 x19: ffffffe51c6fcfc0 x18: 00000000ffff1020 x17: 00000000e1e2ac90 x16: 00000000000000e0 x15: ffffffe51b710708 x14: 0000000000000066 x13: 0000000000000018 x12: 0000000000000000 x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000 x8 : 0000000000000000 x7 : 61632065726f6665 x6 : 6220646573752027 x5 : ffffffe51c641d25 x4 : ffffffe51c13142c x3 : ffff0a00ffffff05 x2 : 40000000ffffe003 x1 : 00000000000001c0 x0 : 0000000000000065 Call trace: static_key_enable_cpuslocked+0xb0/0xb8 static_key_enable+0x2c/0x40 crng_set_ready+0x24/0x30 execute_in_process_context+0x80/0x90 _credit_init_bits+0x100/0x154 add_bootloader_randomness+0x64/0x78 early_init_dt_scan_chosen+0x140/0x184 early_init_dt_scan_nodes+0x28/0x4c early_init_dt_scan+0x40/0x44 setup_machine_fdt+0x7c/0x120 setup_arch+0x74/0x1d8 start_kernel+0x84/0x44c __primary_switched+0xc0/0xc8 ---[ end trace 0000000000000000 ]--- random: crng init done Machine model: Google Lazor (rev1 - 2) with LTE A trivial fix went in to address this on arm64, 73e2d827a501 ("arm64: Initialize jump labels before setup_machine_fdt()"). But it appears that fixing it on other platforms might not be so trivial. Instead, defer the setting of the static branch until later in the boot process. Fixes: f5bda35fba61 ("random: use static branch for crng_ready()") Reported-by: Stephen Boyd <swboyd@chromium.org> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Russell King <linux@armlinux.org.uk> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Phil Elwell <phil@raspberrypi.com> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/char/random.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)