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.

Sign up for GitHub

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

Jump to bottom

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

Closed
3 tasks done
leonjza opened this issue Aug 25, 2021 · 2 comments · Fixed by #2716
Closed
3 tasks done

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

leonjza opened this issue Aug 25, 2021 · 2 comments · Fixed by #2716
Assignees
@dianpopa
Labels
Priority: High Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled Type: Bug Indicates an unexpected problem or unintended behavior

Comments

@leonjza
Copy link

leonjza commented Aug 25, 2021 •
edited
Loading

Describe the bug

Currently, it is not possible to pass arguments to init using -- (ref) as extra arguments are appended after the user provided command line here. When a user uses --, the behaviour causes essential arguments to be ignored as they will be passed to init.

In a bit of a contrived example, but for illustrations sake, let's imagine my init was to /bin/cat a file.
When specifying kernel boot_args such as console=ttyS0 nomodules random.trust_cpu=on reboot=k panic=1 pci=off ip=:::::eth0:dhcp init=/bin/cat -- /etc/password, the aforementioned code block will append extra arguments which technically get discarded because of the the --. The resultant command line would therefore be:

[    0.000000] Command line: console=ttyS0 nomodules random.trust_cpu=on reboot=k panic=1 pci=off ip=:::::eth0:dhcp init=/bin/cat -- /etc/password root=/dev/vda rw virtio_mmio.device=4K@0xd0000000:5 virtio_mmio.device=4K@0xd0001000:6 virtio_mmio.device=4K@0xd0002000:7

To Reproduce

  1. Start firecracker with firecracker --api-sock /tmp/firecracker.socket.
  2. Configure the kernel with the following JSON body:
{
   "kernel_image_path":"/path/to/kern",
   "boot_args":"console=ttyS0 nomodules random.trust_cpu=on reboot=k panic=1 pci=off ip=:::::eth0:dhcp init=/bin/cat -- /etc/password"
}
  1. Configure other parts to get a VM to boot.
  2. Start the instance and observe the Command line: output.

Expected behaviour

Kernel arguments such as root= and virtio_mmio.device= should appear before user supplied arguments.

Environment

  1. Arch : x86_64
  2. OS: Ubuntu 21.04
  3. Firecracker 0.24.6

Checks

  • Have you searched the Firecracker Issues database for similar problems?
  • Have you read the existing relevant Firecracker documentation?
  • Are you certain the bug being reported is a Firecracker issue?
@leonjza leonjza added the Type: Bug Indicates an unexpected problem or unintended behavior label Aug 25, 2021
@serban300 serban300 added the Priority: High Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled label Aug 31, 2021
@dianpopa dianpopa self-assigned this Sep 6, 2021
@dianpopa dianpopa mentioned this issue Sep 7, 2021
Merged
9 tasks
@dianpopa
Copy link
Contributor

dianpopa commented Sep 7, 2021

Hi @leonjza ,

I posted a fix for this issue #2716. Take a look and let me know if this fixes your problem.

@dianpopa dianpopa mentioned this issue Sep 7, 2021
Closed
@andreeaflorescu andreeaflorescu mentioned this issue Sep 13, 2021
Closed
@georgepisaltu
Copy link
Contributor

Pending on this PR in rust-vmm/linux-loader, we will have a fix for this.

andreeaflorescu added a commit to andreeaflorescu/firecracker that referenced this issue Sep 28, 2021
@andreeaflorescu
split the boot args in regular args and init args
92e994b 
This split is needede 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
dept 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>
dianpopa pushed a commit to dianpopa/firecracker that referenced this issue Oct 7, 2021
@andreeaflorescu @dianpopa
split the boot args in regular args and init args
581293a 
This split is needede 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
dept 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>
dianpopa pushed a commit to dianpopa/firecracker that referenced this issue Oct 8, 2021
@andreeaflorescu @dianpopa
split the boot args in regular args and init args
3251a88 
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
depth 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>
dianpopa pushed a commit to dianpopa/firecracker that referenced this issue Oct 8, 2021
@andreeaflorescu @dianpopa
split the boot args in regular args and init args
017cf1a 
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
depth 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>
dianpopa pushed a commit to dianpopa/firecracker that referenced this issue Oct 19, 2021
@andreeaflorescu @dianpopa
builder: place device related info at the start of cmdline
8d230c7 
This is needed because we're altering the kernel commandline
passed by Firecracker customers by appending device configuration (virtio
and root filesystem info). However, the cmdline can contain init params
which are expected to be at the end of the cmdline separated by "--".
Solved this by prepending all device related info.

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
depth 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>
dianpopa pushed a commit to dianpopa/firecracker that referenced this issue Oct 19, 2021
@andreeaflorescu @dianpopa
split the boot args in regular args and init args
6150361 
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>
georgepisaltu pushed a commit that referenced this issue Oct 20, 2021
@andreeaflorescu @georgepisaltu
split the boot args in regular args and init args
5c6ceae 
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: #2709

Signed-off-by: Andreea Florescu <fandree@amazon.com>
alindima pushed a commit to alindima/firecracker that referenced this issue Nov 23, 2021
@andreeaflorescu @alindima
split the boot args in regular args and init args
4ca1f08 
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>
alindima pushed a commit that referenced this issue Nov 24, 2021
@andreeaflorescu @alindima
split the boot args in regular args and init args
aefc93e 
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: #2709

Signed-off-by: Andreea Florescu <fandree@amazon.com>
gbionescu pushed a commit to gbionescu/firecracker that referenced this issue Feb 9, 2022
@andreeaflorescu @gbionescu
split the boot args in regular args and init args
2fff452 
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>
@upxe upxe mentioned this issue Jun 1, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dianpopa dianpopa

Labels
Priority: High Indicates than an issue or pull request should be resolved ahead of issues or pull requests labelled Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

kernel: fix bug in cmdline setup dianpopa/firecracker
4 participants
@leonjza @dianpopa @serban300 @georgepisaltu