Page MenuHomeFreeBSD

kenv: allow static hints to disable loader hints
AcceptedPublic

Authored by kevans on Oct 13 2021, 6:40 AM.

Details

Reviewers
jah
mhorne
imp
Group Reviewers
manpages
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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 42215
Build 39103: arc lint + arc unit

Event Timeline

sys/kern/kern_environment.c
360

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

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
105

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.

sys/kern/subr_hints.c
105

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?

sys/kern/subr_hints.c
105

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.

sys/kern/kern_environment.c
360

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 added inline comments.
sys/kern/kern_environment.c
360

static_hints.disabled=1, of course

Address some of the review commentary (manpage, _res_checkenv)

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

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

sys/kern/kern_environment.c
401

/* Remove disabled hints from the kenv */

1036–1037

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

This revision is now accepted and ready to land.Nov 3 2021, 9:53 PM
gbe added a subscriber: gbe.

LGTM for the man page parts.

mhorne added inline comments.
sys/kern/kern_environment.c
1045

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

pauamma_gundo.com added inline comments.
usr.sbin/config/config.5
235–236

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?