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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+166 −68
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions src/libostree/ostree-sysroot-cleanup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,15 @@ cleanup_other_bootversions (OstreeSysroot *self,
const int cleanup_subbootversion = self->subbootversion == 0 ? 1 : 0;
/* Reusable buffer for path */
g_autoptr(GString) buf = g_string_new ("");
struct stat stbuf;

if ((glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW, error))
&& (!S_ISLNK (stbuf.st_mode)))
{
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;
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


/* These directories are for the other major version */
g_string_truncate (buf, 0); g_string_append_printf (buf, "boot/loader.%d", cleanup_bootversion);
Expand Down
95 changes: 74 additions & 21 deletions src/libostree/ostree-sysroot-deploy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2026,18 +2026,18 @@ install_deployment_kernel (OstreeSysroot *sysroot,
return TRUE;
}

/* We generate the symlink on disk, then potentially do a syncfs() to ensure
/* 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.
*/
Copy link
Member

Choose a reason for hiding this comment

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

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.

static gboolean
prepare_new_bootloader_link (OstreeSysroot *sysroot,
int current_bootversion,
int new_bootversion,
GCancellable *cancellable,
GError **error)
prepare_new_bootloader_dir (OstreeSysroot *sysroot,
int current_bootversion,
int new_bootversion,
GCancellable *cancellable,
GError **error)
{
GLNX_AUTO_PREFIX_ERROR ("Preparing final bootloader swap", error);
GLNX_AUTO_PREFIX_ERROR ("Preparing new bootloader directory", error);
g_assert ((current_bootversion == 0 && new_bootversion == 1) ||
(current_bootversion == 1 && new_bootversion == 0));

Expand All @@ -2047,23 +2047,76 @@ prepare_new_bootloader_link (OstreeSysroot *sysroot,
if (errno != EEXIST)
return glnx_throw_errno_prefix (error, "symlinkat");

g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion);
if (!_ostree_sysroot_ensure_boot_fd (sysroot, error))
return FALSE;

/* We shouldn't actually need to replace but it's easier to reuse
that code */
if (!symlink_at_replace (new_target, sysroot->sysroot_fd, "boot/loader.tmp",
cancellable, error))
g_autofree char *loader_dir_name = g_strdup_printf ("loader.%d", new_bootversion);

if (!glnx_shutil_mkdir_p_at (sysroot->boot_fd, loader_dir_name, 0755,
cancellable, error))
return FALSE;

g_autofree char *version_name = g_strdup_printf ("%s/version", loader_dir_name);

if (!glnx_file_replace_contents_at (sysroot->boot_fd, version_name,
(guint8*)loader_dir_name, strlen(loader_dir_name),
0, cancellable, error))
return FALSE;

return TRUE;
}

static gboolean
renameat2_exchange (int olddirfd,
const char *oldpath,
int newdirfd,
const char *newpath,
gboolean *is_atomic,
GError **error)
{
if (renameat2 (olddirfd, oldpath, newdirfd, newpath, RENAME_EXCHANGE) == 0)
return TRUE;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I missed that part.

else
{
if ((errno == EINVAL)
|| (errno == ENOSYS))
{
if (glnx_renameat2_exchange (olddirfd, oldpath, newdirfd, newpath) == 0)
{
is_atomic = FALSE;
return TRUE;
}
}
}

if (errno != ENOENT)
return glnx_throw_errno_prefix (error, "renameat2");

if (renameat2 (olddirfd, oldpath, newdirfd, newpath, RENAME_NOREPLACE) == 0)
return TRUE;
else
{
if ((errno == EINVAL)
|| (errno == ENOSYS))
{
if (glnx_renameat2_noreplace (olddirfd, oldpath, newdirfd, newpath) == 0)
{
is_atomic = FALSE;
return TRUE;
}
}
}

return glnx_throw_errno_prefix (error, "renameat2");
}

/* 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,
gboolean *is_atomic,
GCancellable *cancellable,
GError **error)
{
Expand All @@ -2075,11 +2128,8 @@ swap_bootloader (OstreeSysroot *sysroot,
if (!_ostree_sysroot_ensure_boot_fd (sysroot, 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))
g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion);
if (!renameat2_exchange(sysroot->boot_fd, new_target, sysroot->boot_fd, "loader", is_atomic, error))
return FALSE;

/* Now we explicitly fsync this directory, even though it
Expand Down Expand Up @@ -2254,6 +2304,7 @@ write_deployments_bootswap (OstreeSysroot *self,
OstreeBootloader *bootloader,
SyncStats *out_syncstats,
char **out_subbootdir,
gboolean *is_atomic,
GCancellable *cancellable,
GError **error)
{
Expand Down Expand Up @@ -2317,15 +2368,16 @@ write_deployments_bootswap (OstreeSysroot *self,
return glnx_prefix_error (error, "Bootloader write config");
}

if (!prepare_new_bootloader_link (self, self->bootversion, new_bootversion,
cancellable, error))
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))
is_atomic, cancellable, error))

return FALSE;

if (out_subbootdir)
Expand Down Expand Up @@ -2534,7 +2586,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self,
bootloader_is_atomic = bootloader != NULL && _ostree_bootloader_is_atomic (bootloader);

if (!write_deployments_bootswap (self, new_deployments, opts, bootloader,
&syncstats, &new_subbootdir, cancellable, error))
&syncstats, &new_subbootdir, &bootloader_is_atomic,
cancellable, error))
return FALSE;
}

Expand Down
106 changes: 67 additions & 39 deletions src/libostree/ostree-sysroot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,62 @@ compare_loader_configs_for_sorting (gconstpointer a_pp,
return compare_boot_loader_configs (a, b);
}

/* Get the bootversion from the `/boot/loader` directory or symlink. */
static gboolean
read_current_bootversion (OstreeSysroot *self,
int *out_bootversion,
GCancellable *cancellable,
GError **error)
{
int ret_bootversion;
struct stat stbuf;

if (!glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW, error))
return FALSE;
if (errno == ENOENT)
{
g_debug ("Didn't find $sysroot/boot/loader directory or symlink; assuming bootversion 0");
ret_bootversion = 0;
}
else
{
if (!S_ISLNK (stbuf.st_mode))
{
gsize len;
g_autofree char* version_content = glnx_file_get_contents_utf8_at(self->sysroot_fd, "boot/loader/version",
&len, cancellable, error);
if (version_content == NULL) {
return FALSE;
}
if (len != 8)
return glnx_throw (error, "Invalid version in boot/loader/version");
else if (g_strcmp0 (version_content, "loader.0") == 0)
ret_bootversion = 0;
else if (g_strcmp0 (version_content, "loader.1") == 0)
ret_bootversion = 1;
else
return glnx_throw (error, "Invalid version in boot/loader/version");
}
else
{
/* Backward compatibility with boot symbolic links */
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);
}
}

