random(4): Simplify RANDOM_LOADABLE

[cem]

Summary

Simplify RANDOM_LOADABLE by removing the ability to unload a LOADABLE
random(4) implementation. The most common use for a RANDOM_LOADABLE module
is to supply one's own FIPS random module. That can be done once, at boot.
Swapping modules on the fly doesn't seem especially useful.

This removes the need to hold a lock over the sleepable module calls
read_random and read_random_uio.

[cem]

sys/dev/random/random_infra.c

124 ↗

(On Diff #64751)

On second thought, this unrelated change may break userspace load (arc4random will be fine, but there are many direct read_random callers and I have not audited all of them for is_random_seeded checks). So I will drop it from a subsequent revisions of this diff.

143–144 ↗

(On Diff #64751)

This change makes 3-4 assumptions:

1. Pointer-sized and natively aligned stores are atomic on all FreeBSD arch.
2. Pointer-sized and natively aligned loads are similarly atomic.
3. If the arbitrary order of function pointer initialization is significant to the loadable implementation, the loadable random module will invoke random_infra_init before callers.

Are 1-2 true, or should we instead use the atomic_store_rel / atomic_load_acq wrappers? Maybe that would show intent better even if the assumption is true.

My 4th assumption is this: that we care about being able to load a random.ko from kldload in userspace (for example, because the architecture does not use loader(8), or some other reason). If we don't need to be able to kldload from userspace, we could simplify this a *ton* by removing all the wrappers and having the loadable random provide the read_random_foo and is_random_seeded symbols directly. That would remove all of the assumptions above, and still remove the need for any configuration lock.

Thoughts?

I don't know what the consumers of random_loadable actually want from the interface. I'd prefer to just remove support entirely and let downstreams hack their proprietary random modules into their own downstream source trees in a way that meets their needs. Does anyone else consider that an option?

[markm]

sys/dev/random/random_infra.c

143–144 ↗

(On Diff #64751)

When I did the last load of major code updates in this area, there was a LOT of pushback, some of it idealistic (the people who wanted every choice to be made by the owner/operator, at least by high principle), and some of it from the folks who wanted/needed *small* systems with most components unloadable if not deletable. Most of those objections seem to have quietened down now.

I'd be *delighted* to see the RANDOM_LOADABLE facility gone. Keep the KLD ability, please - it's nice to be able to test e.g. the NIST algorithms.

[markm]

The whole change LGTM, but please go further and remove RANDOM_LOADABLE altogether if the consensus allows!

[cem]

Simplify LOADABLE further.

Pull init/deinit out of the random_algorithm and let algorithms run sysinits
to initialize themselves at SI_SUB_RANDOM:SI_ORDER_SECOND. In LOADABLE
systems, algorithms should install p_random_alg_context at that time.
deinit has been removed entirely.

struct random_algorithm contexts are constant, so go ahead and mark them so.

Remove the NULL checks in random_harvestq; by initializing random_harvestq's
hc_harvest_mask at SI_ORDER_THIRD, after algorithm init, the relevant
pointers will already be initialized.

Remove most of the random_infra shim wrappers and replace them with plain
function pointers and macro magic in sys/random.h. On LOADABLE kernels,
read_random(9) et al are just macros around invoking the associated function
pointer. There is no need to provide a registration system; LOADABLE
modules are simply expected to initialize the pointers at SI_SUB_RANDOM:
SI_ORDER_SECOND, early in boot. An example is provided in randomdev.c.

Tested with random_fortuna.ko on a dev vm; it seems to work fine.

[markm]

This is a GREAT step forward, so I'm happy to accept it as such. The objection to #undefs is stylistic/idealistic, so you may ignore it at your pleasure.

Happy Christmas and ahve a jolly good new year!

UPDATING

31 ↗

(On Diff #65940)

Strong approval here!

sys/dev/random/random_harvestq.c

169 ↗

(On Diff #65940)

Seeing these go gives me MUCH pleasure!

sys/dev/random/randomdev.c

73 ↗

(On Diff #65940)

Does this have to be?

I'm prejudiced against #undef. :-)

sys/dev/random/randomdev.h

119 ↗

(On Diff #65940)

More joy!

sys/sys/random.h

68 ↗

(On Diff #65940)

Can those #undefs above not be reworked to here somehow?

[cem]

In D22512#501907, @markm wrote:

This is a GREAT step forward, so I'm happy to accept it as such. The objection to #undefs is stylistic/idealistic, so you may ignore it at your pleasure.

Happy Christmas and ahve a jolly good new year!

Thanks Mark! You as well!

sys/dev/random/randomdev.c

73 ↗

(On Diff #65940)

I think it may be possible. Here is my idea (untested): the (LOADABLE) function pointers already have non-conflicting names; leaving the existing (LOADABLE) sys/random.h macros. Then just define the (LOADABLE) version of these symbols with the ordinary names, rather than the suffixed names; they won't conflict. The macro can be avoided with void (read_random)( ... ) { } in the declarations/definitions, and function pointer assignment. I'll give it a shot. I will have intermittent availability for the next handful of days, so hopefully no rush :-).

sys/sys/random.h

68 ↗

(On Diff #65940)

No, unfortunately. The idea is that these are the standard definitions everyone gets -- so "read_random(9)" just works regardless of LOADABLE. The undefs in randomdev were an implementation detail for that file only. But I think it may be possible to avoid them, so I'll give it a shot.

[gordon]

sys/dev/random/other_algorithm.c

114 ↗

(On Diff #65940)

Tiny nit. Update the comment to match the new function declaration.

sys/dev/random/random_harvestq.c

169 ↗

(On Diff #65940)

Do we have any notion of how much faster this will be without the locking?

378 ↗

(On Diff #65940)

Are there any potential problems with this being reordered to THIRD that breaks assumptions about availability?

[cem]

sys/dev/random/other_algorithm.c

114 ↗

(On Diff #65940)

static? Maybe just remove the comment entirely since it purely duplicates the code?

sys/dev/random/random_harvestq.c

169 ↗

(On Diff #65940)

Honestly, speed wasn't a factor I'd considered at all. That's not the motivation for the change. GENERIC sets nooption RANDOM_LOADABLE, so it didn't have these locks before this change.

378 ↗

(On Diff #65940)

I will double check, but I don't believe so.

[cem]

sys/dev/random/random_harvestq.c

378 ↗

(On Diff #65940)

I don't believe this one is problematic. The most problematic ordering is that algorithms (i.e., Fortuna), and randomdev.c both initialize at SI_SUB_RANDOM:SI_ORDER_SECOND. However, neither actually depends on the other, and SI_SUB_RANDOM is long before SI_SUB_SMP or any concurrency kicks off. So I think we're fine in practice.

sys/dev/random/randomdev.c

73 ↗

(On Diff #65940)

Ok, fixed!

[cem]

• Remove use of undef by using parentheses to prevent macro expansion
• Remove useless comment in deadcode other_algorithm

[markm]

I'm delighted with the amount of OS-interface and locking code that is removed!

sys/dev/random/randomdev.c

73 ↗

(On Diff #65940)

Yay! :-)