efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE #669

HuijingHei · 2024-06-19T12:19:36Z

See Timothée's comment #454 (comment)
Reuse TMP_PREFIX, logic is like this:

cp -a fedora .btmp.fedora
- 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 .btmp.fedora with the new binaries
Exchange .btmp.fedora -> fedora
Remove now "old" .btmp.fedora

If we have a file not in a directory in EFI, then we can copy
it to .btmp.foo and then act on it and finally rename it. No
need to copy the entire EFI.

Additional info for the logic:

for removals, copy to temp dir, remove file in temp dir; if
have a file not in a directory, remove it directly
for changes/additions, copy to temp dir, write files in temp
dir; if have a file not in a directory, copy as temp file
Do local exchange/rename and remove temp dir/file

And use insert() instead of push() to match starts_with()
when scanning temp files & dirs.

Fixes #454

filetree: add failpoint when doing exchange
Inspired by #669 (comment)

cgwalters

Thanks for working on this!

src/efi.rs

cgwalters · 2024-06-19T23:54:06Z

src/efi.rs

+    fn esp_path_tmp(&self) -> Result<PathBuf> {
+        self.ensure_mounted_esp(Path::new("/"))
+            .map(|v| v.join(".EFI.tmp"))
+    }


I think it'd be clearer to have just const TEMP_EFI_DIR: &str = ".EFI.tmp"; or so and use it where needed?

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.

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?

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 were some elements in https://lists.fedoraproject.org/archives/list/[email protected]/thread/VRJ7CRQUZ7SNEDW4AIOHKKJ7TMPTOX5A/

See #669 (comment), will 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.

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.

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.

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.

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?

src/efi.rs

travier · 2024-06-20T08:55:29Z

[2024-06-20T08:07:06.180Z] # Previous BIOS: grub2-tools-1:2.06-89.fc38.x86_64
[2024-06-20T08:07:06.180Z] # Updated BIOS: grub2-tools-1:2.06-102.fc38.x86_64
[2024-06-20T08:07:06.180Z] # error: Failed to update EFI: No such file or directory (os error 2)
[2024-06-20T08:07:06.180Z] + fatal 'test failed'
[2024-06-20T08:07:06.180Z] + echo error: test failed
[2024-06-20T08:07:06.180Z] error: test failed

🤔

src/efi.rs

HuijingHei · 2024-06-20T12:56:50Z

[2024-06-20T12:39:28.464Z] # Previous BIOS: grub2-tools-1:2.06-89.fc38.x86_64

[2024-06-20T12:39:28.464Z] # Updated BIOS: grub2-tools-1:2.06-102.fc38.x86_64

[2024-06-20T12:39:28.464Z] # Previous EFI: grub2-efi-x64-1:2.06-89.fc38.x86_64,shim-x64-15.8-3.x86_64

[2024-06-20T12:39:28.464Z] # Updated EFI: grub2-efi-x64-1:2.06-102.fc38.x86_64,shim-x64-15.8-3.x86_64,test-bootupd-payload-1.0-1.x86_64

[2024-06-20T12:39:28.464Z] # grep: /tmp/tmp.hin3bC02Ci/EFI/fedora/test-bootupd.efi: No such file or directory

[2024-06-20T12:39:28.464Z] # ls: cannot access '/tmp/tmp.hin3bC02Ci/EFI/fedora/test-bootupd.efi': No such file or directory

travier · 2024-06-21T08:39:55Z

We've merged the CI fixes. Can you rebase this one? Thanks

HuijingHei · 2024-06-21T08:51:03Z

We've merged the CI fixes. Can you rebase this one? Thanks

Sure, updated, thanks!

HuijingHei · 2024-06-21T10:05:24Z

Updating shim-x64 15.8-2 -> 15.8-3, it will also update BOOT/fbx64.efi

[root@cosa-devsh ~]# bootupctl update -vvv
Previous EFI: grub2-efi-x64-1:2.06-103.fc40.x86_64,shim-x64-15.8-2.x86_64
Updated EFI: grub2-efi-x64-1:2.06-123.fc40.x86_64,shim-x64-15.8-3.x86_64

[root@cosa-devsh ~]# mount /dev/vda2 /boot/efi/
[root@cosa-devsh ~]# ls /boot/efi/EFI/BOOT/ -l
total 1014
-rwxr-xr-x. 1 root root 949424 Jan  1  1980 BOOTX64.EFI
-rwxr-xr-x. 1 root root  87816 Jun 21 10:01 fbx64.efi

