Do not return NULL from the Intel hw PRNG ifunc resolver.

[markj]

Summary

The kernel linker and its users don't handle a symbol value of 0, and
the GNU ifunc documentation suggests that NULL is not a valid return
value from a resolver. ("The implementation functions’ declarations
must match the API of the function being implemented.")

I considered asserting against such a return value in
elf_lookup_ifunc(), but it would simply cause a reset before the console
is initialized and thus be a pain to debug. Maybe it's worth doing
anyway.

[cem]

Huh, I figured linker failure would prevent MOD_LOAD and thus it wouldn't matter. I had thought about providing a 3rd panic function like this but didn't know if it was required and couldn't think of a reason it might be. Guess I was mistaken :-).

sys/dev/random/ivy.c

104 ↗

(On Diff #57228)

it's more like we shouldn't be invoking x86_rng_store at all if neither rdrand nor rdseed was detected -- MOD_LOAD() shouldn't register our callback unless HW support is present.

So I guess I'm saying, this should panic because of an invariants violation, not "missing hardware support".

[markj]

In D20218#435593, @cem wrote:

Huh, I figured linker failure would prevent MOD_LOAD and thus it wouldn't matter. I had thought about providing a 3rd panic function like this but didn't know if it was required and couldn't think of a reason it might be. Guess I was mistaken :-).

Kernel ifuncs are resolved quite early during boot, well before MOD_LOAD. And the linker has no problem with the NULL pointer at ifunc resolution time, it's later when subsystems like DTrace want to examine kernel symbols and trip over a symbol with non-zero size and a value of 0; it's not quite right of me to say that the kernel linker doesn't "handle" this case.

[markj]

Use a better panic message and switch to KASSERT().

[cem]

In D20218#435600, @markj wrote:

Kernel ifuncs are resolved quite early during boot, well before MOD_LOAD.

Right, I expected ifunc resolution long before MOD_LOAD -- it's where has_rdrand etc are initialized for a reason. But I figured the later mod_load was the only possible way for the 0 symbol to be accessed, so as long as it was prevented there. Anyway, dtrace.

And the linker has no problem with the NULL pointer at ifunc resolution time, it's later when subsystems like DTrace want to examine kernel symbols and trip over a symbol with non-zero size and a value of 0; it's not quite right of me to say that the kernel linker doesn't "handle" this case.

Sure, I guess it's really dtrace that doesn't like zero symbols. We could monkey patch this or we could change dtrace to just ignore ifuncs that resolve to zero. I don't know if there are other consumers than dtrace that might stumble over the same obstacle, though it seems like maybe a pain point for further adoption of ifuncs.

sys/dev/random/ivy.c

104 ↗

(On Diff #57229)

Hm, I preferred panic -- KASSERT is compiled out on !INVARIANTS and this is always wrong.

If you prefer KASSERT, we should at least return failure (0) instead of no return value, to match function signature.

[kib]

This is the trail of the dtrace fbt panic ?

sys/dev/random/ivy.c

105 ↗

(On Diff #57229)

I am surprised that this function compiles. You should return something from it.

I vote to panic() always.

[markj]

Go back to panic().

[markj]

sys/dev/random/ivy.c

104 ↗

(On Diff #57229)

Ok. KASSERT seemed reasonable since the function should be unreachable unless there's a bug somewhere.

105 ↗

(On Diff #57229)

It compiles with INVARIANTS on because the branch in the KASSERT() is always taken.

[markj]

In D20218#435623, @cem wrote:

In D20218#435600, @markj wrote:

And the linker has no problem with the NULL pointer at ifunc resolution time, it's later when subsystems like DTrace want to examine kernel symbols and trip over a symbol with non-zero size and a value of 0; it's not quite right of me to say that the kernel linker doesn't "handle" this case.

Sure, I guess it's really dtrace that doesn't like zero symbols. We could monkey patch this or we could change dtrace to just ignore ifuncs that resolve to zero. I don't know if there are other consumers than dtrace that might stumble over the same obstacle, though it seems like maybe a pain point for further adoption of ifuncs.

I think it's reasonable to assume that a function symbol with non-zero size has a sane value. I was thinking of a way to enforce a non-NULL return value at compile-time, akin to the nonnull attribute, but I don't see a good way to do it.

[kib]

sys/dev/random/ivy.c

117 ↗

(On Diff #57232)

In fact you could return x86_rdseed_store if !has_rdrand, hardware then would trap if RDSEED is not implemented.

[markj]

sys/dev/random/ivy.c

117 ↗

(On Diff #57232)

Or even always return x86_rdrand() when !has_rdseed. I think I prefer having an explicit panic() though.