random(4) fenestrasX: Push root seed version to arc4random(3)

[cem]

Summary

Push the root seed version to userspace through the VDSO page.

arc4random(3) obtains a pointer to the root seed version published by
the kernel in the shared page at allocation time. Like arc4random(9),
it maintains its own per-process copy of the seed version corresponding
to the root seed version at the time it last rekeyed. On read requests,
the process seed version is compared with the version published in the
shared page; if they do not match, arc4random(3) reseeds from the
kernel.

This change does not implement the fenestrasX concept of PCPU userspace
generators seeded from a per-process base generator. That change is
left for future discussion/work.

[kib]

lib/libc/gen/arc4random.c

138

libraries should not kill the process.

lib/libc/gen/arc4random.h

134

Please do not emit messages from libc.

136

This puns on int/long for ILP32. Since you use #ifdef __LP64 already, I think this should be type-correct as well.

sys/kern/kern_sharedpage.c

303

Why do you need this persistent 'zero' structure with single use ? You can copy from zero_region and save some memory.

340

Please move the declaration to the start of the function.

sys/sys/vdso.h

66

Why not use 32bit generation on 64bit arch ?

97

The prototype definitely does not need the #ifdef around.

[cem]

lib/libc/gen/arc4random.c

138

Right. This assertion should never fire.

Note that this library is already special in that _rs_init() abort()s if _rs_allocate() fails. This is not new in this revision.

lib/libc/gen/arc4random.h

134

Ok

136

How does it pun on int/long? fxrng_root_generationp is unsigned long*

sys/kern/kern_sharedpage.c

303

Sure, that would be fine.

340

Ok

sys/sys/vdso.h

66

Why use 32bit generation on 64bit arch? The 64 bit counter is canonical and cheap on 64bit, might as well stick with the longer generation there.

97

Hm, I'm not sure why not. It exists only conditional on option RANDOM_FENESTRASX

[kib]

lib/libc/gen/arc4random.h

136

Hm, yes, you do type-punning with fxrng_root_generation instead. One more reason to use 32bit IMO.

sys/sys/vdso.h

66

I am not convinced that 64bit are useful if 32bit are enough.

But regardless, I do not think that fx_generation32 is needed if you provide fx_generation. 32bit read can be done from the proper word of fx_generation (high word on BE).

97

But prototype is valid regardless. Also it puts a dependency on the opt_ header before this one.

[cem]

• Remove warn from libc
• Use more verbose 32/64 atomic spelling instead of long
• Use zero_region
• C89-ify a scoped variable declaration to top of function

[cem]

sys/sys/vdso.h

66

I thought we already discussed that reading 32 bits out of kernel 64 bit atomic write wasn't safe. I'm confused why you propose it now.

The generation32 costs nothing; we've already allocated a full cache line. We have 52 bytes to spare.

I will leave it as is.

97

The prototype is not valid if the option isn't present; that's the point.

If the option isn't present, using that declaration will eventually result in a link error. It's better to cause a compile error earlier on than a link error at the end of the kernel build, IMO.

Opt header is not a concern; opt_global.h already precedes all headers.

[kib]

sys/sys/vdso.h

66

We discussed that reading both high and low words is not safe because we can see inconsistent results without some form of lock. But reading just LSW is fine, even if the write is not 64bit atomic.

[kib]

I have one more suggestion for this stuff. Put a version into the struct vdso_fxrng_generation, and make the current version number one. This would allow to expand it later without allocating another AT_ auxv id, and even more important, it allows to implement global disable knob by zeroing out version. I found a way to disable vdso timehands quite life-saving when users reported some issues with it (implemented as a sysctl knob).

[cem]

Version the VDSO structure.

[kib]

There should be a knob which switches the version to 0, disabling the feature at runtime.
I stil think that having both 64 and 32bit data word with identical content is ugly and even erroneous ABI decision.

lib/libc/gen/arc4random.h

115

Why use _1 and not _CURR ?

[cem]

Add sysctl knob to disable VDSO page ABI.

[cem]

lib/libc/gen/arc4random.h

115

Why _CURR? This version of the code understands version 1 of the ABI; if CURR is bumped, this code should still be associated with version 1, not whatever moving target CURR is.

[kib]

I still do not understand the need for both fx_generation and fx_generation32, and why fx_generation is 64bit.

lib/libc/gen/arc4random.c

127

Again, this seems to miss the version check.

lib/libc/gen/arc4random.h

115

This is the common practice, look e.g. at the sys/conf.h. The motivation is that ver 2 typically differs from ver 1 by some adjustments, instead of being rewritten from scratch. Correspondingly, the code gets some fiddling with, which makes one less place to change (and forget).

