Use rename of directories instead of symbolic links in boot partition. #1967

valentindavid · 2019-11-03T12:21:22Z

This implements @AdrianVovk idea from #1719 (comment) to solve issue #1719.

This uses renameat2 to do atomic swap of the loader directory in the
boot partition. It fallsback to non-atomic rename. This stays atomic
on filesystems supporting links but also provide a non-atomic behavior
when filesystem does not provide any atomic alternative.

This is working with SystemD boot on EFI using boot loader
specifications.

There is still the issue of losing /loader/loader.conf with SystemD
boot. Maybe we should think about copying other files from previous loader directories.

openshift-ci-robot · 2019-11-03T12:21:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: valentindavid
To complete the pull request process, please assign cgwalters
You can assign the PR to them by writing /assign @cgwalters in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2019-11-03T12:21:37Z

Hi @valentindavid. Thanks for your PR.

I'm waiting for a ostreedev 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.

rh-atomic-bot · 2019-11-03T12:22:28Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

AdrianVovk · 2019-11-03T16:25:39Z

The challenge with this is that the ESP is vfat, and therefore rename will always be non-atomic. It's still better than what I'm doing now (manually copying files into /efi)

#1951
^ here's a proposed solution that I think could solve this same problem without needing the renameat2 call.

rh-atomic-bot · 2019-12-11T20:35:33Z

☔ The latest upstream changes (presumably #1767) made this pull request unmergeable. Please resolve the merge conflicts.

Systemd boot requires the boot partition to be the EFI partition which means FAT is required. OSTree uses symlinking as a way to do atomic update. There is no solution yet for atomic update of FAT partitions. THis patch however changes symlinking by doing directory move which can be atomic in conditions. In practice filesystems with symbolic link support usually also support atomic rename of directories. But this allows to also work when no atomic update is working. Patch was submitted upstream as ostreedev/ostree#1967

bam80 · 2020-02-21T21:36:45Z

Is this PR still proposed or abandoned?

valentindavid · 2020-02-21T22:26:20Z

Is this PR still proposed or abandoned?

I can rebase and take care of it a bit. But since there was no interaction from maintainers, then I am not sure whether they are considering it.

bam80 · 2020-03-23T22:45:22Z

@valentindavid have you been able to pay some attention to this issue again? The changes are highly anticipated here, in one form or another. Thanks.

jjardon · 2020-04-07T10:48:10Z

@mwleeds @rfairley any chance to take a look to this? We currently need this to make GNOME images work, see https://gitlab.gnome.org/GNOME/gnome-build-meta/-/commit/79fb62e0d243a21ab58dc1dda439c23db5d474ab

jjardon · 2020-05-30T10:04:29Z

@cgwalters Hey! Any chance to take a look to this?

bam80 · 2020-08-06T21:15:58Z

Friendly ping.

AdrianVovk · 2020-09-27T06:30:31Z

@valentindavid This PR conflicts with upstream again. GNOME OS uses an ancient version of OSTree so they have yet to run into this issue, but on my OS I just updated to the latest version and I'm about to be downgrading again because this patch is broken.

@cgwalters Is there any chance for this patch to be reviewed or considered at all? Without this patch, I have to follow every single ostree admin deploy anywhere in the process of building, generating ISOs, installing the OS, and updating the OS with a bunch of ugly commands that mess with the contents of the ESP. With this patch, it just works. OSTree operates on BLS files, but has yet to support the bootloader that was actually built for the standard 🤷

damianatorrpm · 2020-10-24T22:55:11Z

updated.txt
Rebased patch file.

kowalski7cc · 2020-11-25T23:18:39Z

Any news on this?

jjardon · 2021-09-08T23:35:19Z

In case is useful, this patch has been updated in GNOME OS to work with libostree-2021.2: https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/41.rc/files/ostree/no-boot-symlink.patch

AdrianVovk · 2021-09-09T00:42:07Z

carbonOS has been using this patch for a while. It works flawlessly so far! Hopefully this will get merged at some point...

@jjardon that same patch works for 2021.3 also

cgwalters · 2021-11-10T18:53:03Z

Hi, sorry about the delay.

I am OK in principle with the idea and code. But I'd like to have some design to close the atomicity hole longer term.

If you (or someone else) doesn't mind rebasing we can try to get this merged.

dbnicholson

I didn't finish thinking through all of this, but I wouldn't want to merge this without a lot more comments and tests. Is the symlink fallback path tested at all?

dbnicholson · 2021-11-10T21:39:07Z

src/libostree/ostree-sysroot-deploy.c

+                    GError    **error)
+{
+  if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_EXCHANGE) == 0)
+    return TRUE;


