Lgtm

Nice observation and fix.

No objection. I'll let someone else stamp it.

Lgtm.

Nevermind

LGTM

As mentioned in email, we might also consider publishing a seed generation to the VDSO page from Fortuna.

Something to keep in mind re: exactly 64 is that it's possible for entropy sources to return less entropy than was requested, if for example some internal state means they don't have anything available at the moment. That's one reason I prefer polling repeatedly until seeded (or some spin threshold is crossed).

Discussed with: cem

Thanks.

Seems reasonable to me.

Seems very reasonable to me. Thanks!

Seems reasonable to me!

LGTM. My only bikeshed contribution is maybe gctl_get_devnameparam or get_devparam. But I don’t object to the current name.

Thanks.

Setting cstd seems strongly preferable vs making upstream updates more difficult.

Either way seems fine to me. I’d ask vangyzen or bdrewery for input.

What workflow?

In D34220#774261, @sg2342_googlemail.com wrote:

subtract one from vt_logo_sprite_height as well

In D34186#773413, @dim wrote:

In D34186#773402, @cem wrote:

Looks like TESTAB expects boolean (0 and 1) values, too.

Well, the interesting thing is that this macro produces values 0 through 4, and it's a sort of weird shortcut to make the switch(TESTAB()) construct possible.

Looks like TESTAB expects boolean (0 and 1) values, too.

In D34152#772589, @hselasky wrote:

@cem: You mean "iff" ? if and only if?

Looks fine to me. We should probably also actively feed jitter entropy if we hit the blocked state on boot, but that doesn't mean we shouldn't just do this first.

Typo in summary: s/path/patch/

Thanks.

I’m hoping emaste will be able to test and commit it.

LGTM, thanks.

LGTM modulo overflow concern and what looks like a typo.

Lgtm. I didn’t attempt to verify the asm implementation.

Seems ok in principle.

At a minimum, you have rdseed (rdrand). But I expect there are other non-ethernet sources present as well.

The problem is that in some use cases we might not have a lot of entropy good sources, with the ethernet being the only good candidate.

Consider just disabling Ethernet entropy collection instead. In fact, I thought it was off by default in approximately the 13 timeframe. Maybe even 12.

Cool

Fortuna doesn't specify the 100ms sleep behavior, as far as I can tell. Removing it seems reasonable to me.

This change should come with a motivational ministat graph.

I meant fundamentally bad idea. Very little work should happen in interrupt context. Taking a global lock and running AES in software is somewhat expensive.

I don't believe anything should be consuming random in interrupt contexts. Could you elaborate on the scenario / bug?

Address of packed member? How?

Lgtm, thanks for driving this.

Let’s re-remove read_rate_increment but otherwise it’s looking good to me.

In D32013#722497, @kevans wrote:

OK, I can throw together a patch to this effect. Based on your description here, we're tentatively punting on the idea of just collecting into the zero pool for now, right? My understanding is that 128 bits into the zero pool is the bare minimum and 256 bits per pool per second as a good compromise between there and where we're at now.

In D32013#722391, @kevans wrote:

If that's an option, then sure. =-) I didn't/don't understand fortuna's design well enough to make claims about what a reasonable rate is. If we can securely drop it down to <= 1kB/s on average then yeah, we can probably just not care about this at all.

I hadn’t seen the patch moving the stack buffer to the softc when I wrote my earlier remarks. I still think we should be polling less volume.

As mentioned earlier in the stack I don’t think this mitigation is great.

I don’t think this is a great mitigation for random - the pending request will still be written in guest memory, and we need the queue completion to know when we can free the memory.

Lgtm.

LTO builds can see across CUs. I don’t know of any particular pass that would eliminate this, though.

In D31510#710608, @markj wrote:

What exactly is the UB here? I'm not sure how the compiler could legally eliminate SHA256_Update(&ctx, key, sizeof(key)).

It's not exactly a false positive, although poisoning the output as uninitialized is sort of unhelpful. Are we confident the compiler isn't eliminating access to it (due to UB) outside of KMSAN? In the abstract, I think we would prefer to eliminate bypass_before_seeding.

Oops. Use C89-style comments per style(9).

Add comment explaining the cast.

In D31327#705944, @bdrewery wrote:

I don't know exactly but I figured it was simple latency with something like debugnet_arp_gw sending arp request 1, delay(500), retry -> arp request 2 sent, arp reply 1 received (state changes), delay(500), arp reply 2 received (panic due to unexpected state).

This would only happen if you have multiple competing dhcp servers on a subnet, right? Edit: not dhcp, oops. Still, kind of surprising we’re getting multiple ARP responses?

Use ssize_t.

In D31292#704741, @kib wrote:

ptrdiff_t sounds arbitrary. Why did not you used e.g. ssize_t? Because it is not defined by C STD?

Is there a way we could make MB_CUR_MAX or __mb_cur_max suck less, in a more general way? Like, isn't it basically always four or one?

I think the idea is very reasonable and could be done in a way that preserves the safety of the sandbox, at least for fstyp. I don't think the popen approach makes sense. There may be a lot of work required to sandbox the suggested consumer (mount).