Skip to content

Commit

Permalink
Use rename of directories instead of symbolic links in boot partition.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@valentindavid
valentindavid committed Apr 13, 2020
1 parent 8baee5c commit 861306d
Show file tree
Hide file tree
Showing 4 changed files with 165 additions and 69 deletions.
9 changes: 9 additions & 0 deletions src/libostree/ostree-sysroot-cleanup.c
View file
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;
}

/* 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
99 changes: 75 additions & 24 deletions src/libostree/ostree-sysroot-deploy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1872,38 +1872,89 @@ install_deployment_kernel (OstreeSysroot *sysroot,
return TRUE;
}

/* 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.
*/
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_autofd int boot_dfd = -1;

GLNX_AUTO_PREFIX_ERROR ("Preparing new bootloader directory", error);
g_assert ((current_bootversion == 0 && new_bootversion == 1) ||
(current_bootversion == 1 && new_bootversion == 0));

g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion);
if (!glnx_opendirat (sysroot->sysroot_fd, "boot", TRUE, &boot_dfd, 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 (boot_dfd, 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 (boot_dfd, 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;
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 @@ -1916,11 +1967,8 @@ swap_bootloader (OstreeSysroot *sysroot,
if (!glnx_opendirat (sysroot->sysroot_fd, "boot", TRUE, &boot_dfd, 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 (boot_dfd, "loader.tmp", boot_dfd, "loader", error))
g_autofree char *new_target = g_strdup_printf ("loader.%d", new_bootversion);
if (!renameat2_exchange(boot_dfd, new_target, boot_dfd, "loader", is_atomic, error))
return FALSE;

/* Now we explicitly fsync this directory, even though it
Expand Down Expand Up @@ -2141,6 +2189,7 @@ write_deployments_bootswap (OstreeSysroot *self,
OstreeSysrootWriteDeploymentsOpts *opts,
OstreeBootloader *bootloader,
SyncStats *out_syncstats,
gboolean *is_atomic,
GCancellable *cancellable,
GError **error)
{
Expand Down Expand Up @@ -2203,15 +2252,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;

return TRUE;
Expand Down Expand Up @@ -2452,7 +2502,8 @@ ostree_sysroot_write_deployments_with_options (OstreeSysroot *self,

/* Note equivalent of try/finally here */
gboolean success = write_deployments_bootswap (self, new_deployments, opts, bootloader,
&syncstats, cancellable, error);
&syncstats, &bootloader_is_atomic,
cancellable, error);
/* Below here don't set GError until the if (!success) check.
* Note we only bother remounting if a mount namespace isn't in use.
* */
Expand Down
102 changes: 65 additions & 37 deletions src/libostree/ostree-sysroot.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,60 @@ compare_loader_configs_for_sorting (gconstpointer a_pp,
return compare_boot_loader_configs (a, b);
}

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)
{
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;
}

gboolean
_ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self,
int bootversion,
Expand All @@ -527,12 +581,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 @@ -572,42 +636,6 @@ _ostree_sysroot_read_boot_loader_configs (OstreeSysroot *self,
return TRUE;
}

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)
{
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
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,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 @@ -97,8 +98,9 @@ ${CMD_PREFIX} ostree admin deploy --os=testos testos:testos/buildmaster/x86_64-r
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 @@ -115,8 +117,9 @@ echo "ok second deploy"

${CMD_PREFIX} ostree admin deploy --os=testos testos:testos/buildmaster/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 @@ -130,7 +133,8 @@ ${CMD_PREFIX} ostree admin os-init otheros

${CMD_PREFIX} ostree admin deploy --os=otheros testos/buildmaster/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 @@ -142,8 +146,9 @@ validate_bootloader
echo "ok independent deploy"

${CMD_PREFIX} ostree admin deploy --retain --os=testos testos:testos/buildmaster/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 @@ -160,7 +165,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/buildmaster/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 @@ -184,8 +190,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/buildmaster/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 @@ -195,7 +202,8 @@ echo "ok deploy --not-as-default"

${CMD_PREFIX} ostree admin deploy --retain-rollback --os=otheros testos:testos/buildmaster/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

0 comments on commit 861306d

Please sign in to comment.