You're not setting the is_atomic out parameter here or in the other renameat2 success case below.

We start within ostree_sysroot_write_deployments_with_options with:

bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader);

Then we only mark bootloader_is_atomic as FALSE if we do not manage to do a renameat2. But if we manage, we want to still keep FALSE if that was the value given by _ostree_bootloader_is_atomic.

OK, I missed that part.

dbnicholson · 2021-11-10T21:41:13Z

src/libostree/ostree-sysroot-deploy.c

-/* We generate the symlink on disk, then potentially do a syncfs() to ensure
- * that it (and everything else we wrote) has hit disk. Only after that do we
- * rename it into place.
- */


Please don't remove comments like this. This is some of the most sensitive code in ostree. If anything, it should have a lot more comments and I don't see any in the code you've added.

dbnicholson · 2021-11-10T21:43:51Z

src/libostree/ostree-sysroot-deploy.c

+  if (errno != ENOENT)
+    return glnx_throw_errno_prefix (error, "renameat2");
+
+  if (renameat2(olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE) == 0)


Nitpicky, but space before parentheses here and in a few other spots.

dbnicholson · 2021-11-10T22:16:45Z

src/libostree/ostree-sysroot-cleanup.c

+      g_string_truncate (buf, 0); g_string_append_printf (buf, "boot/loader.%d", self->bootversion);
+      if (!glnx_shutil_rm_rf_at (self->sysroot_fd, buf->str, cancellable, error))
+        return FALSE;
+    }


Is this logic even right? In swap_bootloader, loader.<newversion> is exchanged with loader. That means that the current bootloader directory is now named loader.<newversion>. What's even at loader.<currentversion>? This is also in conflict with the code below that cleans up loader.<currentversion>. So, after this change, there's no more loader.* bootloader directories left. Is that correct?

Right. Only loader directory is left after deployment is finished successfully. We could just use loader.tmp and loader. But if I remember correctly, we need to support older deployments that are still using symlinks. For that reason we still to need to keep this 0/1 version names. Also maintaining a patch for long term I cannot really afford changing too much the logic.

Reading the code, I do not think the support for older deployment is working. I will try to add tests for that.

This uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. This is working with SystemD boot on EFI using boot loader specifications. There is still the issue of losing `/loader/loader.conf` with SystemD boot.

dbnicholson · 2021-11-18T18:35:45Z

I'm still not totally sure what the best approach here is, but I think the fundamental change from replacing loader to exchanging loader is likely to have subtle effects that need more detailed analysis.

Since the replacing process needs to be supported in case RENAME_EXCHANGE is not available or there's no existing loader, I think the best approach would be to make it always work as if it's replacing. An approach I think would work is:

Use the existing symlink replacement, which involves creating a temporary loader.tmp symlink and the renaming it to loader. This is the simple tried and true approach.
If symlinking fails because of EPERM, then recursively copy the new loader directory to loader.tmp and then exchange it with loader using RENAME_EXCHANGE + the renaming fallbacks you already coded. When exchanging has occurred, recursively delete the exchanged loader.tmp directory.

Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Signed-off-by: Ricardo Salveti <ricardo@foundries.io>

martinezjavier · 2022-06-03T16:08:04Z

Since the replacing process needs to be supported in case RENAME_EXCHANGE is not available

@dbnicholson FYI, I worked on a patch series to add RENAME_EXCHANGE support to vfat, latest revision is: https://lkml.org/lkml/2022/6/1/1049

