kernel: fix bug in cmdline setup #2716

dianpopa · 2021-09-07T10:51:28Z

Reason for This PR

Fixes #2709.
As per the documentation, everything that is
passed after "--" to the commandline is considered
an argument to init.
However, in Firecracker we are dynamically adjusting the
commandline by appending the virtio mmio devices and the
root device. As such, using "--" with Firecracker would result
in faulty behavior.

Description of Changes

[Author TODO: add description of changes.]

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.
All added/changed functionality is tested.

luminitavoicu

Looks good!

luminitavoicu · 2021-09-07T11:10:22Z

src/kernel/src/cmdline/mod.rs

        self.end_push();

        Ok(())
    }

-    /// Validates and inserts a string to the end of the current command line.
+    /// Validates and inserts a string into the commandline. The position of the
+    /// string depends on the presence of  "--".


I would also specify here that string is inserted before --, if present. It's not clear from the description.

src/kernel/src/cmdline/mod.rs

dianpopa

@luminitavoicu @serban300 take a look at this new version. Thanks!

src/kernel/src/cmdline/mod.rs

serban300 · 2021-09-07T15:15:31Z

src/vmm/src/vmm_config/boot_source.rs

@@ -102,7 +102,7 @@ impl BootConfig {
            Some(str) => str.as_str(),
        };
        cmdline
-            .insert_str(boot_args)
+            .prepend_str(boot_args)


Nitish: We might get away with this because this step is peformed before adding the virtio devices. But in order to make absolutelty certain that everything is ok we would need to make sure that the boot_args are actually appended and everything else is prepended. So do you think we should also have an append_str() method ?

The cmdline is empty when the boot args are "prepended". This is actualy a workaround to the fact the Cmdline does not have a way of getting initialized to a certain value.
However, I just gave it a second thought and it might be that the prepend method is not a very nice choice. Take a look at this:

firecracker/src/vmm/src/builder.rs

Line 849 in 200c2db

let flags = if locked.is_read_only() { "ro" } else { "rw" };

.
We are appending the ro/rw to the string specifying the root device. With the new prepend method, we would need to first create the entire string (i.e "root=/dev/vda ro/rw") and after that call into the Cmdline. IMO this makes things a little counterintuitive.
So I am thinking of reverting to the previous insert_str version. WDYT?

I see. There are other options. We can have 2 "regions" in the string:

one for the firecracker custom args (virtio devices, etc) at the beginning

one for the boot_args at the end
We can keep track of them by keeping the end index for each one. And have 2 groups of methods:

some that insert firecracker custom args. These would go in the first region.

some that insert boot_args. These would go in the second region

Or there might be simpler methods to keep track of the regions. Or we can have 2 strings and concat them when as_string is called. It shouldn't add a significant overhead.

What do you think ?

In rust-vmm we already started adding helper methods on top of insert* functions. For example, we have a function for add_virtio_mmio_device.

Having a new function for insert_boot_args sounds like a good idea to me, as it is inline with existing helpers. This would come with the advantage that the existing Cmdline interface stays the same, so we do not introduce breaking changes for existing customers. With this, most of the changes will be in the implementation details of Cmdline.

In the future we would like to look at other improvements to Cmdline as well such as having it implemented with the builder pattern and allow even more custom/commonly used parameters as specified here: rust-vmm/linux-loader#25.

Nice info! ok, so since the direction in which rust-vmm's linux-loader has already shifted in the same way we want to go with this PR, I will open a PR there, merge it there and then bring the changes here. Thanks @andreeaflorescu for the heads up

andreeaflorescu · 2021-09-07T15:32:24Z

Since in #2470 we are preparing to consume the upstream linux-loader, should we fix this in upstream rust-vmm?

LE: here is the cmdline functionality: https://github.com/rust-vmm/linux-loader/blob/main/src/cmdline/mod.rs

dianpopa · 2021-09-07T15:47:03Z

Since in #2470 we are preparing to consume the upstream linux-loader, should we fix this in upstream rust-vmm?

LE: here is the cmdline functionality: https://github.com/rust-vmm/linux-loader/blob/main/src/cmdline/mod.rs

I am thinking of fixing it in both repos in order to unblock things here.

andreeaflorescu · 2021-09-07T16:33:04Z

I am thinking of fixing it in both repos in order to unblock things here.

Nice, thanks for opening the issue in rust-vmm as well. I'll have a look at the PR as well in this case so we can keep both implementations aligned.

AlexandruCihodaru · 2021-09-18T12:50:48Z

Considering that merging of #2661 unlocked #2470 should we consider adding needed support just in rust-vmm/linux-loader and then cherry-picking your commits on top of #2470?

dianpopa · 2021-09-21T10:01:15Z

Considering that merging of #2661 unlocked #2470 should we consider adding needed support just in rust-vmm/linux-loader and then cherry-picking your commits on top of #2470?

Indeed, since we are now close to moving to rust-vmm's linux-loader, I will do the fix there.

