MIME-Version: 1.0
References: <20211012082708.121931-1-iivanov@suse.de> <YWVKAk4h5bsUA3b6@light.dominikbrodowski.net>
 <YaivhAV8LouB0zGV@light.dominikbrodowski.net> <CAHmME9qxBeBzfKCjzfAFX9ZWAGKv1TKCQw3x22d_DmJtaAewLw@mail.gmail.com>
 <YanOIvAV1iPBEXR3@light.dominikbrodowski.net>
In-Reply-To: <YanOIvAV1iPBEXR3@light.dominikbrodowski.net>
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
Date:   Fri, 3 Dec 2021 16:39:55 +0100
Message-ID: <CAHmME9qGHo4n6QGxnE+O46pagOR0bA+9E8bi8ZLPAzMuMZpPwg@mail.gmail.com>
Subject: Re: [PATCH v4] random: fix crash on multiple early calls to add_bootloader_randomness()
To:     Dominik Brodowski <linux@dominikbrodowski.net>
Cc:     "Theodore Ts'o" <tytso@mit.edu>,
        "Ivan T. Ivanov" <iivanov@suse.de>,
        Ard Biesheuvel <ardb@kernel.org>, linux-efi@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>, hsinyi@chromium.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Dominik,

Thanks for your analysis. Some more questions:

On Fri, Dec 3, 2021 at 8:59 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
> On subsequent calls to add_bootloader_randomness() and then to
> add_hwgenerator_randomness(), crng_fast_load() will be skipped. Instead,
> wait_event_interruptible() (which makes no sense for the init process)
> and then credit_entropy_bits() will be called. If the entropy count for
> that second seed is large enough, that proceeds to crng_reseed().
> However, crng_reseed() may depend on workqueues being available, which
> is not the case early during boot.

It sounds like *the* issue you've identified is that crng_reseed()
calls into workqueue functions too early in init, right? The bug is
about paths into crng_reseed() that might cause that?

If so, then specifically, are you referring to crng_reseed()'s call to
numa_crng_init()? In other words, the cause of the bug would be
6c1e851c4edc ("random: fix possible sleeping allocation from irq
context")? If that's the case, then I wonder if the problem you're
seeing goes away if you revert both 6c1e851c4edc ("random: fix
possible sleeping allocation from irq context") and its primary
predecessor, 8ef35c866f88 ("random: set up the NUMA crng instances
after the CRNG is fully initialized"). These fix an actual bug, so I'm
not suggesting we actually revert these in the tree, but for the
purpose of testing, I'm wondering if this is actually the root cause
of the bug you're seeing.

Also, if you have a nice way of reproducing this, please do tell - I'd
like to give it a spin if possible.

Regards,
Jason