On Thu, Mar 10, 2022 at 01:59:05PM -0700, Jason A. Donenfeld wrote:
> > However, one thing that seems a bit odd is that this method can result in two
> > reseeds with very little time in between. For example, if no one is using the
> > RNG from second 40-78, but then it is used in seconds 79-80, then it will be
> > reseeded at both seconds 79 and 80 if there is entropy available.
>
> I've been sort of going back and forth on this. I think the idea is
> that there are two relative time measurements. The ordinary one we use
> is time since last reseeding. But at boot, we're trying to account for
> the fact that entropy is coming in relative to the init process of the
> system, which means we need it to be relative to boot time rather than
> relative to the last reseeding. As you point out, this is a little
> wonky with how things are now, because we only ever reseed on usage.
> To get closer to what I have in mind, we could reseed in a timer, so
> that it hits it exactly on the 5,10,20,40,etc dot. But that seems a
> bit cumbersome and maybe unnecessary. The effect of the behavior of
> this v1 you pointed out is:
>
> - We might reseed at 79, and then fail to reseed at 80. Consequence:
> we lose 1 second of entropy that could have made it for that try.
> - We might reseed at 79, and then also reseed at 80 too. Consequence:
> that's a fairly quick refresh, but on the other hand, we're still
> requiring 256 bit credits, so maybe not so bad, and if we've got so
> much entropy coming in during that small period of time, maybe it
> really isn't a concern.
>
> So I'm not sure either of these cases really matter that much.
>
> > Perhaps the condition should still be:
> >
> > time_after(jiffies, READ_ONCE(base_crng.birth) + interval);
> >
> > ... as it is in the non-early case, but where 'interval' is a function of
> > 'uptime' rather than always CRNG_RESEED_INTERVAL? Maybe something like:
> >
> > interval = CRNG_RESEED_INTERVAL;
> > if (uptime < 2 * CRNG_RESEED_INTERVAL / HZ)
> > interval = max(5, uptime / 2) * HZ;
>
> I'd experimented with things like that, for example making it exponential:
>
> static bool early_boot = true;
> unsigned long interval = CRNG_RESEED_INTERVAL;
>
> if (unlikely(READ_ONCE(early_boot))) {
> unsigned int uptime = min_t(u64, INT_MAX, ktime_get_seconds());
> if (uptime >= CRNG_RESEED_INTERVAL / HZ)
> WRITE_ONCE(early_boot, false);
> else
> interval = (5U << fls(uptime / 5)) * HZ;
> }
> return time_after(jiffies, READ_ONCE(base_crng.birth) + interval);
>
> But the whole thing of combining relative-to-last-reseed with
> relative-to-boottime seems really strange. I'm not quite sure what
> that's supposed to represent, whereas what I have in v1 is
> "exponentially increasing intervals from boottime" which is fairly
> easy to understand.
I don't think it's strange. Maybe it seems strange because of how you wrote it
('interval = (5U << fls(uptime / 5)) * HZ'), where the reseed interval suddenly
jumps from X to 2*X seconds. The version I suggested is 'interval = max(5,
uptime / 2) * HZ', which is smoother. It's simply saying that the reseed
interval increases as the uptime increases, which seems to be what we want.
(Bounded by [5*HZ, CRNG_RESEED_INTERVAL], of course.)
Your current patch interacts badly with how crng_reseed() is a no-op if fewer
than 256 entropy bits are available. Suppose that random bytes are requested at
seconds 10 and 15, and at second 10 there are 200 entropy bits available and at
second 15 there are 256. With your current patch, the CRNG wouldn't be reseeded
before either request, since the no-op reseed at second 10 would consume the
only attempt for the whole interval of 10-19. I.e. the request at second 10
would actually be preventing the CRNG from being reseeded when it should be.
Using an interval since the actual last reseed time (base_crng.birth) avoids
this quirk, as well the one I mentioned before where two reseeds can be done
with very little time in between.
What you have now is still better than the status quo, but I'm not sure it's the
best way.
- Eric