dianpopa · 2021-10-07T15:54:56Z

As per the latest comment in rust-vmm's linux-loader, we agreed that the cmdline implementation needs to be refactored in order to cleanly accommodate the changes required for this fix. As such we decided to implement a temporary fix in Firecracker. The fix was provided by @andreeaflorescu, thanks a lot for this!
@firecracker-microvm/compute-capsule PTAL!

georgepisaltu

Looks good, nice fix!

georgepisaltu · 2021-10-08T07:42:48Z

tests/integration_tests/functional/test_kernel_cmdline.py

+
+    # We will override the init with /bin/cat so that we try to read the
+    # Ubuntu version from the /etc/issue file.
+    vm.basic_config(


Can we think of another test that doesn't involve the use of the serial device? With our future efforts to phase out the serial device, we will eventually have to refactor all tests using it, so it would be great if we could do this now instead of later.

Some examples I can think of are using touch to create an empty file or starting a daemon process, then check for their existence using ssh.

Ok, I will switch to using ssh.

With our future efforts to phase out the serial device,

@georgepisaltu would you be so kind and point me to some discussion and rationale behind this decision if it's publicly available? I'm just curious what future holds for us.

Removing the serial device is something that came up in team discussion as a way to simplify the device model. It is mainly a debugging tool and is not intended for production use-cases, as outlined in our prod-host-setup.md. Gives its relatively large attack surface and poor performance, there are other tools, such as ssh over the net device, to interact with the guest. Even if we remove the serial device as you see it now, the console functionality might be implemented in some other way (maybe through virtio-console).

This is just an idea for now. When making significant changes (e.g. removing the serial device) we will surely reach out to the community for input, as always. So far, we have not yet committed to this idea or scheduled any work in this space, but for the purposes of this PR, using ssh is just the better route regardless of what we decide on the serial device matter.

@georgepisaltu init= will execute the touch and then the vm will exit so I cannot use ssh to check for its existence. There may be some options to check the touch succeeded but the serial one is the most clean one for the moment. Any other suggestions are welcome.

serban300 · 2021-10-11T11:55:01Z

src/vmm/src/builder.rs

+    if let Some(init) = init_params {
+        boot_cmdline.insert_str(format!("--{}", init))?;
+    }
+


Do we need all the logic above (starting from split("--")) ? Instead could we just do boot_cmdline.insert_str(boot_config.cmdline.as_str())?; here ? This way, if the string provided by the user is correct, the part after -- should always be placed at the end of the kernel cmdline.

Are you suggesting just creating an empty command line and just insert everything at the end? That could also work, and would simplify this code. We can have the split between init args and regular args just in linux loader.

Yes, starting with an empty command line, adding the virtio devices and then adding boot_config.cmdline at the end.

Sounds good to me. @dianpopa what do you say?

I am not necessarily a fan of adding the device related info at the start of the commandline just because it is not the usual practice to have them there. Also, I think we need to be consistent with what the commandline will look like when the fix is done in rust-vmm which I guess will not put them at the beginning... @serban300 what are your concerns with current solution? The doc explicitly mentions that the presence of a "--" will be used to specify init params.

I don't have a concern, I just wanted to avoid complicating the code if possible. As long as there are no risks. Also this could eliminate the need for a rust-vmm fix. I'm not sure how much the order of the parameters matter though.

Going back to what Diana said, I think it is better to not invest more time in this and merge it as is. In the end there's just a few lines of code, and it makes more sense to see the command line in dmesg in the order from this PR: first the parameters you set via boot_args, then the virtio configuration that we are adding on top, and lastly the init.

It's ok for me. I agree it's not a blocker.

This split is needed because we're altering the kernel commandline passed by Firecracker customers to add virtio device configuration (on x86_64). The virtio config needs to be specified *before* the init. This is not the ideal implementation of the fix, as it would make more sense to have it in rust-vmm/linux-loader. Due to existing technical debt implementing it directly in upstream is not straightforward. See: rust-vmm/linux-loader#92. Fixes: firecracker-microvm#2709 Signed-off-by: Andreea Florescu <fandree@amazon.com>

Signed-off-by: Diana Popa <dpopa@amazon.com>

serban300 · 2021-10-19T13:32:06Z

src/vmm/src/builder.rs

+    if let Some(init) = init_params {
+        boot_cmdline.insert_str(format!("--{}", init))?;
+    }
+


It's ok for me. I agree it's not a blocker.

dianpopa self-assigned this Sep 7, 2021

dianpopa mentioned this pull request Sep 7, 2021

[Bug] virtio-mmio kernel cmdline append breaks init arguments separated by -- #2709

Closed

3 tasks

luminitavoicu reviewed Sep 7, 2021

View reviewed changes

dianpopa added Status: Awaiting review Indicates that a pull request is ready to be reviewed Status: Author labels Sep 7, 2021

dianpopa force-pushed the little_fixes_2 branch from 9850bc4 to c24f542 Compare September 7, 2021 13:57

serban300 reviewed Sep 7, 2021

View reviewed changes

src/kernel/src/cmdline/mod.rs Outdated Show resolved Hide resolved

dianpopa force-pushed the little_fixes_2 branch from c24f542 to 4e6884c Compare September 7, 2021 15:04

dianpopa commented Sep 7, 2021

View reviewed changes

src/kernel/src/cmdline/mod.rs Outdated Show resolved Hide resolved

serban300 reviewed Sep 7, 2021

View reviewed changes

dianpopa mentioned this pull request Sep 7, 2021

cmdline: port fix for issue reported in Firecracker rust-vmm/linux-loader#92

Closed

andreeaflorescu self-requested a review September 7, 2021 16:33

dianpopa closed this Sep 21, 2021

dianpopa reopened this Oct 7, 2021

dianpopa force-pushed the little_fixes_2 branch from 4e6884c to b002c13 Compare October 7, 2021 15:12

georgepisaltu reviewed Oct 8, 2021

View reviewed changes

dianpopa force-pushed the little_fixes_2 branch 2 times, most recently from 76fb208 to e0b2e06 Compare October 8, 2021 17:19

serban300 reviewed Oct 11, 2021

View reviewed changes

dianpopa force-pushed the little_fixes_2 branch from e0b2e06 to 93f81c6 Compare October 19, 2021 10:52

dianpopa added Type: Bug Indicates an unexpected problem or unintended behavior and removed Status: Author labels Oct 19, 2021

andreeaflorescu and others added 2 commits October 19, 2021 15:24

tests: exercise behavior with kernel init params

6da3f71

Signed-off-by: Diana Popa <dpopa@amazon.com>

changelog: add entry for issue 2709

0062f13

Signed-off-by: Diana Popa <dpopa@amazon.com>

dianpopa force-pushed the little_fixes_2 branch from 2d90c69 to 0062f13 Compare October 19, 2021 12:35

serban300 previously approved these changes Oct 19, 2021

View reviewed changes

georgepisaltu dismissed serban300’s stale review via 0062f13 October 19, 2021 15:41

Merge branch 'main' into little_fixes_2

ffb4614

serban300 approved these changes Oct 20, 2021

View reviewed changes

georgepisaltu approved these changes Oct 20, 2021

View reviewed changes

georgepisaltu merged commit ea8d55c into firecracker-microvm:main Oct 20, 2021

This was referenced Nov 22, 2021

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

Closed

[v0.25] Backport: kernel: fix bug in cmdline setup #2796

Merged

upxe mentioned this pull request Jun 1, 2022

[Bug] FC doesn't parse kernel command lines containing -- correctly #3023

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kernel: fix bug in cmdline setup #2716

kernel: fix bug in cmdline setup #2716

dianpopa commented Sep 7, 2021 •

edited

Loading

luminitavoicu left a comment

luminitavoicu Sep 7, 2021

dianpopa left a comment

serban300 Sep 7, 2021

dianpopa Sep 7, 2021 •

edited

Loading

serban300 Sep 7, 2021

andreeaflorescu Sep 8, 2021

dianpopa Sep 9, 2021

andreeaflorescu commented Sep 7, 2021 •

edited

Loading

dianpopa commented Sep 7, 2021

andreeaflorescu commented Sep 7, 2021

AlexandruCihodaru commented Sep 18, 2021 •

edited

Loading

dianpopa commented Sep 21, 2021

dianpopa commented Oct 7, 2021

georgepisaltu left a comment

georgepisaltu Oct 8, 2021

dianpopa Oct 8, 2021

gregorybrzeski Oct 8, 2021

georgepisaltu Oct 8, 2021

dianpopa Oct 8, 2021

serban300 Oct 11, 2021

andreeaflorescu Oct 11, 2021

serban300 Oct 11, 2021

andreeaflorescu Oct 11, 2021

dianpopa Oct 18, 2021

serban300 Oct 19, 2021 •

edited

Loading

andreeaflorescu Oct 19, 2021

serban300 Oct 19, 2021

serban300 Oct 19, 2021

kernel: fix bug in cmdline setup #2716

kernel: fix bug in cmdline setup #2716

Conversation

dianpopa commented Sep 7, 2021 • edited Loading

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

luminitavoicu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreeaflorescu commented Sep 7, 2021 • edited Loading

dianpopa commented Sep 7, 2021

andreeaflorescu commented Sep 7, 2021

AlexandruCihodaru commented Sep 18, 2021 • edited Loading

dianpopa commented Sep 21, 2021

dianpopa commented Oct 7, 2021

georgepisaltu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serban300 Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianpopa commented Sep 7, 2021 •

edited

Loading

dianpopa Sep 7, 2021 •

edited

Loading

andreeaflorescu commented Sep 7, 2021 •

edited

Loading

AlexandruCihodaru commented Sep 18, 2021 •

edited

Loading

serban300 Oct 19, 2021 •

edited

Loading