kern: Support duplicate variables in early kenv
ClosedPublic
Actions

Authored by cperciva on Aug 13 2022, 12:16 AM.

Details

Reviewers

kevans
imp

Commits

rGc32bd97641da: kern: Support duplicate variables in early kenv

Summary

Some virtual machines pass virtio MMIO device parameters via the kernel
command line as a series of virtio_mmio.device=<parameters> options.
These get translated into FreeBSD kernel environment variables; but
unfortunately they all use the same variable name, which resulted in
all but the first such parameter being ignored when the dynamic kernel
environment is set up from the initial environment buffers.

With this commit, duplicate environment settings will instead be stored
as ${name}_1, ${name}_2... ${name}_9999. In the unlikely event that
the same variable is set over 10000 times before the dynamic kernel
environment is set up, we panic.

Variable settings after the dynamic environment is initialized continue
to override the previously-set value; the change is limited to the very
early kernel boot (prior to SI_SUB_KMEM + 1) and changes behaviour from
"ignore" to "store with a different name" only.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 46916
Build 43805: arc lint + arc unit

Event Timeline

cperciva requested review of this revision.Aug 13 2022, 12:16 AM

cperciva created this revision.

Harbormaster completed remote builds in B46916: Diff 109281.Aug 13 2022, 12:16 AM

cperciva added a child revision: D36189: virtio_mmio: Support command-line parameters.Aug 13 2022, 12:55 AM

@kevans On IRC it sounded like you were ok with this -- was that "ok with the concept" or "ok with the code"?

In D36187#822594, @cperciva wrote:

@kevans On IRC it sounded like you were ok with this -- was that "ok with the concept" or "ok with the code"?

Both seem fine to me; loader shouldn't naturally generate environments that add duplicates like this, so there's not really a problem with the possibility of, e.g., geli passphrases, not getting removed properly. The consistency issue with these not being set as such in the pre-dynamic environment is a bit wonky, but I don't think that's all too big of a deal.

sys/kern/kern_environment.c
370	I'd maybe like to see `9999` spelled here and below as a `#define` (perhaps just thrown right above that allocation so it draws attention to the `4` that's derived from the limit if someone gets a wild hair and finds a reason to change it)

This revision is now accepted and ready to land.Aug 17 2022, 12:35 AM

So I think the comments need to be better and explain why the code is right.
I think it might be, but there's something not lining up that some more context will likely clear up for me.

sys/kern/kern_environment.c
418	This comment doesn't match what the code does now, I don't think...
421	So how do we replace env vars that were in the loader env with ones in the kernenv? This seems to affect the behavior globally, and not just for the command line that you want to see this behavior for. I'm still confused how you sort this all out. Maybe this just needs to be sorted out in fixing the above comment, but I'm having trouble connecting the dots from the commit description through this code through the rest of it...

kevans added inline comments.Aug 17 2022, 3:45 AM

sys/kern/kern_environment.c
421	Most kenv manipulation will still work like you'd expect, since this is a one-time thing after kmem comes online to create the dynamic environment. It does break overriding static env vars with loader env vars, but I'm not convinced anyone else actually takes advantage of those being able to co-exist since I made it work... and I don't specify anything in the static env that loader may care about.

kevans added inline comments.Aug 17 2022, 3:51 AM

sys/kern/kern_environment.c
421	Oh, no, it doesn't really break that, either. We process them in reverse order so that loader env does override static env, so static env just unintuitively gets the indexed names.