libraries should not kill the process.

Please do not emit messages from libc.

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

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

Please move the declaration to the start of the function.

Why not use 32bit generation on 64bit arch ?

The prototype definitely does not need the #ifdef around.

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.

Ok

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

Sure, that would be fine.

Ok

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.

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

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

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).

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

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.

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.

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.

Why use _1 and not _CURR ?

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.

Again, this seems to miss the version check.

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).

There is no check for version there.

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.

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

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

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.

Thanks, will investigate.

And what about la57 ABI ?

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.

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

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

It is newer than this patch. Will add.

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.

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).

One year*

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.

You still did not changed the number.

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