*out_bootversion = ret_bootversion;
return TRUE;
}

/* Read all the bootconfigs from `/boot/loader/`. */
gboolean
_ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self,
Expand All @@ -571,12 +627,22 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self,
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);

gboolean entries_exists;
g_auto(GLnxDirFdIterator) dfd_iter = { 0, };
if (!ot_dfd_iter_init_allow_noent (self->sysroot_fd, entries_path,
&dfd_iter, &entries_exists, error))
return FALSE;

if (!entries_exists)
{
/* Note early return */
Expand Down Expand Up @@ -616,44 +682,6 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self,
return TRUE;
}

/* Get the bootversion from the `/boot/loader` symlink. */
static gboolean
read_current_bootversion (OstreeSysroot *self,
int *out_bootversion,
GCancellable *cancellable,
GError **error)
{
int ret_bootversion;
struct stat stbuf;

if (!glnx_fstatat_allow_noent (self->sysroot_fd, "boot/loader", &stbuf, AT_SYMLINK_NOFOLLOW, error))
return FALSE;
if (errno == ENOENT)
{
g_debug ("Didn't find $sysroot/boot/loader 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;
else
return glnx_throw (error, "Invalid target '%s' in boot/loader", target);
}

*out_bootversion = ret_bootversion;
return TRUE;
}

static gboolean
load_origin (OstreeSysroot *self,
OstreeDeployment *deployment,
Expand Down
24 changes: 16 additions & 8 deletions tests/admin-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ echo "ok --print-current-dir"

# Test layout of bootloader config and refs
assert_not_has_dir sysroot/boot/loader.0
assert_has_dir sysroot/boot/loader.1
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.1'
assert_has_dir sysroot/ostree/boot.1.1
assert_has_file sysroot/boot/loader/entries/ostree-1-testos.conf
assert_file_has_content sysroot/boot/loader/entries/ostree-1-testos.conf 'options.* root=LABEL=MOO'
Expand All @@ -103,8 +104,9 @@ ${CMD_PREFIX} ostree admin deploy --os=testos testos:testos/buildmain/x86_64-run
new_mtime=$(stat -c '%.Y' sysroot/ostree/deploy)
assert_not_streq "${orig_mtime}" "${new_mtime}"
# Need a new bootversion, sine we now have two deployments
assert_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.0'
assert_has_dir sysroot/ostree/boot.0.1
assert_not_has_dir sysroot/ostree/boot.0.0
assert_not_has_dir sysroot/ostree/boot.1.0
Expand All @@ -121,8 +123,9 @@ echo "ok second deploy"

${CMD_PREFIX} ostree admin deploy --os=testos testos:testos/buildmain/x86_64-runtime
# Keep the same bootversion
assert_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.0'
# But swap subbootversion
assert_has_dir sysroot/ostree/boot.0.0
assert_not_has_dir sysroot/ostree/boot.0.1
Expand All @@ -136,7 +139,8 @@ ${CMD_PREFIX} ostree admin os-init otheros

${CMD_PREFIX} ostree admin deploy --os=otheros testos/buildmain/x86_64-runtime
assert_not_has_dir sysroot/boot/loader.0
assert_has_dir sysroot/boot/loader.1
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.1'
assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf
assert_has_file sysroot/boot/loader/entries/ostree-3-otheros.conf
assert_file_has_content sysroot/ostree/deploy/testos/deploy/${rev}.1/etc/os-release 'NAME=TestOS'
Expand All @@ -148,8 +152,9 @@ validate_bootloader
echo "ok independent deploy"

${CMD_PREFIX} ostree admin deploy --retain --os=testos testos:testos/buildmain/x86_64-runtime
assert_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.0'
assert_has_file sysroot/boot/loader/entries/ostree-4-testos.conf
assert_file_has_content sysroot/ostree/deploy/testos/deploy/${rev}.2/etc/os-release 'NAME=TestOS'
assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf
Expand All @@ -166,7 +171,8 @@ rm sysroot/ostree/deploy/testos/deploy/${rev}.3/etc/aconfigfile
ln -s /ENOENT sysroot/ostree/deploy/testos/deploy/${rev}.3/etc/a-new-broken-symlink
${CMD_PREFIX} ostree admin deploy --retain --os=testos testos:testos/buildmain/x86_64-runtime
assert_not_has_dir sysroot/boot/loader.0
assert_has_dir sysroot/boot/loader.1
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.1'
link=sysroot/ostree/deploy/testos/deploy/${rev}.4/etc/a-new-broken-symlink
if ! test -L ${link}; then
ls -al ${link}
Expand All @@ -190,8 +196,9 @@ assert_has_file sysroot/boot/loader/entries/ostree-1-testos.conf
assert_not_has_file sysroot/boot/loader/entries/ostree-2-testos.conf
assert_not_has_file sysroot/boot/loader/entries/ostree-3-otheros.conf
${CMD_PREFIX} ostree admin deploy --not-as-default --os=otheros testos:testos/buildmain/x86_64-runtime
assert_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.0
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.0'
assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf
assert_has_file sysroot/boot/loader/entries/ostree-1-otheros.conf
${CMD_PREFIX} ostree admin status
Expand All @@ -201,7 +208,8 @@ echo "ok deploy --not-as-default"

${CMD_PREFIX} ostree admin deploy --retain-rollback --os=otheros testos:testos/buildmain/x86_64-runtime
assert_not_has_dir sysroot/boot/loader.0
assert_has_dir sysroot/boot/loader.1
assert_not_has_dir sysroot/boot/loader.1
assert_file_has_content sysroot/boot/loader/version 'loader.1'
assert_has_file sysroot/boot/loader/entries/ostree-3-otheros.conf
assert_has_file sysroot/boot/loader/entries/ostree-2-testos.conf
assert_has_file sysroot/boot/loader/entries/ostree-1-otheros.conf
Expand Down