This topic has come up countless times, and usually doesn't go anywhere.
This time I thought I'd bring it up with a slightly narrower focus,
updated for some developments over the last three years: we finally can
make /dev/urandom always secure, in light of the fact that our RNG is
now always seeded.

Ever since Linus' 50ee7529ec45 ("random: try to actively add entropy
rather than passively wait for it"), the RNG does a haveged-style jitter
dance around the scheduler, in order to produce entropy (and credit it)
for the case when we're stuck in wait_for_random_bytes(). How ever you
feel about the Linus Jitter Dance is beside the point: it's been there
for three years and usually gets the RNG initialized in a second or so.

As a matter of fact, this is what happens currently when people use
getrandom(). It's already there and working, and most people have been
using it for years without realizing.

So, given that the kernel has grown this mechanism for seeding itself
from nothing, and that this procedure happens pretty fast, maybe there's
no point any longer in having /dev/urandom give insecure bytes. In the
past we didn't want the boot process to deadlock, which was
understandable. But now, in the worst case, a second goes by, and the
problem is resolved. It seems like maybe we're finally at a point when
we can get rid of the infamous "urandom read hole".

The one slight drawback is that the Linus Jitter Dance relies on random_
get_entropy() being implemented. The first lines of try_to_generate_
entropy() are:

	stack.now = random_get_entropy();
	if (stack.now == random_get_entropy())
		return;

On most platforms, random_get_entropy() is simply aliased to get_cycles().
The number of machines without a cycle counter or some other
implementation of random_get_entropy() in 2022, which can also run a
mainline kernel, and at the same time have a both broken and out of date
userspace that relies on /dev/urandom never blocking at boot is thought
to be exceedingly low. And to be clear: those museum pieces without
cycle counters will continue to run Linux just fine, and even
/dev/urandom will be operable just like before; the RNG just needs to be
seeded first through the usual means, which should already be the case
now.

On systems that really do want unseeded randomness, we already offer
getrandom(GRND_INSECURE), which is in use by, e.g., systemd for seeding
their hash tables at boot. Nothing in this commit would affect
GRND_INSECURE, and it remains the means of getting those types of random
numbers.

This patch goes a long way toward eliminating a long overdue userspace
crypto footgun. After several decades of endless user confusion, we will
finally be able to say, "use any single one of our random interfaces and
you'll be fine. They're all the same. It doesn't matter." And that, I
think, is really something. Finally all of those blog posts and
disagreeing forums and contradictory articles will all become correct
about whatever they happened to recommend, and along with it, a whole
class of vulnerabilities eliminated.

With very minimal downside, we're finally in a position where we can
make this change.

Cc: Dinh Nguyen <dinguyen@kernel.org>
Cc: Nick Hu <nickhu@andestech.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Guo Ren <guoren@kernel.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Joshua Kinard <kumba@gentoo.org>
Cc: David Laight <David.Laight@aculab.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Eric Biggers <ebiggers@google.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Lennart Poettering <mzxreary@0pointer.de>
Cc: Konstantin Ryabitsev <konstantin@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Having learned that MIPS32 isn't affected by this (initially my largest
worry), and then heartened today upon reading LWN's summary of our
previous discussion ("it would seem there are no huge barriers to
removing the final distinction between /dev/random and /dev/urandom"), I
figured I'd go ahead and submit a v1 of this. It seems at least worth
trying and seeing if somebody arrives with legitimate complaints. To
that end I've also widened the CC list quite a bit.

Changes v0->v1:
- We no longer touch GRND_INSECURE at all, in anyway. Lennart (and to an
  extent, Andy) pointed out that getting insecure numbers immediately at
  boot is still something that has legitimate use cases, so this patch
  no longer touches that code.

 drivers/char/mem.c     |  2 +-
 drivers/char/random.c  | 51 ++++++------------------------------------
 include/linux/random.h |  2 +-
 3 files changed, 9 insertions(+), 46 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index cc296f0823bd..9f586025dbe6 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -707,7 +707,7 @@ static const struct memdev {
 	 [5] = { "zero", 0666, &zero_fops, FMODE_NOWAIT },
 	 [7] = { "full", 0666, &full_fops, 0 },
 	 [8] = { "random", 0666, &random_fops, 0 },
-	 [9] = { "urandom", 0666, &urandom_fops, 0 },
+	 [9] = { "urandom", 0666, &random_fops, 0 },
 #ifdef CONFIG_PRINTK
 	[11] = { "kmsg", 0644, &kmsg_fops, 0 },
 #endif
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8d5abeefcc4f..fda5182d655d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -89,8 +89,6 @@ static LIST_HEAD(random_ready_list);
 /* Control how we warn userspace. */
 static struct ratelimit_state unseeded_warning =
 	RATELIMIT_STATE_INIT("warn_unseeded_randomness", HZ, 3);
-static struct ratelimit_state urandom_warning =
-	RATELIMIT_STATE_INIT("warn_urandom_randomness", HZ, 3);
 static int ratelimit_disable __read_mostly;
 module_param_named(ratelimit_disable, ratelimit_disable, int, 0644);
 MODULE_PARM_DESC(ratelimit_disable, "Disable random ratelimit suppression");
@@ -336,11 +334,6 @@ static void crng_reseed(void)
 				  unseeded_warning.missed);
 			unseeded_warning.missed = 0;
 		}
