Skip to content

sysroot: Support for directories instead of symbolic links in boot part #3359

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

sysroot: Support for directories instead of symbolic links in boot part #3359

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f937458
sysroot: Support for directories instead of symbolic links in boot part
ricardosalveti Jun 1, 2022
a2a67c8
tests: directories instead of symbolic links in boot part
igoropaniuk Jan 21, 2025
d950b3e
sysroot: Support for standard-conformance marker file
igoropaniuk Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Makefile-tests.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ _installed_or_uninstalled_test_scripts = \
tests/test-admin-upgrade-endoflife.sh \
tests/test-admin-upgrade-systemd-update.sh \
tests/test-admin-deploy-syslinux.sh \
tests/test-admin-dir-deploy-syslinux.sh \
tests/test-admin-deploy-bootprefix.sh \
tests/test-admin-deploy-composefs.sh \
tests/test-admin-deploy-var.sh \
Expand All @@ -112,9 +113,12 @@ _installed_or_uninstalled_test_scripts = \
tests/test-admin-deploy-switch.sh \
tests/test-admin-deploy-etcmerge-cornercases.sh \
tests/test-admin-deploy-uboot.sh \
tests/test-admin-dir-deploy-uboot.sh \
tests/test-admin-deploy-grub2.sh \
tests/test-admin-dir-deploy-grub2.sh \
tests/test-admin-deploy-nomerge.sh \
tests/test-admin-deploy-none.sh \
tests/test-admin-dir-deploy-none.sh \
tests/test-admin-deploy-bootid-gc.sh \
tests/test-admin-deploy-whiteouts.sh \
tests/test-admin-deploy-emptyetc.sh \
Expand Down
179 changes: 165 additions & 14 deletions src/libostree/ostree-sysroot-deploy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,21 @@ sysroot_flags_to_copy_flags (GLnxFileCopyFlags defaults, OstreeSysrootDebugFlags
return defaults;
}