HuijingHei · 2024-06-21T10:19:14Z

If using .fedora.tmp will cause CI failed grep: /tmp/tmp.hin3bC02Ci/EFI/fedora/test-bootupd.efi: No such file or directory, guess it is because filetree is based on "BOOT/BOOTX64.EFI" or "fedora/BOOTX64.CSV", so the apply_diff will also create the dir like BOOT and fedora under temp dir, which makes it actually /tmp/tmp.hin3bC02Ci/EFI/fedora/fedora/test-bootupd.efi.

travier · 2024-06-21T11:19:04Z

The problem with copying the entire EFI directory is that we don't know what's there, and there might a lot of other vendor directories (windows, ubuntu, etc.) that we don't really want to copy as that might take more space than what we have in the EFI partition.

Inspired by coreos#669 (comment)

cgwalters · 2024-08-02T21:19:42Z

Looks like this needs a rebase now btw

cgwalters · 2024-08-02T21:23:21Z

Anything else that I should do before merged? CC @cgwalters @travier , thanks!

I should emphasize again: thanks for working on this!

My feeling on this though is still very mixed not because of any flaw in your code, but because the structure of shim means we will get splits: #669 (comment)

There's also the fact that (again no fault of yours) this is just an inherently subtle problem domain, and while you did add a lot of new unit tests, OTOH, if we mess this up we break people's bootloaders.

cgwalters

Nevertheless, while I haven't yet truly audited the code, it looks overall sane to me and the newly added tests help, so if you feel confident I am OK moving ahead. Adding my approval.

See Timothée's comment coreos#454 (comment) Reuse `TMP_PREFIX`, logic is like this: - `cp -a fedora .btmp.fedora` - 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 `.btmp.fedora` with the new binaries - Exchange `.btmp.fedora` -> `fedora` - Remove now "old" `.btmp.fedora` If we have a file not in a directory in `EFI`, then we can copy it to `.btmp.foo` and then act on it and finally rename it. No need to copy the entire `EFI`. And use `insert()` instead of `push()` to match `starts_with()` when scanning temp files & dirs. Fixes coreos#454

Inspired by coreos#669 (comment)

HuijingHei · 2024-08-06T05:54:42Z

Thanks @cgwalters and @travier for the grate help and a lot of instructions.
I am OK to merge this now to trigger more testing from bootc and find regressions earlier, WDYT? @travier

travier · 2024-08-22T13:34:34Z

Thanks for the work here Huijing. Let's get this into Rawhide and test it there on full systems.

HuijingHei · 2024-08-26T06:09:13Z

Much appreciated for the help from @travier and @cgwalters !

HuijingHei changed the title ~~efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE~~ WIP: efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE Jun 19, 2024

openshift-ci bot added the do-not-merge/work-in-progress label Jun 19, 2024

HuijingHei force-pushed the esp_temp branch 2 times, most recently from 3a7cd83 to f3bb105 Compare June 19, 2024 14:25

cgwalters reviewed Jun 20, 2024

View reviewed changes

HuijingHei force-pushed the esp_temp branch 2 times, most recently from b635467 to e91cab0 Compare June 20, 2024 07:37

travier reviewed Jun 20, 2024

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

travier reviewed Jun 20, 2024

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

HuijingHei force-pushed the esp_temp branch 2 times, most recently from 366b8eb to a6c8400 Compare June 20, 2024 10:43

HuijingHei commented Jun 20, 2024

View reviewed changes

src/efi.rs Outdated Show resolved Hide resolved

HuijingHei force-pushed the esp_temp branch 2 times, most recently from d033a4e to ad56520 Compare June 20, 2024 12:10

travier mentioned this pull request Jun 20, 2024

Port to rustix, drop nix #670

Merged

HuijingHei force-pushed the esp_temp branch from ad56520 to 6bf2e6f Compare June 20, 2024 13:38

HuijingHei force-pushed the esp_temp branch from 6bf2e6f to b8fd9a9 Compare June 21, 2024 08:50

HuijingHei force-pushed the esp_temp branch 3 times, most recently from 79cb3ad to 3cf8636 Compare June 21, 2024 09:06

HuijingHei force-pushed the esp_temp branch from 3cf8636 to 5891859 Compare June 21, 2024 10:14

HuijingHei removed the do-not-merge/work-in-progress label Jun 21, 2024

HuijingHei changed the title ~~WIP: efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE~~ efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE Jun 21, 2024