-		if (urandom_warning.missed) {
-			pr_notice("%d urandom warning(s) missed due to ratelimiting\n",
-				  urandom_warning.missed);
-			urandom_warning.missed = 0;
-		}
 	}
 }
 
@@ -993,10 +986,8 @@ int __init rand_initialize(void)
 		pr_notice("crng init done (trusting CPU's manufacturer)\n");
 	}
 
-	if (ratelimit_disable) {
-		urandom_warning.interval = 0;
+	if (ratelimit_disable)
 		unseeded_warning.interval = 0;
-	}
 	return 0;
 }
 
@@ -1382,20 +1373,16 @@ static void try_to_generate_entropy(void)
  * getrandom(2) is the primary modern interface into the RNG and should
  * be used in preference to anything else.
  *
- * Reading from /dev/random has the same functionality as calling
- * getrandom(2) with flags=0. In earlier versions, however, it had
- * vastly different semantics and should therefore be avoided, to
- * prevent backwards compatibility issues.
- *
- * Reading from /dev/urandom has the same functionality as calling
- * getrandom(2) with flags=GRND_INSECURE. Because it does not block
- * waiting for the RNG to be ready, it should not be used.
+ * Reading from /dev/random and /dev/urandom both have the same effect
+ * as calling getrandom(2) with flags=0. (In earlier versions, however,
+ * they each had different semantics.)
  *
  * Writing to either /dev/random or /dev/urandom adds entropy to
  * the input pool but does not credit it.
  *
- * Polling on /dev/random indicates when the RNG is initialized, on
- * the read side, and when it wants new entropy, on the write side.
+ * Polling on /dev/random or /dev/urandom indicates when the RNG
+ * is initialized, on the read side, and when it wants new entropy,
+ * on the write side.
  *
  * Both /dev/random and /dev/urandom have the same set of ioctls for
  * adding entropy, getting the entropy count, zeroing the count, and
@@ -1480,21 +1467,6 @@ static ssize_t random_write(struct file *file, const char __user *buffer,
 	return (ssize_t)count;
 }
 