static gboolean
create_bootself_link (OstreeSysroot *self, GCancellable *cancellable, GError **error)
{
/* When there is no loader, check if the fs supports symlink or not */
if (TEMP_FAILURE_RETRY (symlinkat (".", self->sysroot_fd, "boot/boot")) < 0)
{
if (errno == EPERM)
return FALSE;
Copy link
Member

Choose a reason for hiding this comment

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

Again need to adhere to GError conventions; this case returns FALSE but leaves error unset. Honestly I think it's simplest to have this helper function not use gerror i.e.

// Create the symlink boot/boot. Sets `errno` on failure.
static int
create_bootself_link (OstreeSysroot *self) 
{
  return TEMP_FAILURE_RETRY (symlinkat (".", self->sysroot_fd, "boot/boot");
}

Then the callers can just directly check errno as they see fit?

else if (errno != EEXIST)
return glnx_throw_errno_prefix (error, "symlinkat");
}

return TRUE;
}

/* Try a hardlink if we can, otherwise fall back to copying. Used
* right now for kernels/initramfs/device trees in /boot, where we can just
* hardlink if we're on the same partition.
Expand Down Expand Up @@ -2176,6 +2191,53 @@ install_deployment_kernel (OstreeSysroot *sysroot, int new_bootversion,
return TRUE;
}

/* Determine whether an existing directory implements the semantics described in
* https://uapi-group.org/specifications/specs/boot_loader_specification/#type-1-boot-loader-entry-keys
*/
static gboolean
is_bootconfig_type1_semantics (OstreeSysroot *sysroot, GCancellable *cancellable, GError **error)
{
struct stat stbuf;

if (!_ostree_sysroot_ensure_boot_fd (sysroot, error))
return FALSE;

if (!glnx_fstatat_allow_noent (sysroot->boot_fd, "loader/entries.srel", &stbuf,
Copy link
Collaborator

@champtar champtar Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We need to create / write entries.srel each time we create a new loader dir ? Or we already copy the content of the current directory first ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

systemd-boot also reads loader/loader.conf and loader/entries/*.conf https://www.freedesktop.org/software/systemd/man/latest/loader.conf.html
either we need to just create loader/entries.tmp then rename to loader/entries.tmp (so we keep the content of loader at least), or we need to copy the full loader dir and just cleanup loader/entries/ostree-*.conf only to keep everything else

AT_SYMLINK_NOFOLLOW, error))
return FALSE;

if (errno == ENOENT)
{
g_debug ("Didn't find loader/entries.srel file");
return FALSE;
}
else
{
/* Get semantics type by reading loader/entries.srel */
gsize len;
g_autofree char *type = glnx_file_get_contents_utf8_at (
sysroot->boot_fd, "loader/entries.srel", &len, cancellable, error);
if (type == NULL)
{
g_debug ("Invalid loader/entries.srel file");
return FALSE;
}

if (g_str_has_prefix (type, "type1"))
{
g_debug ("type1 semantics is found in loader/entries.srel file");
return TRUE;
}
else
{
g_debug ("Unsupported semantics type ('%s') in loader/entries.srel file", type);
return FALSE;
}
}

return FALSE;
}

/* 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.
Expand All @@ -2190,9 +2252,7 @@ prepare_new_bootloader_link (OstreeSysroot *sysroot, int current_bootversion, in

/* This allows us to support both /boot on a seperate filesystem to / as well
* as on the same filesystem. */
if (TEMP_FAILURE_RETRY (symlinkat (".", sysroot->sysroot_fd, "boot/boot")) < 0)
if (errno != EEXIST)
return glnx_throw_errno_prefix (error, "symlinkat");
(void)create_bootself_link (sysroot, cancellable, error);
Copy link
Member

Choose a reason for hiding this comment

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

See above; let's keep checking errors here, with my above suggestion this just changes to

  if (create_bootself_link (sysroot) < 0 && errno != EEXIST)
    return glnx_throw_errno_prefix (error, "Creating bootself link);


g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion);

Expand All @@ -2204,10 +2264,57 @@ prepare_new_bootloader_link (OstreeSysroot *sysroot, int current_bootversion, in
return TRUE;
}

/* We generate the directory 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 (via renameat2 RENAME_EXCHANGE).
*/
static gboolean
prepare_new_bootloader_dir (OstreeSysroot *sysroot, int current_bootversion, int new_bootversion,
Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about this whole thing more and it's not clear to me we need the concept of a "bootversion" at all in the case where we're doing a directory swap, do we?

We could easily have

  • boot/loader == current version
  • boot/loader.tmp == staging/temp dir

That would have been harder to do with the symlink logic but I think it'd be quite a simplification to the current code here. Especially then we can stop writing the special ostree_bootversion file into the loader directory right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would have been harder to do with the symlink logic

The directory name could have been random, or a hash of the content, and for the cleanup you just need to readlink before you rename, but we can't change now

I think it'd be quite a simplification to the current code here. Especially then we can stop writing the special ostree_bootversion file into the loader directory right?

Agreed, no need for bootversion when using directory swap

GCancellable *cancellable, GError **error)
{
GLNX_AUTO_PREFIX_ERROR ("Preparing bootloader directory", error);
g_assert ((current_bootversion == 0 && new_bootversion == 1)
|| (current_bootversion == 1 && new_bootversion == 0));

if (!_ostree_sysroot_ensure_boot_fd (sysroot, error))
return FALSE;

/* This allows us to support both /boot on a seperate filesystem to / as well
* as on the same filesystem. Allowed to fail with EPERM on ESP/vfat.
*/
if (TEMP_FAILURE_RETRY (symlinkat (".", sysroot->sysroot_fd, "boot/boot")) < 0)
if (errno != EPERM && errno != EEXIST)
return glnx_throw_errno_prefix (error, "symlinkat");
Comment on lines +2285 to +2287
Copy link
Member

Choose a reason for hiding this comment

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

Then this one is:

  if (create_bootself_link (sysroot) < 0 && !G_IN_SET(errno, EEXIST, EPERM))
    return glnx_throw_errno_prefix (error, "Creating bootself link);


/* As the directory gets swapped with glnx_renameat2_exchange, the new bootloader
* deployment needs to first be moved to the 'old' path, as the 'current' one will
* become the older deployment after the exchange.
*/
g_autofree char *loader_new = g_strdup_printf ("loader.%d", new_bootversion);
g_autofree char *loader_old = g_strdup_printf ("loader.%d", current_bootversion);

/* Tag boot version under an ostree specific file */
g_autofree char *version_name = g_strdup_printf ("%s/ostree_bootversion", loader_new);
if (!glnx_file_replace_contents_at (sysroot->boot_fd, version_name, (guint8 *)loader_new,
strlen (loader_new), 0, cancellable, error))
return FALSE;

/* It is safe to remove older loader version as it wasn't really deployed */
if (!glnx_shutil_rm_rf_at (sysroot->boot_fd, loader_old, cancellable, error))
return FALSE;

/* Rename new deployment to the older path before the exchange */
if (!glnx_renameat2_noreplace (sysroot->boot_fd, loader_new, sysroot->boot_fd, loader_old))
return FALSE;

return TRUE;
}

/* Update the /boot/loader symlink to point to /boot/loader.$new_bootversion */
static gboolean
swap_bootloader (OstreeSysroot *sysroot, OstreeBootloader *bootloader, int current_bootversion,
int new_bootversion, GCancellable *cancellable, GError **error)
swap_bootloader (OstreeSysroot *sysroot, OstreeBootloader *bootloader, gboolean loader_link,
int current_bootversion, int new_bootversion, GCancellable *cancellable,
GError **error)
{
GLNX_AUTO_PREFIX_ERROR ("Final bootloader swap", error);

Expand All @@ -2217,14 +2324,24 @@ swap_bootloader (OstreeSysroot *sysroot, OstreeBootloader *bootloader, int curre
if (!_ostree_sysroot_ensure_boot_fd (sysroot, error))
return FALSE;

g_assert_cmpint (sysroot->boot_fd, !=, -1);
if (loader_link)
{
g_assert_cmpint (sysroot->boot_fd, !=, -1);

/* The symlink was already written, and we used syncfs() to ensure
* its data is in place. Renaming now should give us atomic semantics;
* see https://bugzilla.gnome.org/show_bug.cgi?id=755595
*/
if (!glnx_renameat (sysroot->boot_fd, "loader.tmp", sysroot->boot_fd, "loader", error))
return FALSE;
/* The symlink was already written, and we used syncfs() to ensure
* its data is in place. Renaming now should give us atomic semantics;
* see https://bugzilla.gnome.org/show_bug.cgi?id=755595
*/
if (!glnx_renameat (sysroot->boot_fd, "loader.tmp", sysroot->boot_fd, "loader", error))
return FALSE;
}
else
{
/* New target is currently under the old/current version */
g_autofree char *new_target = g_strdup_printf ("loader.%d", current_bootversion);
if (glnx_renameat2_exchange (sysroot->boot_fd, new_target, sysroot->boot_fd, "loader") != 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

glnx_renameat2_exchange swap the 2 directories, so you need to delete loader.%d after fsfreeze_thaw_cycle
(glnx_renameat only leaves 1 file on success)

return FALSE;
}

/* As grub doesn't support replaying XFS journal,
* we must fsfreeze/thaw again here:
Expand Down Expand Up @@ -2447,13 +2564,47 @@ write_deployments_bootswap (OstreeSysroot *self, GPtrArray *new_deployments,
return glnx_prefix_error (error, "Bootloader write config");
}

if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, cancellable, error))
/* Handle when boot/loader is a symlink (original ostree model) or is a directory (e.g. EFI/vfat)
* for recommended systemd type1 BLS
*/
struct stat stbuf;
gboolean loader_link = FALSE;
gboolean force_type1_semantics = is_bootconfig_type1_semantics (self, cancellable, error);
if (!glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW,
error))
return FALSE;
if (errno == ENOENT)
loader_link = create_bootself_link (self, cancellable, error);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. In an ideal world we wouldn't get here...it's a bit tempting to ask that for the case of a user deploying this new version of ostree targeting an ESP we ask them to pre-create the link.

Or...actually another tempting thing to do is check the filesystem type for boot/loader and unconditionally require this.

Anyways...for now I think what I'd ask is we continue to properly check errors here...

OK I thought more about this and came up with
#3405
to start.

Then a lot of this code can start checking sysroot->boot_is_vfat instead of the return value of symlinkat; WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

#3405 merged! Let me know what you think!

else if (S_ISLNK (stbuf.st_mode))
loader_link = TRUE;
else if (S_ISDIR (stbuf.st_mode))
loader_link = FALSE;
else
return FALSE;

if (force_type1_semantics && loader_link)
return glnx_throw_errno_prefix (error, "type1 semantics, but boot/loader is symlink");
Copy link
Member

Choose a reason for hiding this comment

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

This should just be glnx_throw, there's no relevant errno set here


if (loader_link)
{
/* Default and when loader is a link is to swap links */
if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion, cancellable,
error))
return FALSE;
}
else
{
/* Handle boot/loader as a directory, and swap with renameat2 RENAME_EXCHANGE */
if (!prepare_new_bootloader_dir (self, self->bootversion, new_bootversion, cancellable,
error))
return FALSE;
}

if (!full_system_sync (self, out_syncstats, cancellable, error))
return FALSE;

if (!swap_bootloader (self, bootloader, self->bootversion, new_bootversion, cancellable, error))
if (!swap_bootloader (self, bootloader, loader_link, self->bootversion, new_bootversion,
cancellable, error))
return FALSE;

if (out_subbootdir)
Expand Down
69 changes: 54 additions & 15 deletions src/libostree/ostree-sysroot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
#include "ostree-sysroot-private.h"
#include "otcore.h"

#define BOOTVERSION_FILE "boot/loader/ostree_bootversion"

/**
* SECTION:ostree-sysroot
* @title: Root partition mount point
Expand Down Expand Up @@ -601,6 +603,9 @@ compare_loader_configs_for_sorting (gconstpointer a_pp, gconstpointer b_pp)
return compare_boot_loader_configs (a, b);
}

static gboolean read_current_bootversion (OstreeSysroot *self, int *out_bootversion,
GCancellable *cancellable, GError **error);

/* Read all the bootconfigs from `/boot/loader/`. */
gboolean
_ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self, int bootversion,
Expand All @@ -613,7 +618,16 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self, int bootversion,
g_autoptr (GPtrArray) ret_loader_configs
= g_ptr_array_new_with_free_func ((GDestroyNotify)g_object_unref);

g_autofree char *entries_path = g_strdup_printf ("boot/loader.%d/entries", bootversion);
g_autofree char *entries_path = NULL;
int current_version;
if (!read_current_bootversion (self, &current_version, cancellable, error))
return FALSE;

if (current_version == bootversion)
entries_path = g_strdup ("boot/loader/entries");
else
entries_path = g_strdup_printf ("boot/loader.%d/entries", bootversion);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we ever reading anything else than the current config ? ie boot/loader/entries

gboolean entries_exists;
g_auto (GLnxDirFdIterator) dfd_iter = {
0,
Expand Down Expand Up @@ -660,7 +674,7 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self, int bootversion,
return TRUE;
}

/* Get the bootversion from the `/boot/loader` symlink. */
/* Get the bootversion from the `/boot/loader` directory or symlink. */
static gboolean
read_current_bootversion (OstreeSysroot *self, int *out_bootversion, GCancellable *cancellable,
GError **error)
Expand All @@ -673,24 +687,49 @@ read_current_bootversion (OstreeSysroot *self, int *out_bootversion, GCancellabl
return FALSE;
if (errno == ENOENT)
{
g_debug ("Didn't find $sysroot/boot/loader symlink; assuming bootversion 0");
g_debug ("Didn't find $sysroot/boot/loader directory or symlink; assuming bootversion 0");
ret_bootversion = 0;
}
else
{
if (!S_ISLNK (stbuf.st_mode))
return glnx_throw (error, "Not a symbolic link: boot/loader");

g_autofree char *target
= glnx_readlinkat_malloc (self->sysroot_fd, "boot/loader", cancellable, error);
if (!target)
return FALSE;
if (g_strcmp0 (target, "loader.0") == 0)
ret_bootversion = 0;
else if (g_strcmp0 (target, "loader.1") == 0)
ret_bootversion = 1;
if (S_ISLNK (stbuf.st_mode))
{
/* Traditional link, check version by reading link name */
g_autofree char *target
= glnx_readlinkat_malloc (self->sysroot_fd, "boot/loader", cancellable, error);
if (!target)
return FALSE;
if (g_strcmp0 (target, "loader.0") == 0)
ret_bootversion = 0;
else if (g_strcmp0 (target, "loader.1") == 0)
ret_bootversion = 1;
else
return glnx_throw (error, "Invalid target '%s' in boot/loader", target);
}
else
return glnx_throw (error, "Invalid target '%s' in boot/loader", target);
{
/* Loader is a directory, check version by reading ostree_bootversion */
glnx_autofd int bversion_fd = -1;
if (!ot_openat_ignore_enoent (self->sysroot_fd, BOOTVERSION_FILE, &bversion_fd, error))
return FALSE;

if (bversion_fd == -1)
{
g_debug ("File " BOOTVERSION_FILE " is not available, assuming bootversion 0");
ret_bootversion = 0;
}
else
{
g_autofree char *version
= glnx_fd_readall_utf8 (bversion_fd, NULL, cancellable, error);
if (g_strcmp0 (version, "loader.0") == 0)
ret_bootversion = 0;
else if (g_strcmp0 (version, "loader.1") == 0)
ret_bootversion = 1;
else
return glnx_throw (error, "Invalid version '%s' in " BOOTVERSION_FILE, version);
}
}
}

*out_bootversion = ret_bootversion;
Expand Down
2 changes: 1 addition & 1 deletion src/switchroot/ostree-prepare-root.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ main (int argc, char *argv[])
* at /boot inside the deployment. */
if (snprintf (srcpath, sizeof (srcpath), "%s/boot/loader", root_mountpoint) < 0)
err (EXIT_FAILURE, "failed to assemble /boot/loader path");
if (lstat (srcpath, &stbuf) == 0 && S_ISLNK (stbuf.st_mode))
if (lstat (srcpath, &stbuf) == 0 && (S_ISLNK (stbuf.st_mode) || S_ISDIR (stbuf.st_mode)))
{
if (lstat ("boot", &stbuf) == 0 && S_ISDIR (stbuf.st_mode))
{
Expand Down
Loading
Loading