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

Don't adopt systemd boot loaders #462

Merged
merged 2 commits into from
May 5, 2023
Merged

Don't adopt systemd boot loaders #462

merged 2 commits into from
May 5, 2023

Conversation

dbnicholson
Copy link
Contributor

Skip adoption if systemd-boot or systemd-stub are used since bootctl will handle updating them. The 2nd commit is IMO correct but more controversial. I needed it to test that our sd-boot system would be detected and skipped. It did work in that case, though:

Apr 11 10:56:49 endless bootupd[3086]: [DEBUG bootupd::efi] Reusing existing "/boot"
Apr 11 10:56:49 endless bootupd[3086]: [TRACE bootupd::efi] No CoreOS aleph detected
Apr 11 10:56:49 endless bootupd[3086]: [TRACE bootupd::efi] Skipping adoption for "systemd-boot 251.2+dev56.9738984-22bem1"

I'm not that familiar with writing rust tests, but I think the UTF-16 string function could be written to take an optional root and then a test that writes various stuff into <testroot>/sys/firmware/efi/efivars/<somevar> could be added.

@openshift-ci
Copy link

openshift-ci bot commented Apr 11, 2023

Hi @dbnicholson. Thanks for your PR.

I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cgwalters
Copy link
Member

/ok-to-test

cgwalters
cgwalters approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane overall, just a few minor nits

src/efi.rs
return None;
}
let path = efivars.join(name);
if !path.exists() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional cleanup in the future: We do have an open_file_optional helper

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice that.

src/efi.rs
// Drop the last byte if there's an odd number.
let size = slice.len() / 2;
let v: Vec<u16> = (0..size)
.map(|i| u16::from_ne_bytes([slice[2 * i], slice[2 * i + 1]]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use https://doc.rust-lang.org/std/primitive.slice.html#method.chunks here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that does look nicer. My rust noobness shows through.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code is great honestly! Learning a language (programming or not) is always a journey; even the relatively small std in Rust has a lot of surface area and idioms.

src/efi.rs
@@ -105,6 +110,70 @@ impl Efi {
}
}

/// Convert a nul-terminated UTF-16 byte array to a String.
fn string_from_utf16_bytes(slice: &[u8]) -> String {
// For some reason, systemd appends 3 nul bytes after the string.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, messy. But I wonder if we should care about this? Maybe instead we return a Vec<u8> since in the end all we care about is if it starts with the ASCII string systemd right?

OK as is though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't just send an array of bytes (which you already have from reading the file contents) back since they're be misinterpreted as UTF-8 since every other byte is nul. That's why you have to convert it to Vec<u16> and parse it as UTF-16. I don't believe there's any spec that says you have to write UTF-16 strings to EFI variables. I think that's just a convention systemd follows since that's how other vendors do it. I think I read a comment in the systemd code that they append 3 nul bytes so that in case they start parsing in the middle of the buffer they'll still end up with a valid C string. However, stripping an odd numbered trailing byte seems like good practice.

When I wrote roughly the same code in Python where I didn't have to change types, I walked backwards from the end of the buffer until I found no more pairs of nul bytes. Here I let U16CString handle that part.

Copy link
Member

@cgwalters cgwalters May 8, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes...hm. What about going the other way, and converting from systemd in UTF-8 to UTF-16, and then using https://doc.rust-lang.org/std/primitive.slice.html#method.starts_with ?

That way we can also just ignore the trailing NULs.

@cgwalters cgwalters added the triaged This issue was triaged label May 5, 2023
cgwalters
cgwalters approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine as is, we can do followups later.

@cgwalters
Copy link
Member

Hmm...I don't think the CI failure here is related, I see it on other PRs, but now I'm a bit confused why CI worked in #450 but not elsewhere.

@dbnicholson @cgwalters
efi: Skip adopting for systemd-boot and systemd-stub
4debd1c 
If the system is booted with systemd-boot or systemd-stub then `bootctl`
will handle updating the boot components. Both of these components write
identifying information to EFI variables as NUL terminated UTF-16
strings. Check these variables to detect if either systemd-boot or
systemd-stub are in use and skip adoption if so.
@dbnicholson @cgwalters
efi: Allow /boot as the ESP
8c5d4e7 
Some systems use the ESP as the entire `/boot` filesystem. This check
needs to go before `boot/efi` since that would also detect the FAT
filesystem but would expect the `EFI` directory at `/boot/efi/EFI`
instead of `/boot/EFI`. A more typical system with the ESP mounted at
`/boot/efi` would not have use FAT for the `/boot` filesystem and would
be skipped in this check.
@cgwalters
Copy link
Member

Tried rebasing this just to rerun CI and see if the failure is somehow fixed in main but I think the problem is rooted in #229

@cgwalters cgwalters merged commit 9d80017 into coreos:main May 5, 2023
6 checks passed
@dbnicholson dbnicholson deleted the no-adopt-systemd-boot branch May 8, 2023 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgwalters cgwalters cgwalters approved these changes

Assignees
No one assigned
Labels
ok-to-test triaged This issue was triaged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dbnicholson @cgwalters