efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE #669
Conversation
HuijingHei
changed the title
HuijingHei
force-pushed
the
esp_temp
branch
2 times, most recently
from
3a7cd83
to
f3bb105
Compare
src/efi.rs
Outdated
| fn esp_path_tmp(&self) -> Result<PathBuf> { | ||
| self.ensure_mounted_esp(Path::new("/")) | ||
| .map(|v| v.join(".EFI.tmp")) | ||
| } |
There was a problem hiding this comment.
I think it'd be clearer to have just const TEMP_EFI_DIR: &str = ".EFI.tmp"; or so and use it where needed?
There was a problem hiding this comment.
But...related to @travier's comment...it'd be good to try to scope this to the "lowest" directory we need to make the change.
For us that means e.g. we only affect EFI/fedora for example and not all of EFI.
There was a problem hiding this comment.
The bootupd-state.json also includes BOOT/BOOTX64.EFI and BOOT/fbx64.efi under EFI/, both are from shim-x64, we might never do the update? Is it necessary to check or just do not care?
There was a problem hiding this comment.
I think it should be reasonable to prepare both the new BOOT and fedora dirs and then do fedora first, BOOT second.
I don't think there is a dependency "across" those folders for the bootloader files, i.e. grub.efi may depend on an update shim.efi from the same folder but not on an update fbx.efy from the BOOT folder.
I'm not sure however.
There was a problem hiding this comment.
There was a problem hiding this comment.
See #669 (comment), will prepare both the new BOOT and fedora dirs and then do fedora first, BOOT second.
There was a problem hiding this comment.
I don't think there is a dependency "across" those folders for the bootloader files, i.e. grub.efi may depend on an update shim.efi from the same folder but not on an update fbx.efy from the BOOT folder.
Hmmmm...this kind of rains on the parade here though given that shim includes files in these two directories, and we can't update it atomically without updating everything in EFI, which also includes potentially other files we don't manage.
There was a problem hiding this comment.
According to @travier 's suggestion, copy first sub dir as temp (like BOOT/foo -> BOOT.tmp/foo) and remember, then do remove/addition/change files in temp dir, finally scan temp and do exchange. I think it might be helpful when we do A/B updates, instead of current updates based on loop updates dirs.
There was a problem hiding this comment.
Yes, this just means that shim updates won't be transactional, which is unfortunate as shim is the main one we need to update.
It should be an improvement...but...I'm a bit uncertain about saying it's enough that we unconditionally turn on auto-updates by default?
It feels like shim/grub update infrequently enough that in practice we could just accept the hit today of the non-transactionality and recommend turning on auto-updates in various distros/OSes?
I'd be roughly in favor of doing that in FCOS and fedora bootc today just to start.
There was a problem hiding this comment.
It should be an improvement...but...I'm a bit uncertain about saying it's enough that we unconditionally turn on auto-updates by default?
Not sure, is it necessary to check the shim files integrity (like sha512) after sync to provide more confidence?
HuijingHei
force-pushed
the
esp_temp
branch
2 times, most recently
from
b635467
to
e91cab0
Compare
🤔 |
HuijingHei
force-pushed
the
esp_temp
branch
2 times, most recently
from
366b8eb
to
a6c8400
Compare
HuijingHei
force-pushed
the
esp_temp
branch
2 times, most recently
from
d033a4e
to
ad56520
Compare
|
|
We've merged the CI fixes. Can you rebase this one? Thanks |
Sure, updated, thanks! |
HuijingHei
force-pushed
the
esp_temp
branch
3 times, most recently
from
79cb3ad
to
3cf8636
Compare
|
Updating |
|
If using |
HuijingHei
changed the title
|
The problem with copying the entire |
src/filetree.rs
Outdated
| path.with_file_name(buf) | ||
| let mut parent = String::from(""); | ||
| if let Some(p) = path.parent() { | ||
| parent = format!("{}.tmp", p.display()); |
There was a problem hiding this comment.
Use fedora.tmp instead of .fedora.tmp, in case if the parent is like a/b, then it will be .a/b.tmp which is not expected.
|
Update the change in
|
| let path = path.as_ref(); | ||
| let mut buf = path.file_name().expect("filename").to_os_string(); | ||
| buf.push(TMP_PREFIX); |
There was a problem hiding this comment.
Seems the file name is likea.btmp., so to check should not use start_with, but maybe I am wrong.
|
That looks ok but the logic feels a little bit difficult to follow. I might have missed something however so maybe my understanding is not correct. I'm thinking about the following logic for
|
Copy the changed files to prepared temp dir, the final result will not be as expected if the change dir is included by another change. For example, if changes are
|
With I.e. we need to find the first subdir in |
This is complicated as we do not know which comes first, we just scan them in diff.additions, diff.changes, diff.removals. If the parent already in temp dir, then no need to create sub temp dir; if no, will copy to temp dir; This will make the update slow as we do a lot of checking, WDYT? |
|
I'm not sure I understand what you mean by "which comes first". I think the idea is more to think of it as the first subdir in the If I have the following update/addition/removal for example:
Then the logic would be:
|
Sorry for the confusion, if the changes are
If the change is like |
Maybe should scan update/addition/removal in diff to get parent dir, like { |
|
Thanks for working on this! It's a really important feature. I hope to get some time by the end of this week or early next week to look at this. |
src/filetree.rs
Outdated
| // waiting for writeback to kick in. | ||
| if !opts.skip_sync { | ||
| syncfs(destdir)?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
I think what would really help build confidence here is a few new unit tests. A good way to implement this would be by e.g. checking the ctime (creation time) on files. If we did any extra copying we shouldn't have, then the ctime on the files will have changed when it shouldn't.
There was a problem hiding this comment.
Sorry that I changed the logic, use the first sub dir, if change like fedora/subdir/foo, will copy to feodra.tmp/, this might also copy other files that should not update. Not sure if the current change is OK.
HuijingHei
force-pushed
the
esp_temp
branch
4 times, most recently
from
d73ed88
to
39cd6aa
Compare
If we have
If we have a file not in a directory in EFI, then we can copy it to |
|
The goal is to make the operation atomic at the |
|
(I've not looked at the code again yet) |
| .args(["-a"]) | ||
| .arg(src) | ||
| .arg(dst) | ||
| .pre_exec(move || rustix::process::fchdir(rootfd).map_err(Into::into)) |
There was a problem hiding this comment.
This can only copy from src to dst at same rootfd, it will not work if src or dst is not under the same rootfd, maybe better to use cap-std in #674
HuijingHei
force-pushed
the
esp_temp
branch
3 times, most recently
from
e663d4d
to
7413a99
Compare
src/filetree.rs
Outdated
| /// "foo" -> ("foo", "foo.tmp") | ||
| #[cfg(any(target_arch = "x86_64", target_arch = "aarch64"))] | ||
| fn get_subdir(path: &Utf8Path) -> Result<(String, String)> { | ||
| let buf = if let Some(p) = path.parent().filter(|&v| !v.as_os_str().is_empty()) { |
There was a problem hiding this comment.
Maybe https://docs.rs/camino/latest/camino/struct.Utf8Path.html#method.components would make it easier than iterating over the parents.
There was a problem hiding this comment.
Agree with you, no need to iter parent, just iter path and get iter.next() is enough.
If using components, need to import module camino::Utf8Component, but I think iter the path is enough. WDYT?
src/filetree.rs
Outdated
| .local_rename(&pathtmp, path) | ||
| .with_context(|| format!("renaming {path}"))?; | ||
|
|
||
| // Write new files to temp dir if exists, else write directly in dest |
There was a problem hiding this comment.
Theoretically, a binary could change behavior if another file is there or not.
So maybe let's move that after the changed files loop to make it safer. Or we add only to a temp dir copy in all cases.
There was a problem hiding this comment.
So maybe let's move that after the changed files loop to make it safer. Or we add only to a temp dir copy in all cases.
Move additions after changed seems easier.
If move to temp dir, it might be different as there are no original copy and finally will use rename() instead of exchange(). But we can change to temp if it is necessary. WDYT?
See Timothée's comment coreos#454 (comment) logic is like this: - `cp -a fedora fedora.tmp` - We start with a copy to make sure to keep all other files that we do not explicitly track in bootupd - Update the content of `fedora.tmp` with the new binaries - Exchange `fedora.tmp` -> `fedora` - Remove now "old" `fedora.tmp` If we have a file not in a directory in `EFI`, then we can copy it to `foo.tmp` and then act on it and finally rename it. No need to copy the entire `EFI`. Fixes coreos#454
See Timothée's comment #454 (comment)
logic is like this:
cp -a fedora fedora.tmpthat we do not explicitly track in bootupd
fedora.tmpwith the new binariesfedora.tmp->fedorafedora.tmpIf we have a file not in a directory in
EFI, then we can copyit to
foo.tmpand then act on it and finally rename it. Noneed to copy the entire
EFI.Fixes #454
Additional info for the logic:
in temp dir; if have a file not in a directory, remove it directly
directly in dest; if have a file not in a directory, add it directly
files to temp dir; if have a file not in a directory, copy to
foo.tmp