Page MenuHomeFreeBSD

kenv: allow static hints to disable loader hints
AcceptedPublic

Authored by kevans on Oct 13 2021, 6:40 AM.
Tags
None
Referenced Files
F80187651: D32477.id97973.diff
Thu, Mar 28, 11:19 PM
Unknown Object (File)
Jan 10 2024, 2:16 AM
Unknown Object (File)
Dec 26 2023, 5:46 PM
Unknown Object (File)
Dec 25 2023, 12:18 PM
Unknown Object (File)
Nov 25 2023, 10:12 PM
Unknown Object (File)
Nov 2 2023, 4:40 AM
Unknown Object (File)
Oct 28 2023, 9:59 PM
Unknown Object (File)
Oct 2 2023, 7:42 PM

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 Passed
Unit
No 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
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.

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?

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.

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
402

/* 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
1046

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?

kevans added inline comments.
sys/kern/kern_environment.c
1036–1037

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
235–236

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.

sys/kern/kern_environment.c
1036–1037

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