Fix possible bugs when loading ELF/PVH kernel #29

jiangliu · 2020-03-22T08:38:34Z

This series includes:

a patch to exactly matching PVH entry point in ELF note section.
do not assume ELF program headers are sorted ascendantly by virtual address.
refine the dependency relationship to vm-memory.
Thanks!

sameo · 2020-03-24T07:15:20Z

This LGTM but I'd like to get @aljimenezb review on that one.

src/loader/mod.rs

andreeaflorescu · 2020-03-24T08:47:08Z

@jiangliu is it possible to also add unit tests for the bug fixes? I strongly encourage to have regression tests whenever we fix bugs to make sure we don't introduce them again.

aljimenezb · 2020-03-24T16:09:22Z

src/loader/mod.rs

+#[cfg(feature = "elf")]
+#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
+// Size of string "PVHNote", including the terminating NULL.
+const PVH_NOTE_STR_SZ: usize = 8;
+


I agree that checking for both name and type when searching for the PVH note is the right approach, for the reason @jiangliu mentions in the commit message. This will avoid any future collisions due to another namespace using type 0x12 as well.

This patch will not work correctly yet, because the name specified in the PVH ABI is actually just "Xen". So the code above must be changed to:

// Size of string "Xen", including the terminating NULL. const PVH_NOTE_STR_SZ: usize = 4;

The unit tests I added on this commit use "PVHNote" as the name to generate the test_*note.bin binaries and they must be corrected. I can generate new binaries and share them, or they can be built using the instructions in the commit. Please let me know if you'd like me to do this.

@jiangliu I built new binaries that are needed to fix the unit tests. Binaries, source and instructions on how to build are here:
https://gist.github.com/aljimenezb/9918b8562070cb89ab6696025305e3f9

Yes, the linux kernel use "Xen" and I used "Xen" too at first, but it fails the unit tests:(
I will update it in next version.

@aljimenezb can you add the instructions about generating the binaries to this repository as well? It would be easier if someone else needs to generate some in the future.

@andreeaflorescu Sure, I'll open a separate PR for that change. That content feels a little bit out of place in the main README, but we can figure out the details on the discussion.

Yeah, maybe we can have a contributing document? Or development?

Another idea would be for those binaries to be generated at build time. I am not a big fun of having binaries in repositories.

Another idea would be for those binaries to be generated at build time. I am not a big fun of having binaries in repositories.

That was my first instinct, but I saw test_elf.bin was already in use so I followed that simpler approach instead.

I am not familiar with the capabilities of inline assembly in Rust, so I'd like feedback from others here as to whether or not it is possible to build the assembly source code using Rust inline assembly templates.

src/loader/mod.rs

rbradford

This PR breaks PVH boot (the code goes back through the vmlinux) path when it was tested with Cloud Hypervisor. I recommend you put a debug statement in arch::x86_64::configure_system to print out the value of boot_proto.

aljimenezb · 2020-03-25T18:22:04Z

This PR breaks PVH boot (the code goes back through the vmlinux) path when it was tested with Cloud Hypervisor. I recommend you put a debug statement in arch::x86_64::configure_system to print out the value of boot_proto.

@rbradford I am assuming you tested with the current code, and without the changes I suggested above. In that case yes, it will break PVH boot since the Linux kernel uses "Xen" as the name for the note header that encodes the PVH address.

After applying my proposed changes above, I booted successfully using the PVH path on Cloud Hypervisor. If you did the same and still found the error please let me know and I'll take another look.

rbradford · 2020-03-25T18:39:24Z

@rbradford I am assuming you tested with the current code, and without the changes I suggested above. In that case yes, it will break PVH boot since the Linux kernel uses "Xen" as the name for the note header that encodes the PVH address.

Indeed.

sboeuf · 2020-03-26T08:13:47Z

@jiangliu just wanted to mention that I tried this PR yesterday, and it breaks the PVH support.

jiangliu · 2020-03-26T08:20:30Z

@jiangliu just wanted to mention that I tried this PR yesterday, and it breaks the PVH support.

Yes, the patch should be updated to checking "Xen" instead of "PVHNote".
Will send out updated version soon.

Refine dependency on vm-memory crate by: 1) relax version number constraint to "0.x.y" 2) enable feature vm-memory/backend-mmap for development only because it's used by unit test cases. Signed-off-by: Liu Jiang <[email protected]>

According to Linux kernel documentation: Each note has three parts: a name, a type and a desc. The name is intended to distinguish the note's originator, so it would be a company, project, subsystem, etc; it must be in a suitable form for use in a section name. The type is an integer which is used to tag the data, and is considered to be within the "name" namespace (so "FooCo"'s type 42 is distinct from "BarProj"'s type 42). The "desc" field is the actual data. There are no constraints on the desc field's contents, though typically they're fairly small. So check both name and type when searching for the PVH note. Signed-off-by: Liu Jiang <[email protected]>

aljimenezb

Minor nits about comments and one correctness issue.

src/loader/x86_64/elf/mod.rs

josephlr · 2020-03-30T11:09:26Z

rust-hypervisor-firmware recently added PVH support in cloud-hypervisor/rust-hypervisor-firmware#26.

I tested this patch w/ Cloud-Hypervisor, and I can confirm that this patch doesn't break anything with regards to the firmware. We can still boot via the PVH Boot protocol.

rbradford

lgtm based on @josephlr feedback

aljimenezb

LGTM with one minor nit:
The addition of the new test ELF binaries should be moved to commit d19aea9 rather than where it is now at commit be5edd4.