-static ssize_t urandom_read(struct file *file, char __user *buf, size_t nbytes,
-			    loff_t *ppos)
-{
-	static int maxwarn = 10;
-
-	if (!crng_ready() && maxwarn > 0) {
-		maxwarn--;
-		if (__ratelimit(&urandom_warning))
-			pr_notice("%s: uninitialized urandom read (%zd bytes read)\n",
-				  current->comm, nbytes);
-	}
-
-	return get_random_bytes_user(buf, nbytes);
-}
-
 static ssize_t random_read(struct file *file, char __user *buf, size_t nbytes,
 			   loff_t *ppos)
 {
@@ -1581,15 +1553,6 @@ const struct file_operations random_fops = {
 	.llseek = noop_llseek,
 };
 
-const struct file_operations urandom_fops = {
-	.read = urandom_read,
-	.write = random_write,
-	.unlocked_ioctl = random_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-	.fasync = random_fasync,
-	.llseek = noop_llseek,
-};
-
 
 /********************************************************************
  *
diff --git a/include/linux/random.h b/include/linux/random.h
index d7354de9351e..38ff777aad19 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -44,7 +44,7 @@ extern void del_random_ready_callback(struct random_ready_callback *rdy);
 extern size_t __must_check get_random_bytes_arch(void *buf, size_t nbytes);
 
 #ifndef MODULE
-extern const struct file_operations random_fops, urandom_fops;
+extern const struct file_operations random_fops;
 #endif
 
 u32 get_random_u32(void);
-- 
2.35.0

On Thu, Feb 17, 2022, at 8:28 AM, Jason A. Donenfeld wrote:
> This topic has come up countless times, and usually doesn't go anywhere.
> This time I thought I'd bring it up with a slightly narrower focus,
> updated for some developments over the last three years: we finally can
> make /dev/urandom always secure, in light of the fact that our RNG is
> now always seeded.
>
> Ever since Linus' 50ee7529ec45 ("random: try to actively add entropy
> rather than passively wait for it"), the RNG does a haveged-style jitter
> dance around the scheduler, in order to produce entropy (and credit it)
> for the case when we're stuck in wait_for_random_bytes(). How ever you
> feel about the Linus Jitter Dance is beside the point: it's been there
> for three years and usually gets the RNG initialized in a second or so.
>
> As a matter of fact, this is what happens currently when people use
> getrandom(). It's already there and working, and most people have been
> using it for years without realizing.
>
> So, given that the kernel has grown this mechanism for seeding itself
> from nothing, and that this procedure happens pretty fast, maybe there's
> no point any longer in having /dev/urandom give insecure bytes. In the
> past we didn't want the boot process to deadlock, which was
> understandable. But now, in the worst case, a second goes by, and the
> problem is resolved. It seems like maybe we're finally at a point when
> we can get rid of the infamous "urandom read hole".
>

This patch is 100% about a historical mistake.  Way back when (not actually that long ago), there were two usable interfaces to the random number generator: /dev/random and /dev/urandom.  /dev/random was, at least in principle, secure, but it blocked unnecessarily and was, therefore, incredibly slow.  It was totally unsuitable for repeated use by any sort of server.  /dev/urandom didn't block but was insecure if called too early.  *But* urandom was also the correct interface to get best-effort-i-need-them-right-now random bits.  The actual semantics that general crypography users wanted were not available.

Fast forward to today.  /dev/random has the correct semantics for cryptographic purposes.  getrandom() also has the correct semantics for cryptographic purposes and is reliable as such -- it is guaranteed to either not exist or to DTRT.  And best-effort users can use GRND_INSECURE or /dev/urandom.

If we imagine that every user program we care about uses GRND_INSECURE for best-effort and /dev/random or getrandom() without GRND_INSECURE for cryptography, then we're in great shape and this patch is irrelevant.

But we don't get to rely on that.  New kernels are supposed to be compatible with old userspace.  And with *old* userspace, we do not know whether /dev/urandom users want cryptographically secure output or whether they want insecure output.  And there is this window during boot that lasts, supposedly, up to 1 second, there is a massive difference. [0]

So, sorry, this patch is an ABI break.  You're reinterpreting any program that wanted best-effort randomness right after boot as wanting cryptographic randomness, this can delay boot by up to a second [0], and that's more than enough delay to be considered a break.

So I don't like this without a stronger justification and a clearer compatibility story.  I could *maybe* get on board if you had a urandom=insecure boot option to switch back to the old behavior and a very clear message like "random: startup of %s is delayed. Set urandom=insecure for faster boot if you do not need cryptographically secure urandom during boot", but I don't think this patch is okay otherwise.

Or we stick with the status quo and make the warning clearer.  "random: %s us using insecure urandom output.  Fix it to use getrandom() or /dev/rando as appropriate."

[0] I just booted 5.16 in a Skylake -rdrand,-rdseed VM and it took 1.14 seconds to initialize.

Hi Andy,

I think your analysis is a bit mismatched from the reality of the
situation. That reality is that cryptographic users still find
themselves using /dev/urandom, as that's been the "standard good
advice" for a very long time. And people are still encouraged to do
that, either out of ignorance or out of "compatibility". The
cryptographic problem is not going away.

Fixing this issue means, yes, adding a 1 second delay to the small
group of init system users who haven't switched to using
getrandom(GRND_INSECURE) for that less common usage (who even are
those users actually?). That's not breaking compatibility or breaking
userspace or breaking anything; that's accepting the reality of _how_
/dev/urandom is mostly used -- for crypto -- and making that usage
finally secure, at the expense of a 1 second delay for those other
users who haven't switched to getrandom(GRND_INSECURE) yet. That seems
like a _very_ small price to pay for eliminating a footgun.

And in general, deemphasizing the rare performance of the less common
usage in favor of fixing a commonly triggered footgun seems on par
with how things morph and change over time. There's no actual
breakage. There's no ABI change violation. What you're saying simply
isn't so.

In other words, I'm not really at all convinced by what you're saying.

Jason

On Wed, Feb 23, 2022 at 06:02:52PM +0100, Jason A. Donenfeld wrote:
> 
> I think your analysis is a bit mismatched from the reality of the
> situation. That reality is that cryptographic users still find
> themselves using /dev/urandom, as that's been the "standard good
> advice" for a very long time. And people are still encouraged to do
> that, either out of ignorance or out of "compatibility". The
> cryptographic problem is not going away.

Or they open /dev/urandom because getrandom() and getentropy() isn't
available on some OS's (all the world is not Linux, despite what the
systemd folks like to believe), and some other OS's have a
/dev/urandom-like device that they can open, and so it's just more
convenient for application programers to open and read from
/dev/urandom.

> Fixing this issue means, yes, adding a 1 second delay to the small
> group of init system users who haven't switched to using
> getrandom(GRND_INSECURE) for that less common usage (who even are
> those users actually?). That's not breaking compatibility or breaking
> userspace or breaking anything; that's accepting the reality of _how_
> /dev/urandom is mostly used -- for crypto -- and making that usage
> finally secure, at the expense of a 1 second delay for those other
> users who haven't switched to getrandom(GRND_INSECURE) yet. That seems
> like a _very_ small price to pay for eliminating a footgun.

I agree.  So long as we're only blocking for short amount of time, and
only during early after the system was booted, people shouldn't care.
The reason why we had to add the "gee-I-hope-this-jitterentropy-like-
hack-is-actually-secure on all architectures but it's better than the
alternatives people were trying to get Linus to adopt" was because
there were systems were hanging for hours or days.

      	   	   		    	  - Ted

On Thu, Feb 17, 2022 at 05:28:48PM +0100, Jason A. Donenfeld wrote:
> This topic has come up countless times, and usually doesn't go anywhere.
> This time I thought I'd bring it up with a slightly narrower focus,
> updated for some developments over the last three years: we finally can
> make /dev/urandom always secure, in light of the fact that our RNG is
> now always seeded.
> 

[ ... ]

This patch (or a later version of it) made it into mainline and causes a
large number of qemu boot test failures for various architectures (arm,
m68k, microblaze, sparc32, xtensa are the ones I observed). Common
denominator is that boot hangs at "Saving random seed:". A sample bisect
log is attached. Reverting this patch fixes the problem.

Guenter

---
# bad: [8565d64430f8278bea38dab0a3ab60b4e11c71e4] Merge tag 'bounds-fixes-v5.18-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux
# good: [f443e374ae131c168a065ea1748feac6b2e76613] Linux 5.17
git bisect start 'HEAD' 'v5.17'
# bad: [5628b8de1228436d47491c662dc521bc138a3d43] Merge tag 'random-5.18-rc1-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/crng/random
git bisect bad 5628b8de1228436d47491c662dc521bc138a3d43
# good: [a04b1bf574e1f4875ea91f5c62ca051666443200] Merge tag 'for-5.18/parisc-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux
git bisect good a04b1bf574e1f4875ea91f5c62ca051666443200
# good: [242ba6656d604aa8dc87451fc08143cb28d5a587] Merge tag 'acpi-5.18-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect good 242ba6656d604aa8dc87451fc08143cb28d5a587
# good: [02b82b02c34321dde10d003aafcd831a769b2a8a] Merge tag 'pm-5.18-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect good 02b82b02c34321dde10d003aafcd831a769b2a8a
# bad: [77553cf8f44863b31da242cf24671d76ddb61597] random: don't let 644 read-only sysctls be written to
git bisect bad 77553cf8f44863b31da242cf24671d76ddb61597
# good: [a07fdae346c35c6ba286af1c88e0effcfa330bf9] random: add proper SPDX header
git bisect good a07fdae346c35c6ba286af1c88e0effcfa330bf9
# good: [58340f8e952b613e0ead0bed58b97b05bf4743c5] random: defer fast pool mixing to worker
git bisect good 58340f8e952b613e0ead0bed58b97b05bf4743c5
# good: [da3951ebdcd1cb1d5c750e08cd05aee7b0c04d9a] random: round-robin registers as ulong, not u32
git bisect good da3951ebdcd1cb1d5c750e08cd05aee7b0c04d9a
# good: [abded93ec1e9692920fe309f07f40bd1035f2940] random: unify cycles_t and jiffies usage and types
git bisect good abded93ec1e9692920fe309f07f40bd1035f2940
# bad: [6f98a4bfee72c22f50aedb39fb761567969865fe] random: block in /dev/urandom
git bisect bad 6f98a4bfee72c22f50aedb39fb761567969865fe
# good: [c2a7de4feb6e09f23af7accc0f882a8fa92e7ae5] random: do crng pre-init loading in worker rather than irq
git bisect good c2a7de4feb6e09f23af7accc0f882a8fa92e7ae5
# first bad commit: [6f98a4bfee72c22f50aedb39fb761567969865fe] random: block in /dev/urandom

On Tue, Mar 22, 2022 at 8:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> This patch (or a later version of it) made it into mainline and causes a
> large number of qemu boot test failures for various architectures (arm,
> m68k, microblaze, sparc32, xtensa are the ones I observed). Common
> denominator is that boot hangs at "Saving random seed:". A sample bisect
> log is attached. Reverting this patch fixes the problem.

Ok, it was worth trying, but yeah, it clearly causes problems for
various platforms that can't do jitter entropy and have nothing else
happening either.

Will revert.

Thanks.

               Linus

Hi Linus,

On Tue, Mar 22, 2022 at 10:27 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Mar 22, 2022 at 8:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > This patch (or a later version of it) made it into mainline and causes a
> > large number of qemu boot test failures for various architectures (arm,
> > m68k, microblaze, sparc32, xtensa are the ones I observed). Common
> > denominator is that boot hangs at "Saving random seed:". A sample bisect
> > log is attached. Reverting this patch fixes the problem.
>
> Ok, it was worth trying, but yeah, it clearly causes problems for
> various platforms that can't do jitter entropy and have nothing else
> happening either.
>
> Will revert.

Shucks. I wish I had a good argument here, but I suppose the most I
can say is maybe we'll be able to revisit it some time off. I'll send
you the revert patch in a minute.

Jason

On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote:

> This patch (or a later version of it) made it into mainline and causes a
> large number of qemu boot test failures for various architectures (arm,
> m68k, microblaze, sparc32, xtensa are the ones I observed). Common
> denominator is that boot hangs at "Saving random seed:". A sample bisect
> log is attached. Reverting this patch fixes the problem.

Just as a datapoint for debugging at least qemu/arm is getting coverage
in CI systems (KernelCI is covering a bunch of different emulated
machines and LKFT has at least one configuration as well, clang's tests
have some wider architecture coverage as well I think) and they don't
seem to be seeing any problems - there's some other variable in there.

For example current basic boot tests for KernelCI are at:

   https://linux.kernelci.org/test/job/mainline/branch/master/kernel/v5.17-1442-gb47d5a4f6b8d/plan/baseline/

for mainline and -next has:

   https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20220322/plan/baseline/

These are with a buildroot based rootfs that has a "Saving random seed: " 
step in the boot process FWIW.

On 3/22/22 11:24, Mark Brown wrote:
> On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote:
> 
>> This patch (or a later version of it) made it into mainline and causes a
>> large number of qemu boot test failures for various architectures (arm,
>> m68k, microblaze, sparc32, xtensa are the ones I observed). Common
>> denominator is that boot hangs at "Saving random seed:". A sample bisect
>> log is attached. Reverting this patch fixes the problem.
> 
> Just as a datapoint for debugging at least qemu/arm is getting coverage
> in CI systems (KernelCI is covering a bunch of different emulated
> machines and LKFT has at least one configuration as well, clang's tests
> have some wider architecture coverage as well I think) and they don't
> seem to be seeing any problems - there's some other variable in there.
> 
> For example current basic boot tests for KernelCI are at:
> 
>     https://linux.kernelci.org/test/job/mainline/branch/master/kernel/v5.17-1442-gb47d5a4f6b8d/plan/baseline/
> 
> for mainline and -next has:
> 
>     https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20220322/plan/baseline/
> 
> These are with a buildroot based rootfs that has a "Saving random seed: "
> step in the boot process FWIW.

I use buildroot 2021.02.3. I have not changed the buildroot code, and it
still seems to be the same in 2022.02. I don't see the problem with all
boot tests, only with the architectures mentioned above, and not with all
qemu machines on the affected platforms. For arm, mostly older machines
are affected (versatile, realview, pxa configurations, collie, integratorcp,
sx1, mps2-an385, vexpress-a9, cubieboard). I didn't check, but maybe
kernelci doesn't test those machines ?

Guenter

From: Guenter Roeck
> Sent: 22 March 2022 21:54
> 
> On 3/22/22 11:24, Mark Brown wrote:
> > On Tue, Mar 22, 2022 at 08:58:20AM -0700, Guenter Roeck wrote:
> >
> >> This patch (or a later version of it) made it into mainline and causes a
> >> large number of qemu boot test failures for various architectures (arm,
> >> m68k, microblaze, sparc32, xtensa are the ones I observed). Common
> >> denominator is that boot hangs at "Saving random seed:". A sample bisect
> >> log is attached. Reverting this patch fixes the problem.
> >
> > Just as a datapoint for debugging at least qemu/arm is getting coverage
> > in CI systems (KernelCI is covering a bunch of different emulated
> > machines and LKFT has at least one configuration as well, clang's tests
> > have some wider architecture coverage as well I think) and they don't
> > seem to be seeing any problems - there's some other variable in there.
> >
> > For example current basic boot tests for KernelCI are at:
> >
> >     https://linux.kernelci.org/test/job/mainline/branch/master/kernel/v5.17-1442-
> gb47d5a4f6b8d/plan/baseline/
> >
> > for mainline and -next has:
> >
> >     https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20220322/plan/baseline/
> >
> > These are with a buildroot based rootfs that has a "Saving random seed: "
> > step in the boot process FWIW.
> 
> I use buildroot 2021.02.3. I have not changed the buildroot code, and it
> still seems to be the same in 2022.02. I don't see the problem with all
> boot tests, only with the architectures mentioned above, and not with all
> qemu machines on the affected platforms. For arm, mostly older machines
> are affected (versatile, realview, pxa configurations, collie, integratorcp,
> sx1, mps2-an385, vexpress-a9, cubieboard). I didn't check, but maybe
> kernelci doesn't test those machines ?

I was trying to fix the buildroot save/restore random seed of a system
of mine.
I thought I'd fixed it - needed to use a persistent filesystem.
But I can't get rid of the 'uninitialised random read' messages.
(Which I expected to go away after writing the seed.)
But a quick look at the kernel code didn't seem to credit the
write into the correct logic.
I didn't check whether the data actually got used though.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote:
> On 3/22/22 11:24, Mark Brown wrote:

> > Just as a datapoint for debugging at least qemu/arm is getting coverage
> > in CI systems (KernelCI is covering a bunch of different emulated
> > machines and LKFT has at least one configuration as well, clang's tests
> > have some wider architecture coverage as well I think) and they don't
> > seem to be seeing any problems - there's some other variable in there.

> I use buildroot 2021.02.3. I have not changed the buildroot code, and it
> still seems to be the same in 2022.02. I don't see the problem with all
> boot tests, only with the architectures mentioned above, and not with all
> qemu machines on the affected platforms. For arm, mostly older machines
> are affected (versatile, realview, pxa configurations, collie, integratorcp,
> sx1, mps2-an385, vexpress-a9, cubieboard). I didn't check, but maybe
> kernelci doesn't test those machines ?

Kind of academic given that Jason seems to have a handle on what the
issues are but for KernelCI it's variations on mach-virt, plus
versatile-pb.  There's a physical cubietruck as well, and BeagleBone
Blacks among others.  My best guess would be systems with low RAM are
somehow more prone to issues.

On 3/23/22 05:10, Mark Brown wrote:
> On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote:
>> On 3/22/22 11:24, Mark Brown wrote:
> 
>>> Just as a datapoint for debugging at least qemu/arm is getting coverage
>>> in CI systems (KernelCI is covering a bunch of different emulated
>>> machines and LKFT has at least one configuration as well, clang's tests
>>> have some wider architecture coverage as well I think) and they don't
>>> seem to be seeing any problems - there's some other variable in there.
> 
>> I use buildroot 2021.02.3. I have not changed the buildroot code, and it
>> still seems to be the same in 2022.02. I don't see the problem with all
>> boot tests, only with the architectures mentioned above, and not with all
>> qemu machines on the affected platforms. For arm, mostly older machines
>> are affected (versatile, realview, pxa configurations, collie, integratorcp,
>> sx1, mps2-an385, vexpress-a9, cubieboard). I didn't check, but maybe
>> kernelci doesn't test those machines ?
> 
> Kind of academic given that Jason seems to have a handle on what the
> issues are but for KernelCI it's variations on mach-virt, plus
> versatile-pb.  There's a physical cubietruck as well, and BeagleBone
> Blacks among others.  My best guess would be systems with low RAM are
> somehow more prone to issues.

I don't think it is entirely academic. versatile-pb fails for me;
if it doesn't fail at KernelCI, I'd like to understand why - not to
fix it in my test environment, but to make sure that I _don't_ fix it.
After all, it _is_ a regression. Even if that regression is triggered
by bad (for a given definition of "bad") userspace code, it is still
a regression.

Thanks,
Guenter

On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 3/23/22 05:10, Mark Brown wrote:
> > On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote:
> > Kind of academic given that Jason seems to have a handle on what the
> > issues are but for KernelCI it's variations on mach-virt, plus
> > versatile-pb.  There's a physical cubietruck as well, and BeagleBone
> > Blacks among others.  My best guess would be systems with low RAM are
> > somehow more prone to issues.
>
> I don't think it is entirely academic. versatile-pb fails for me;
> if it doesn't fail at KernelCI, I'd like to understand why - not to
> fix it in my test environment, but to make sure that I _don't_ fix it.
> After all, it _is_ a regression. Even if that regression is triggered
> by bad (for a given definition of "bad") userspace code, it is still
> a regression.

Maybe kernelci has a virtio-rng device assigned to the machine
and you don't? That would clearly avoid the issue here.

        Arnd

Hey Arnd/Guenter,

On Wed, Mar 23, 2022 at 4:53 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 3/23/22 05:10, Mark Brown wrote:
> > > On Tue, Mar 22, 2022 at 02:54:20PM -0700, Guenter Roeck wrote:
> > > Kind of academic given that Jason seems to have a handle on what the
> > > issues are but for KernelCI it's variations on mach-virt, plus
> > > versatile-pb.  There's a physical cubietruck as well, and BeagleBone
> > > Blacks among others.  My best guess would be systems with low RAM are
> > > somehow more prone to issues.
> >
> > I don't think it is entirely academic. versatile-pb fails for me;
> > if it doesn't fail at KernelCI, I'd like to understand why - not to
> > fix it in my test environment, but to make sure that I _don't_ fix it.
> > After all, it _is_ a regression. Even if that regression is triggered
> > by bad (for a given definition of "bad") userspace code, it is still
> > a regression.
>
> Maybe kernelci has a virtio-rng device assigned to the machine
> and you don't? That would clearly avoid the issue here.

Indeed it's probably something like that. Or maybe they're networked
with something that has a steady stream of interrupts. I say this
because I was able to reproduce Guenter's findings using the
versatilepb machine with the versatile_defconfig config and the
versatile-pb.dtb file. Indeed this board doesn't have a cycle counter.
However, I did have success using the fallback timer and the other
patches in the jd/for-guenter branch, so at least for versatile's
nuances, I think (hope?) there's a reasonable success story here.

Jason

On Sat, Apr 23, 2022 at 02:52:51AM +0200, Jason A. Donenfeld wrote:
> On Wed, Mar 23, 2022 at 4:53 PM Arnd Bergmann <arnd@arndb.de> wrote:

> > Maybe kernelci has a virtio-rng device assigned to the machine
> > and you don't? That would clearly avoid the issue here.

> Indeed it's probably something like that. Or maybe they're networked
> with something that has a steady stream of interrupts. I say this
> because I was able to reproduce Guenter's findings using the
> versatilepb machine with the versatile_defconfig config and the
> versatile-pb.dtb file. Indeed this board doesn't have a cycle counter.
> However, I did have success using the fallback timer and the other
> patches in the jd/for-guenter branch, so at least for versatile's
> nuances, I think (hope?) there's a reasonable success story here.

There's no virtio-rng device being instantiated, unless qemu is doing
that by default (I can't see anything in the logs that suggests it did).
There is networking though.  A sample command for invoking qemu for
versatilepb is:

qemu-system-arm -cpu arm926 -machine versatilepb -nographic -net nic,model=smc91c111,macaddr=52:54:00:12:34:58 -net user -m 256 -monitor none -dtb /var/lib/lava/dispatcher/tmp/85180/deployimages-hitd6sn_/dtb/versatile-pb.dtb -kernel /var/lib/lava/dispatcher/tmp/85180/deployimages-hitd6sn_/kernel/zImage -append "console=ttyAMA0,115200 root=/dev/ram0 debug verbose console_msg_format=syslog earlycon" -initrd /var/lib/lava/dispatcher/tmp/85180/deployimages-hitd6sn_/ramdisk/rootfs.cpio.gz -drive format=qcow2,file=/var/lib/lava/dispatcher/tmp/85180/apply-overlay-guest-l9_f_lxl/lava-guest.qcow2,media=disk,if=scsi,id=lavatest

On Wed, Mar 23, 2022 at 04:53:13PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote:

> > I don't think it is entirely academic. versatile-pb fails for me;
> > if it doesn't fail at KernelCI, I'd like to understand why - not to
> > fix it in my test environment, but to make sure that I _don't_ fix it.
> > After all, it _is_ a regression. Even if that regression is triggered
> > by bad (for a given definition of "bad") userspace code, it is still
> > a regression.

> Maybe kernelci has a virtio-rng device assigned to the machine
> and you don't? That would clearly avoid the issue here.

No, nothing I can see in the boot log:

https://storage.kernelci.org/next/master/next-20220323/arm/versatile_defconfig/gcc-10/lab-baylibre/baseline-qemu_arm-versatilepb.html

and I'd be surprised if virtio devices made it through with a specific
platform emulation.  However it looks like for that test the init
scripts didn't do anything with the random seed (possibly due to running
from ramdisk?) so we'd not have hit the condition.

On Wed, Mar 23, 2022 at 5:18 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Mar 23, 2022 at 04:53:13PM +0100, Arnd Bergmann wrote:
> > On Wed, Mar 23, 2022 at 3:23 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> > > I don't think it is entirely academic. versatile-pb fails for me;
> > > if it doesn't fail at KernelCI, I'd like to understand why - not to
> > > fix it in my test environment, but to make sure that I _don't_ fix it.
> > > After all, it _is_ a regression. Even if that regression is triggered
> > > by bad (for a given definition of "bad") userspace code, it is still
> > > a regression.
>
> > Maybe kernelci has a virtio-rng device assigned to the machine
> > and you don't? That would clearly avoid the issue here.
>
> No, nothing I can see in the boot log:
>
> https://storage.kernelci.org/next/master/next-20220323/arm/versatile_defconfig/gcc-10/lab-baylibre/baseline-qemu_arm-versatilepb.html
>
> and I'd be surprised if virtio devices made it through with a specific
> platform emulation.

In general they do: virtio devices appear as regular PCI devices
and get probed from there, as long as the drivers are available.

It looks like the PCI driver does not get initialized here though,
presumably because it's not enabled in versatile_defconfig.
It used to also not be enabled in multi_v5_defconfig, but I have
merged a patch from Anders that enables it in 5.18 for the
multi_v5_defconfig.

> However it looks like for that test the init
> scripts didn't do anything with the random seed (possibly due to running
> from ramdisk?) so we'd not have hit the condition.

Right.

     Arnd

On Wed, Mar 23, 2022 at 05:41:01PM +0100, Arnd Bergmann wrote:
> On Wed, Mar 23, 2022 at 5:18 PM Mark Brown <broonie@kernel.org> wrote:

> > and I'd be surprised if virtio devices made it through with a specific
> > platform emulation.

> In general they do: virtio devices appear as regular PCI devices
> and get probed from there, as long as the drivers are available.

> It looks like the PCI driver does not get initialized here though,
> presumably because it's not enabled in versatile_defconfig.
> It used to also not be enabled in multi_v5_defconfig, but I have
> merged a patch from Anders that enables it in 5.18 for the
> multi_v5_defconfig.

Ah, I thought Versatile was like the other older Arm reference platforms
and didn't have PCI.