HuijingHei force-pushed the esp_temp branch from 43f359a to 0a1ff3b Compare July 17, 2024 14:52

openshift-merge-robot added the needs-rebase label Jul 17, 2024

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Jul 17, 2024

filetree: add failpoint when doing exchange

16cccf1

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from 0a1ff3b to 16cccf1 Compare July 17, 2024 15:02

openshift-merge-robot removed the needs-rebase label Jul 17, 2024

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Jul 17, 2024

filetree: add failpoint when doing exchange

f48da07

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from 16cccf1 to f48da07 Compare July 17, 2024 15:04

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Jul 17, 2024

filetree: add failpoint when doing exchange

0aebd5e

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from f48da07 to 0aebd5e Compare July 17, 2024 15:07

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Jul 17, 2024

filetree: add failpoint when doing exchange

2c05c92

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from 0aebd5e to 2c05c92 Compare July 17, 2024 15:12

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Jul 18, 2024

filetree: add failpoint when doing exchange

0e47c4d

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from 2c05c92 to 0e47c4d Compare July 18, 2024 01:12

HuijingHei removed the do-not-merge/hold label Jul 18, 2024

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Aug 1, 2024

filetree: add failpoint when doing exchange

f6c3906

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from 0e47c4d to f6c3906 Compare August 1, 2024 14:46

cgwalters approved these changes Aug 2, 2024

View reviewed changes

HuijingHei added 2 commits August 5, 2024 09:37

filetree: add failpoint when doing exchange

e08bc29

Inspired by coreos#669 (comment)

HuijingHei force-pushed the esp_temp branch from f6c3906 to e08bc29 Compare August 5, 2024 01:47

cgwalters merged commit e65a074 into coreos:main Aug 21, 2024
12 checks passed

HuijingHei deleted the esp_temp branch August 22, 2024 01:17

HuijingHei mentioned this pull request Aug 27, 2024

New bootupd release #712

Closed

cgwalters mentioned this pull request Sep 3, 2024

packaging: add a systemd unit to run every boot #716

Closed

cgwalters mentioned this pull request Nov 5, 2024

Update the ESP by creating a tmpdir and RENAME_EXCHANGE #454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE #669

efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE #669

HuijingHei commented Jun 19, 2024 •

edited

Loading

cgwalters left a comment

cgwalters Jun 19, 2024

cgwalters Jun 20, 2024

HuijingHei Jun 20, 2024 •

edited

Loading

travier Jun 20, 2024

travier Jun 20, 2024

HuijingHei Jun 21, 2024

cgwalters Jun 28, 2024

HuijingHei Jul 1, 2024 •

edited

Loading

cgwalters Jul 1, 2024

HuijingHei Jul 2, 2024

travier commented Jun 20, 2024 •

edited

Loading

HuijingHei commented Jun 20, 2024

travier commented Jun 21, 2024

HuijingHei commented Jun 21, 2024

HuijingHei commented Jun 21, 2024

HuijingHei commented Jun 21, 2024

travier commented Jun 21, 2024 •

edited

Loading

cgwalters commented Aug 2, 2024

cgwalters commented Aug 2, 2024

cgwalters left a comment

HuijingHei commented Aug 6, 2024

travier commented Aug 22, 2024

HuijingHei commented Aug 26, 2024

efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE #669

efi: update the ESP by creating a tmpdir and RENAME_EXCHANGE #669

Conversation

HuijingHei commented Jun 19, 2024 • edited Loading

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuijingHei Jun 20, 2024 • 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

Choose a reason for hiding this comment

HuijingHei Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travier commented Jun 20, 2024 • edited Loading

HuijingHei commented Jun 20, 2024

travier commented Jun 21, 2024

HuijingHei commented Jun 21, 2024

HuijingHei commented Jun 21, 2024

HuijingHei commented Jun 21, 2024

travier commented Jun 21, 2024 • edited Loading

cgwalters commented Aug 2, 2024

cgwalters commented Aug 2, 2024

cgwalters left a comment

Choose a reason for hiding this comment

HuijingHei commented Aug 6, 2024

travier commented Aug 22, 2024

HuijingHei commented Aug 26, 2024

HuijingHei commented Jun 19, 2024 •

edited

Loading

HuijingHei Jun 20, 2024 •

edited

Loading

HuijingHei Jul 1, 2024 •

edited

Loading

travier commented Jun 20, 2024 •

edited

Loading

travier commented Jun 21, 2024 •

edited

Loading