Sorry I didn't catch this before. This is not a stopper, but besides keeping the logical consistency of the commits, it will preserve the ability to run git bisect without the unit tests failing.

Do not assume program header is sorted ascendantly by virtual address, otherwise loader_result.kernel_end may be wrong. Signed-off-by: Liu Jiang <[email protected]>

aljimenezb

LGTM. Thank you @jiangliu.

jiangliu force-pushed the bugfix_v1 branch from 78165be to 6c9fba5 Compare March 22, 2020 09:02

jiangliu requested review from andreeaflorescu, rbradford and bjzhjing March 23, 2020 14:52

alxiord self-requested a review March 23, 2020 15:03

bjzhjing reviewed Mar 24, 2020

View reviewed changes

src/loader/mod.rs Outdated Show resolved Hide resolved

aljimenezb suggested changes Mar 24, 2020

View reviewed changes

sboeuf mentioned this pull request Mar 25, 2020

Initramfs does not work with vmlinux cloud-hypervisor/cloud-hypervisor#963

Closed

rbradford requested changes Mar 25, 2020

View reviewed changes

aljimenezb mentioned this pull request Mar 25, 2020

Document procedure to generate ELF binaries used in unit tests #30

Open

jiangliu added 2 commits March 28, 2020 16:11

Refine dependency on vm-memory

9f17f6b

Refine dependency on vm-memory crate by: 1) relax version number constraint to "0.x.y" 2) enable feature vm-memory/backend-mmap for development only because it's used by unit test cases. Signed-off-by: Liu Jiang <[email protected]>

jiangliu force-pushed the bugfix_v1 branch 2 times, most recently from 1a0fecb to 025db34 Compare March 28, 2020 08:56

jiangliu requested review from bjzhjing, rbradford and aljimenezb March 28, 2020 09:06

aljimenezb suggested changes Mar 28, 2020

View reviewed changes

src/loader/x86_64/elf/mod.rs Outdated Show resolved Hide resolved

src/loader/x86_64/elf/mod.rs Outdated Show resolved Hide resolved

src/loader/x86_64/elf/mod.rs Outdated Show resolved Hide resolved

jiangliu force-pushed the bugfix_v1 branch from 025db34 to be5edd4 Compare March 30, 2020 05:32

jiangliu requested a review from aljimenezb March 30, 2020 05:47

rbradford previously approved these changes Mar 30, 2020

View reviewed changes

aljimenezb suggested changes Mar 30, 2020

View reviewed changes

Do not assume program header is sorted by VA

8febf53

Do not assume program header is sorted ascendantly by virtual address, otherwise loader_result.kernel_end may be wrong. Signed-off-by: Liu Jiang <[email protected]>

jiangliu dismissed rbradford’s stale review via 8febf53 March 30, 2020 15:15

jiangliu force-pushed the bugfix_v1 branch from be5edd4 to 8febf53 Compare March 30, 2020 15:15

jiangliu requested review from aljimenezb and rbradford April 1, 2020 01:36

aljimenezb approved these changes Apr 1, 2020

View reviewed changes

alxiord approved these changes Apr 1, 2020

View reviewed changes

Merge branch 'master' into bugfix_v1

fc3c12a

sameo approved these changes Apr 2, 2020

View reviewed changes

sameo merged commit 43d1c51 into rust-vmm:master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix possible bugs when loading ELF/PVH kernel #29

Fix possible bugs when loading ELF/PVH kernel #29

jiangliu commented Mar 22, 2020

sameo commented Mar 24, 2020

andreeaflorescu commented Mar 24, 2020

aljimenezb Mar 24, 2020

aljimenezb Mar 24, 2020

jiangliu Mar 25, 2020

andreeaflorescu Mar 25, 2020

aljimenezb Mar 25, 2020

andreeaflorescu Mar 25, 2020

aljimenezb Mar 25, 2020 •

edited

Loading

rbradford left a comment •

edited

Loading

aljimenezb commented Mar 25, 2020

rbradford commented Mar 25, 2020

sboeuf commented Mar 26, 2020

jiangliu commented Mar 26, 2020

aljimenezb left a comment

josephlr commented Mar 30, 2020

rbradford left a comment

aljimenezb left a comment

aljimenezb left a comment

Fix possible bugs when loading ELF/PVH kernel #29

Fix possible bugs when loading ELF/PVH kernel #29

Conversation

jiangliu commented Mar 22, 2020

sameo commented Mar 24, 2020

andreeaflorescu commented Mar 24, 2020

aljimenezb Mar 24, 2020

Choose a reason for hiding this comment

aljimenezb Mar 24, 2020

Choose a reason for hiding this comment

jiangliu Mar 25, 2020

Choose a reason for hiding this comment

andreeaflorescu Mar 25, 2020

Choose a reason for hiding this comment

aljimenezb Mar 25, 2020

Choose a reason for hiding this comment

andreeaflorescu Mar 25, 2020

Choose a reason for hiding this comment

aljimenezb Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

rbradford left a comment • edited Loading

Choose a reason for hiding this comment

aljimenezb commented Mar 25, 2020

rbradford commented Mar 25, 2020

sboeuf commented Mar 26, 2020

jiangliu commented Mar 26, 2020

aljimenezb left a comment

Choose a reason for hiding this comment

josephlr commented Mar 30, 2020

rbradford left a comment

Choose a reason for hiding this comment

aljimenezb left a comment

Choose a reason for hiding this comment

aljimenezb left a comment

Choose a reason for hiding this comment

aljimenezb Mar 25, 2020 •

edited

Loading

rbradford left a comment •

edited

Loading