dbnicholson · 2022-06-03T21:28:21Z

Since the replacing process needs to be supported in case RENAME_EXCHANGE is not available

@dbnicholson FYI, I worked on a patch series to add RENAME_EXCHANGE support to vfat, latest revision is: https://lkml.org/lkml/2022/6/1/1049

That's great. Of course, you'd still need to support the fallback since it will be some time until that's commonly available (presuming it gets merged).

I've come around a bit on this idea in the sense that I think it's reasonable to assume RENAME_EXCHANGE as the primary mechanism since there will always be filesystems like vfat that can't do symlinks but presumably they can all gain support for RENAME_EXCHANGE. However, I still feel like any major changes in this area require exhaustive testing.

ricardosalveti · 2022-06-03T21:32:59Z

I'm currently playing with ricardosalveti@731b73f, which is based on the original patch from this PR, which keeps the current behavior for new systems and systems that are already deployed, but also supports loader as a directory (relying on RENAME_EXCHANGE).

That way I can create new deployments (using systemd-boot) with the extra kernel patches without breaking my current deployments that are ok with links (using grub / u-boot).

Still validating with my yocto integration, can post the final set after I'm done testing it.

martinezjavier · 2022-06-14T12:38:02Z

FYI the patch-set got merged by a subsystem maintainer and are heading towards the Linux v6.0 release.

Allow manipulating and updating /boot/loader entries under a normal directory, as well as using symbolic links. For directories this uses `renameat2` to do atomic swap of the loader directory in the boot partition. It fallsback to non-atomic rename. This stays atomic on filesystems supporting links but also provide a non-atomic behavior when filesystem does not provide any atomic alternative. /boot/loader as a normal directory is needed by systemd-boot support, and can be stored under the EFI ESP vfat partition. Based on the original implementation done by Valentin David at ostreedev#1967. Tests were duplicated for simplicity reasons. Upstream-Status: Pending Signed-off-by: Ricardo Salveti <ricardo@foundries.io> %% original patch: 0003-Add-support-for-directories-instead-of-symbolic-link.patch

GrabbenD · 2023-08-29T14:35:48Z

Is GNOME OS's patch still the best workaround until this is finished?
https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/45.beta/files/ostree/no-boot-symlink.patch

Edit: looks like it doesn't work with 2023.6 update

GrabbenD · 2023-09-14T15:36:08Z

I ended up working around this issue by making / a ext4 partition and then I mounted ESP on /boot/efi which makes /boot ext4 thus allowing symbolic links

Here's the commands if anyone else is stuck on Preparing final bootloader swap: symlinkat: Operation not permitted error:
M1cha/archlinux-ostree#1 (comment)

bam80 · 2024-03-12T01:22:07Z

update?

dbnicholson · 2024-03-12T14:57:05Z

I've thought about this several times as I would really like to have this in Endless to support our systemd-boot systems. What always makes me anxious is trying to figure out how to handle compatibility with the vast majority of our systems that have symlink based deployments. There are 2 main issues I'm concerned with:

The semantics of the deployment are different if loader is a symlink to a loader.N directory versus loader itself being a directory. What do the swapped loader.N directories mean in pure directory mode? I think the answer is that they don't really match the semantics of the symlink deployment. It's essentially a different algorithm even though it's seemingly only a small change from the symlink approach.
What do you do with systems that already have symlink based deployments? Potentially you could write a migration to the directory scheme, but that means you can't roll back to a system with a libostree that doesn't understand the directory scheme.

So, to me this requires a couple additional pieces of implementation and policy.

The directory deployment scheme is treated separately from the symlink deployment scheme. Your system can be in one scheme or the other, but the semantics are different and shouldn't be mixed.
If your system is already using a symlink based deployment, it should stay that way. You can opt in to a migration to the directory scheme, but this means you may potentially lose roll back targets. At Endless we'd do that at some major version check point and probably have to delete some old deployments to ensure users didn't try to roll back to a system where they'd be screwed. Or maybe we'd never migrate anyone because the downsides are too great.
If you're on a new deployment (i.e., no existing /boot/loader or /boot/ostree), then the directory scheme is preferred.

