Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Firecracker drives config appends kernel boot args with a root= making it harder to pass args to init #2793

Closed
3 tasks
fadams opened this issue Nov 21, 2021 · 10 comments
Labels
Type: Bug Indicates an unexpected problem or unintended behavior

Comments

@fadams
Copy link

fadams commented Nov 21, 2021

Describe the bug

Firecracker "drives" config appends kernel boo targs with a root= making it harder to pass args to init

The "boot_args" config largely works well, but one edge case is where one might want to pass args to init.

According to https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html

The kernel parses parameters from the kernel command line up to “--“; if it doesn’t recognize a parameter and it doesn’t contain a ‘.’, the parameter gets passed to init: parameters with ‘=’ go into init’s environment, others are passed as command line arguments to init. Everything after “--” is passed as an argument to init.

so intuitively ending whatever is passed to boot_args with "-- " should allow one to pass those args as per the kernel-parameters docs however the Firecracker startup appends its own boot args - e.g. root=/dev/vda rw virtio_mmio.device=4K@0xd0000000:5 virtio_mmio.device=4K@0xd0001000: so ending boot_args with -- would actually cause the root= to be ignored and cause a kernel panic

Either prepending rather than appending the root= virtio_mmio.device= that Fc is adding or splitting the supplied boot_args on -- and appending what comes after -- to the generated boot_args would probably resolve the issue

To Reproduce

create boot_args that end with "-- "

Expected behaviour

anything after -- in the boot_args gets passed to init as arguments

Environment

[Author TODO: Please supply the following information):]
[ - Firecracker version.]v0.25
[ - Host and guest kernel versions.] host v5.8 guest v5.10.77
[ - Rootfs used.] created from Ubunto 20.04 container filesystem
[ - Architecture.] b86
[ - Any other relevant software versions.]

Checks

  • Have you searched the Firecracker Issues database for similar problems? Y
  • Have you read the existing relevant Firecracker documentation? Y
  • Are you certain the bug being reported is a Firecracker issue? Y
@fadams fadams added the Type: Bug Indicates an unexpected problem or unintended behavior label Nov 21, 2021
@alindima
Copy link
Contributor

Hello! This was actually fixed by the following PR: #2716 on the Firecracker main branch.

I see that you reported this as an issue on v0.25 and indeed this fix was not backported there. Is this something you require?

@fadams
Copy link
Author

fadams commented Nov 22, 2021

Hi, thanks, I'm afraid I hadn't noticed that, sorry.

As for "an issue on v0.25 and indeed this fix was not backported there. Is this something you require?" I'm not precious about v0.25 per se, but is this fix in an actual built release yet? The way I'm using Firecracker is by pulling and unpacking a specific release in an automated deployment thing, so the actual release is configurable but having to build FC from main would be that bit more painful.

@alindima
Copy link
Contributor

There aren't any built releases of Firecracker that include this fix. We synced internally and decided to create a v0.25.2 patch release including this fix, coming soon.
Let's close this issue once v0.25.2 is out

@fadams
Copy link
Author

fadams commented Nov 23, 2021

That'd be great, thanks so much @alindima

One related question, do you know whether this fix deals well with double quotes?

What I mean is imagine say: bash -c 'echo "hello world"', then imagine a firecracker rootfs containing a /bin/bash executable (and all the necessary shared libraries) and a boot_args containing "init=/bin/bash" which should init to bash (and it does).

Given that starting point, it'd be good to support something like "init=/bin/bash -- -c 'echo "hello world"'", if you see what I mean? Now that probably won't work directly 'cause boot_args is a JSON string, but something like "init=/bin/bash -- -c 'echo "hello world"'" (with escaped double quotes) should, I think, be a legitimate set of args and should in an ideal world be passed to bash (running as init in Firecracker). That's kind of a simplified version of one of the things I'm trying to achieve and the "bash -c" thing appears straightforward, but the quoting quirks can make it a fun test case.

@alindima
Copy link
Contributor

Hmm try using "init=/bin/sh ... -- -c \"echo 'hello world'\"", this one works for me

@alindima
Copy link
Contributor

Firecracker v0.25.2 is released and it includes the bugfix: https://github.com/firecracker-microvm/firecracker/releases/tag/v0.25.2

Thanks