137

There is no check for version there.

[cem]

Update to latest draft.

[cem]

lib/libc/gen/arc4random.c

127

Why check the version repeatedly? Most other VDSO knobs of this kind are checked only at process startup; I don't see any reason to pessimize this use in particular.

[cem]

Like the other patches in this series, I would like to land this soon. Like the other patches, it should not have any functional change on systems without a RANDOM_FENESTRASX kernel. I've addressed all of kib's earlier objections that I understood.

[kib]

lib/libc/gen/arc4random.c

127

Other vdso data blobs (timehands) allow to dynamically disable its use. The only mechanism your vdso_fxrngp provides for this is version check.

sys/kern/kern_sharedpage.c

353

This function needs update to init sv2->sv_handle fxrng_gen_base.

[cem]

lib/libc/gen/arc4random.c

127

This can be done by just halting update publishing on the VDSO page. sysctl_fxrng_vdso and fxrng_push_seed_generation do not do that yet, but could pretty easily.

sys/kern/kern_sharedpage.c

353

Thanks, will investigate.

[cem]

• Initialize fxrng offset in secondary sysvecs
• Halt seed version update while/if shared page feature is disabled, which has the effect of disabling the feature for existing processes

[kib]

sys/amd64/amd64/elf_machdep.c

110

And what about la57 ABI ?

sys/kern/kern_sharedpage.c

319

This is very strange 'disable'. It only makes newly exec-ed processes to ignore the fxrng_shpage, and if process saw _DISABLED then it is forever (until exec or exit).

I really do not see this 'disable' as useful, it provides very non-obvious behavior. Timecounter provides the 'disable' switch as an emergency measure which appeared useful more than once in field debugging and making people machines operational. Main point of it was that all instances of libc reacted to it right away.

357–361

Assert should be added that secondary sysvec has the same value of SV_RNG_SEED_VER flag as the primary.

sys/sys/elf_common.h

970

Why is it 99 ? You are blowing several arrays without a reason.

[cem]

sys/amd64/amd64/elf_machdep.c

110

It is newer than this patch. Will add.

sys/kern/kern_sharedpage.c

319

I really don't know what you want in a disable mechanism. I don't see how it can possibly be useful for this feature. Just as a reminder, RANDOM_FENESTRASX will land completely compiled-out of the GENERIC kernel anyway. I've tried to add a disable mechanism because you want it, but now you are complaining that it can't be re-enabled for processes that saw it was disabled.

I don't understand the concern on either end. I don't think this needs a disable mechanism, nor do I think it is especially significant how quickly the feature is re-enabled if the user already turned it off once.

I've been running this code in kernel and libc on my workstation and test VMs for something like two years now without issue.

sys/sys/elf_common.h

970

This can be dropped to 33 before commit. It worked better for a WIP patch, when there were repeated collisions with other new auxv values taking the final slot (all of BSDFLAGS through PS_STRINGS collided with this change at one point in time).

[cem]

Add to LA57 amd64 ABI.

[cem]

sys/kern/kern_sharedpage.c

319

One year*

[kib]

sys/kern/kern_sharedpage.c

319

Normally disable in vdso causes all currently running FreeBSD-ABI processes to stop using corresponding vdso facility. In reverse, enable causes them to start using it at the moment of enablement. This is not true for your variant of disable, which only acts once for the program, at exec time.

I am sure that if somebody sees potential issues with the patch the issue is fixed in advance, the 'disable' knob is for emergency and debug. It helped me to save several user machines for timecounter.

Anyway, I do not want to block the feature on that disagreement. Please fix AT_FXRNG numeric value and go ahead.

sys/sys/elf_common.h

970

You still did not changed the number.

[cem]

• Simplify disable mechanism
• Drop AT_ define to next contiguous index

[cem]

Add missing assertion that secondary sysvec flags matches primary for FXRNG.

[kib]

sys/kern/kern_sharedpage.c

358

Does this assert and assignment to base below need #ifdef braces ? If support is not compiled in, base should be NULL. Flag should be consistent anyway.

[cem]

sys/kern/kern_sharedpage.c

358

No, I guess it doesn't. Will fix.

[cem]

• Drop ifdef from exec_sysvec_init_secondary() fxrng assertion, base ptr initialization

[arichardson]

This broke building on macOS+Linux (https://github.com/freebsd/freebsd/runs/1236550352) due to

fatal error: sys/vdso.h: No such file or directory

Could you please check whether D26738 is an appropriate way of fixing this?