openshift-ci-robot requested review from mwleeds and rfairley November 3, 2019 12:21

openshift-ci-robot added the size/L label Nov 3, 2019

openshift-ci-robot added the needs-ok-to-test label Nov 3, 2019

valentindavid force-pushed the support-vfat branch from c3d34fb to c36ee32 Compare November 3, 2019 15:13

openshift-ci-robot added the needs-rebase label Dec 11, 2019

valentindavid force-pushed the support-vfat branch from c36ee32 to 861306d Compare April 13, 2020 13:53

openshift-ci-robot removed the needs-rebase label Apr 13, 2020

cgwalters mentioned this pull request May 12, 2020

Lenovo T470s - "Secure boot is enabled, but shim isn't installed to the EFI system partition" fwupd/fwupd#2084

Closed

damianatorrpm mentioned this pull request Oct 23, 2020

[option] to skip ostree-finalize-stage coreos/rpm-ostree#2281

Open

dbnicholson requested changes Nov 10, 2021

View reviewed changes

valentindavid force-pushed the support-vfat branch from 861306d to 5825205 Compare November 15, 2021 17:58

openshift-ci bot removed the needs-rebase label Nov 15, 2021

dbnicholson mentioned this pull request Feb 4, 2022

Rebase on 2022.1 endlessm/ostree#186

Merged

22 tasks

martinezjavier mentioned this pull request Sep 1, 2022

Add sysroot.bootprefix option #2705

Merged

gdonval mentioned this pull request Oct 3, 2022

systemd-boot support #1719

Open

martinezjavier mentioned this pull request Oct 25, 2022

Provide a command line option to install systemd-boot rather than grub2 on x86_64 and arm64 rhinstaller/anaconda#4368

Merged

cgwalters removed the size/L label May 2, 2023

This was referenced Aug 29, 2023

Manual install without Fedora Kinoite M1cha/archlinux-ostree#1

Closed

Preparing final bootloader swap: symlinkat: Operation not permitted #2478

Open

AdrianVovk mentioned this pull request Feb 2, 2024

[RFE] Allow symlinked "/loader" directory in sd-boot systemd/systemd#31179

Closed

Use rename of directories instead of symbolic links in boot partition. #1967

Are you sure you want to change the base?

Use rename of directories instead of symbolic links in boot partition. #1967

Conversation

valentindavid commented Nov 3, 2019

openshift-ci-robot commented Nov 3, 2019

openshift-ci-robot commented Nov 3, 2019

rh-atomic-bot commented Nov 3, 2019

AdrianVovk commented Nov 3, 2019

rh-atomic-bot commented Dec 11, 2019

bam80 commented Feb 21, 2020

valentindavid commented Feb 21, 2020

bam80 commented Mar 23, 2020

jjardon commented Apr 7, 2020

jjardon commented May 30, 2020

bam80 commented Aug 6, 2020

AdrianVovk commented Sep 27, 2020

damianatorrpm commented Oct 24, 2020

kowalski7cc commented Nov 25, 2020

jjardon commented Sep 8, 2021

AdrianVovk commented Sep 9, 2021

cgwalters commented Nov 10, 2021

dbnicholson left a 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

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

dbnicholson commented Nov 18, 2021

martinezjavier commented Jun 3, 2022

dbnicholson commented Jun 3, 2022

ricardosalveti commented Jun 3, 2022 • edited Loading

martinezjavier commented Jun 14, 2022 • edited Loading

GrabbenD commented Aug 29, 2023 • edited Loading

GrabbenD commented Sep 14, 2023 • edited Loading

bam80 commented Mar 12, 2024

dbnicholson commented Mar 12, 2024

ricardosalveti commented Jun 3, 2022 •

edited

Loading

martinezjavier commented Jun 14, 2022 •

edited

Loading

GrabbenD commented Aug 29, 2023 •

edited

Loading

GrabbenD commented Sep 14, 2023 •

edited

Loading