@fadams
Copy link
Author

fadams commented Nov 25, 2021

I'm so sorry to bother you on this again.

I've just tried v0.25.2 and the boot_args in the vmconfig I'm using look like this.

"boot_args": "ro noapic console=ttyS0 reboot=k panic=1 pci=off random.trust_cpu=on i8042.noaux i8042.nomux i8042.nopnp i8042.dumbkbd tsc=reliable ipv6.disable=1 loglevel=7 init=/bin/bash ip=172.16.0.2::172.16.0.1:255.255.255.0::eth0:off  -- '-c' 'echo \"hello, world\"'"

as far as I can see the args after the -- look fine with the inner double quotes escaped.

This launches FC correctly, so the JSON seems OK too and also looking at the kernel logs I see

[    0.000000] Command line: ro noapic console=ttyS0 reboot=k panic=1 pci=off random.trust_cpu=on i8042.noaux i8042.nomux i8042.nopnp i8042.dumbkbd tsc=reliable ipv6.disable=1 loglevel=7 init=/bin/bash ip=172.16.0.2::172.16.0.1:255.255.255.0::eth0:off   root=/dev/vda rw virtio_mmio.device=4K@0xd0000000:5 virtio_mmio.device=4K@0xd0001000:6 -- '-c' 'echo "hello, world"'

which actually looks like I'd expect, and there's a later one:

[    0.025020] Kernel command line: ro noapic console=ttyS0 reboot=k panic=1 pci=off random.trust_cpu=on i8042.noaux i8042.nomux i8042.nopnp i8042.dumbkbd tsc=reliable ipv6.disable=1 loglevel=7 init=/bin/bash ip=172.16.0.2::172.16.0.1:255.255.255.0::eth0:off   root=/dev/vda rw virtio_mmio.device=4K@0xd0000000:5 virtio_mmio.device=4K@0xd0001000:6 -- '-c' 'echo "hello, world"'

again, I think that's looking like I'd expect

the weird thing however is that when I actually check what is being passed to init, I seem to be seeing this:

'-c' 'echo hello, world"'

That is to say the double quote in front of the h in hello is missing, which is just plain weird.

Starting again with -- -c \"echo 'hello world'\"" as you mentioned previously, so the boot_args looks like:

"boot_args": "ro noapic console=ttyS0 reboot=k panic=1 pci=off random.trust_cpu=on i8042.noaux i8042.nomux i8042.nopnp i8042.dumbkbd tsc=reliable ipv6.disable=1 loglevel=7 init=/bin/bash ip=172.16.0.2::172.16.0.1:255.255.255.0::eth0:off  -- -c \"echo 'hello world'\""

gives

-c echo 'hello world'

which looks like I'd expect, but I really can't account for why using -- '-c' 'echo \"hello, world\"'" yields '-c' 'echo hello, world"'as I say without that first double quote, especially as it seems to look OK in the args reflected in the kernel logs.

I'm pretty sure this is nothing dumb I've done and I created a trivial init the does

#!/bin/sh
echo "$@"

to check it wasn't my imagination.

Given that the "Kernel command line" section of the kernel boot logs actually looks OK it probably isn't a Firecracker thing, and I'm suspecting that it might be a bug in how the kernel itself is parsing those args before it passes them to init. I'm using kernel 5.10.77

It'd be really useful if you were able to replicate what I'm seeing, if it's a kernel bug so be it, but I'd quite like to rule out any potential FC thing (or indeed something I've messed up on)

@alindima
Copy link
Contributor

Hmm I think it has something to do with the way the kernel is parsing these arguments:
https://www.kernel.org/doc/html/latest/admin-guide/kernel-parameters.html

To quote the documentation:

Double-quotes can be used to protect spaces in values, e.g.:
param="spaces in here"

I see that there was a patch sometime ago for adding single-quote support but I see it wasn't merged

@fadams
Copy link
Author

fadams commented Nov 25, 2021

Thanks for the response, yeah that's definitely my suspicion too, I was mostly looking for a sanity check that it wasn't something stupid I've done. I can probably work around it, though probably won't be pretty :-)

Thanks again for getting back to me on these things so quickly, I really do appreciate it!

@alindima
Copy link
Contributor

You're welcome! Glad I could help :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

2 participants