Page MenuHomeFreeBSD

kern: Support duplicate variables in early kenv
ClosedPublic

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

Details

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
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cperciva created this revision.

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

@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–429

This comment doesn't match what the code does now, I don't think...

432–441

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...

sys/kern/kern_environment.c
432–441

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.

sys/kern/kern_environment.c
432–441

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.

Update comment and clarify magic numbers

This revision now requires review to proceed.Aug 21 2022, 2:08 AM

minor nit

sys/kern/kern_environment.c
421

s/environement/environment/?

This revision is now accepted and ready to land.Aug 21 2022, 2:17 AM
This revision now requires review to proceed.Aug 21 2022, 2:39 AM
This revision is now accepted and ready to land.Oct 7 2022, 10:42 PM