kenv: allow static hints to disable loader hints

[kevans]

Summary

This is useful for downstream vendors that cater to specific hardware
and depend on some set hints; risk of loader clobbering something
critical can be effectively mitigated.

X-NetApp-PR: #68
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.

[jah]

sys/kern/kern_environment.c

360–368

Should this logic be structured to match the env check right above it, so that loader hint disablement takes precedence over static hint disablement?

[mhorne]

Seems good to me. One thing missing is loader_hints.disabled needs an entry in the config(5) man page.

sys/kern/subr_hints.c

106

Would this check be better placed around the caller of _res_checkenv(md_envp)? Seems odd to have it in this helper, but I defer to your opinion here.

[mhorne]

sys/kern/subr_hints.c

106

Also, am I right in thinking that this is all to catch the PRESERVE_EARLY_KENV case, where the hints values aren't actually sanitized?

[kevans]

sys/kern/subr_hints.c

106

Ah, nope- I'll push it into the caller. I meant to move it post-extraction and forgot about it.

This is specifically for hint-checking that happens before the dynamic kenv is setup (before PRESERVE_EARLY_KENV comes into play) -- I want to say a number of the resource_* checks in sys/x86. Once the dynamic kenv is setup we'll have ignored it when merging into there.

[kevans]

sys/kern/kern_environment.c

360–368

Sorry for the delay here; I've been kind of crunching through this one. I think you're right (for consistency with loader env disablement), but I've been trying to work through whether it should really be mutually exclusive with static_hints.disabled or not (like it is in the current diff). If you set static_hints.disabled=yes when booting a kernel that has loader hints disabled, do we really care if you've potentially shot yourself in the foot?

I note that we don't do anything special there for environment disablement, and NetApp's use-case doesn't care if they're mutually exclusive or not.

[kevans]

sys/kern/kern_environment.c

360–368

static_hints.disabled=1, of course

[kevans]

Address some of the review commentary (manpage, _res_checkenv)

[kevans]

Check loader_hints.disabled first, don't allow static hints to be disabled if
loader hints are disabled. Document that this is the case.

[imp]

one maybe suggestion of a comment... but it's a suggestion that might not be needed.

sys/kern/kern_environment.c

402

/* Remove disabled hints from the kenv */

1036

This always sets data, but before it would only set the data if we found val

[gbe]

LGTM for the man page parts.

[mhorne]

sys/kern/kern_environment.c

1046

The truthiness of return (ret == 0) should be enough?

[pauamma_gundo.com]

usr.sbin/config/config.5

238–239

Is this saying that a "foo=bar" hint will keep any "foo=quux" in an earlier file (and thus seen later because of the reversal) from being processed?

[kevans]

sys/kern/kern_environment.c

1036

I'm afraid I don't quite see it- kenv_to_bool() only updates *data if we had a non-NULL val that matched one of our truthy/falsey values

usr.sbin/config/config.5

238–239

Right, because the kernel processes assignments and the first one it encounters for the name foo wins, so this statement tries to map how we get the kernel's expected semantics from what is actually written in the configuration. It's mostly an important detail because the reversed order is what you'll see if you dump hints from the kernel binary or observe the generated file.

[imp]

sys/kern/kern_environment.c

1036

derp! I don't see it now either. I likely overlooked the NULL return in my haste.