Merge pull request #31311 from yuwata/journal-user-corruption

journal: fix user journal corruption on rotation

src/basic/stat-util.c

@@ -260,11 +260,31 @@ int path_is_network_fs(const char *path) {
         return is_network_fs(&s);
 }
 
+int stat_verify_linked(const struct stat *st) {
+        assert(st);
+
+        if (st->st_nlink <= 0)
+                return -EIDRM; /* recognizable error. */
+
+        return 0;
+}
+
+int fd_verify_linked(int fd) {
+        struct stat st;
+
+        assert(fd >= 0);
+
+        if (fstat(fd, &st) < 0)
+                return -errno;
+
+        return stat_verify_linked(&st);
+}
+
 int stat_verify_regular(const struct stat *st) {
         assert(st);
 
-        /* Checks whether the specified stat() structure refers to a regular file. If not returns an appropriate error
-         * code. */
+        /* Checks whether the specified stat() structure refers to a regular file. If not returns an
+         * appropriate error code. */
 
         if (S_ISDIR(st->st_mode))
                 return -EISDIR;

src/basic/stat-util.h

@@ -72,6 +72,9 @@ int path_is_network_fs(const char *path);
  */
 #define F_TYPE_EQUAL(a, b) (a == (typeof(a)) b)
 
+int stat_verify_linked(const struct stat *st);
+int fd_verify_linked(int fd);
+
 int stat_verify_regular(const struct stat *st);
 int fd_verify_regular(int fd);
 int verify_regular_at(int dir_fd, const char *path, bool follow);

src/libsystemd/sd-journal/journal-file.c

@@ -734,8 +734,9 @@ int journal_file_fstat(JournalFile *f) {
                 return r;
 
         /* Refuse appending to files that are already deleted */
-        if (f->last_stat.st_nlink <= 0)
-                return -EIDRM;
+        r = stat_verify_linked(&f->last_stat);
+        if (r < 0)
+                return r;
 
         return 0;
 }

src/shared/copy.c

@@ -209,6 +209,7 @@ int copy_bytes_full(
                                         r = reflink_range(fdf, foffset, fdt, toffset, max_bytes == UINT64_MAX ? 0 : max_bytes); /* partial reflink */
                                 if (r >= 0) {
                                         off_t t;
+                                        int ret;
 
                                         /* This worked, yay! Now — to be fully correct — let's adjust the file pointers */
                                         if (max_bytes == UINT64_MAX) {
@@ -227,7 +228,14 @@ int copy_bytes_full(
                                                 if (t < 0)
                                                         return -errno;
 
-                                                return 0; /* we copied the whole thing, hence hit EOF, return 0 */
+                                                if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) {
+                                                        r = fd_verify_linked(fdf);
+                                                        if (r < 0)
+                                                                return r;
+                                                }
+
+                                                /* We copied the whole thing, hence hit EOF, return 0. */
+                                                ret = 0;
                                         } else {
                                                 t = lseek(fdf, foffset + max_bytes, SEEK_SET);
                                                 if (t < 0)
@@ -237,8 +245,18 @@ int copy_bytes_full(
                                                 if (t < 0)
                                                         return -errno;
 
-                                                return 1; /* we copied only some number of bytes, which worked, but this means we didn't hit EOF, return 1 */
+                                                /* We copied only some number of bytes, which worked, but
+                                                 * this means we didn't hit EOF, return 1. */
+                                                ret = 1;
                                         }
+
+                                        if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) {
+                                                r = fd_verify_linked(fdf);
+                                                if (r < 0)
+                                                        return r;
+                                        }
+
+                                        return ret;
                                 }
                         }
                 }
@@ -484,6 +502,12 @@ int copy_bytes_full(
                 copied_something = true;
         }
 
+        if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) {
+                r = fd_verify_linked(fdf);
+                if (r < 0)
+                        return r;
+        }
+
         if (copy_flags & COPY_TRUNCATE) {
                 off_t off = lseek(fdt, 0, SEEK_CUR);
                 if (off < 0)
@@ -799,6 +823,12 @@ static int fd_copy_regular(
         (void) futimens(fdt, (struct timespec[]) { st->st_atim, st->st_mtim });
         (void) copy_xattr(fdf, NULL, fdt, NULL, copy_flags);
 
+        if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) {
+                r = fd_verify_linked(fdf);
+                if (r < 0)
+                        return r;
+        }
+
         if (copy_flags & COPY_FSYNC) {
                 if (fsync(fdt) < 0) {
                         r = -errno;
@@ -1334,6 +1364,12 @@ int copy_file_fd_at_full(
                 (void) copy_xattr(fdf, NULL, fdt, NULL, copy_flags);
         }
 
+        if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) {
+                r = fd_verify_linked(fdf);
+                if (r < 0)
+                        return r;
+        }
+
         if (copy_flags & COPY_FSYNC_FULL) {
                 r = fsync_full(fdt);
                 if (r < 0)
@@ -1404,6 +1440,12 @@ int copy_file_at_full(
         (void) copy_times(fdf, fdt, copy_flags);
         (void) copy_xattr(fdf, NULL, fdt, NULL, copy_flags);
 
+        if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) {
+                r = fd_verify_linked(fdf);
+                if (r < 0)
+                        goto fail;
+        }
+
         if (chattr_mask != 0)
                 (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL);
 

src/shared/copy.h

@@ -30,6 +30,7 @@ typedef enum CopyFlags {
         COPY_GRACEFUL_WARN = 1 << 15, /* Skip copying file types that aren't supported by the target filesystem */
         COPY_TRUNCATE      = 1 << 16, /* Truncate to current file offset after copying */
         COPY_LOCK_BSD      = 1 << 17, /* Return a BSD exclusively locked file descriptor referring to the copied image/directory. */
+        COPY_VERIFY_LINKED = 1 << 18, /* Check the source file is still linked after copying. */
 } CopyFlags;
 
 typedef enum DenyType {

src/shared/journal-file-util.c

@@ -210,11 +210,15 @@ static void journal_file_set_offline_internal(JournalFile *f) {
 
                                 log_debug_errno(r, "Failed to re-enable copy-on-write for %s: %m, rewriting file", f->path);
 
+                                /* Here, setting COPY_VERIFY_LINKED flag is crucial. Otherwise, a broken
+                                 * journal file may be created, if journal_directory_vacuum() ->
+                                 * unlinkat_deallocate() is called in the main thread while this thread is
+                                 * copying the file. See issue #24150 and #31222. */
                                 r = copy_file_atomic_at_full(
                                                 f->fd, NULL, AT_FDCWD, f->path, f->mode,
                                                 0,
                                                 FS_NOCOW_FL,
-                                                COPY_REPLACE | COPY_FSYNC | COPY_HOLES | COPY_ALL_XATTRS,
+                                                COPY_REPLACE | COPY_FSYNC | COPY_HOLES | COPY_ALL_XATTRS | COPY_VERIFY_LINKED,
                                                 NULL, NULL);
                                 if (r < 0) {
                                         log_debug_errno(r, "Failed to rewrite %s: %m", f->path);

src/test/test-copy.c

@@ -566,4 +566,26 @@ TEST(copy_lock) {
         fd = safe_close(fd);
 }
 
+TEST(copy_verify_linked) {
+        _cleanup_(rm_rf_physical_and_freep) char *t = NULL;
+        _cleanup_close_ int tfd = -EBADF, fd_1 = -EBADF, fd_2 = -EBADF;
+
+        tfd = mkdtemp_open(NULL, O_PATH, &t);
+        assert_se(tfd >= 0);
+
+        assert_se(write_string_file_at(tfd, "hoge", "bar bar", WRITE_STRING_FILE_CREATE) >= 0);
+
+        fd_1 = openat(tfd, "hoge", O_CLOEXEC | O_NOCTTY | O_RDONLY);
+        assert_se(fd_1 >= 0);
+        fd_2 = openat(tfd, "hoge", O_CLOEXEC | O_NOCTTY | O_RDONLY);
+        assert_se(fd_2 >= 0);
+        assert_se(unlinkat(tfd, "hoge", 0) >= 0);
+
+        assert_se(copy_file_at(fd_1, NULL, tfd, "to_1", 0, 0644, 0) >= 0);
+        assert_se(read_file_at_and_streq(tfd, "to_1", "bar bar\n"));
+
+        assert_se(copy_file_at(fd_2, NULL, tfd, "to_2", O_EXCL, 0644, COPY_VERIFY_LINKED) == -EIDRM);
+        assert_se(faccessat(tfd, "to_2", F_OK, AT_SYMLINK_NOFOLLOW) < 0 && errno == ENOENT);
+}
+
 DEFINE_TEST_MAIN(LOG_DEBUG);

src/test/test-stat-util.c

@@ -195,6 +195,25 @@ TEST(inode_type_from_string) {
                 assert_se(inode_type_from_string(inode_type_to_string(*m)) == *m);
 }
 
+TEST(fd_verify_linked) {
+        _cleanup_(rm_rf_physical_and_freep) char *t = NULL;
+        _cleanup_close_ int tfd = -EBADF, fd = -EBADF;
+        _cleanup_free_ char *p = NULL;
+
+        tfd = mkdtemp_open(NULL, O_PATH, &t);
+        assert_se(tfd >= 0);
+
+        assert_se(p = path_join(t, "hoge"));
+        assert_se(touch(p) >= 0);
+
+        fd = open(p, O_CLOEXEC | O_PATH);
+        assert_se(fd >= 0);
+
+        assert_se(fd_verify_linked(fd) >= 0);
+        assert_se(unlinkat(tfd, "hoge", 0) >= 0);
+        assert_se(fd_verify_linked(fd) == -EIDRM);
+}
+
 static int intro(void) {
         log_show_color(true);
         return EXIT_SUCCESS;

test/test-functions

@@ -2103,7 +2103,7 @@ install_testuser() {
     # create unprivileged user for user manager tests
     mkdir -p "${initdir:?}/etc/sysusers.d"
     cat >"$initdir/etc/sysusers.d/testuser.conf" <<EOF
-u testuser    4711     "Test User" /home/testuser
+u testuser    4711     "Test User" /home/testuser /bin/bash
 EOF
 
     mkdir -p "$initdir/home/testuser"

test/units/testsuite-04.journal-corrupt.sh

@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: LGPL-2.1-or-later
+set -eux
+set -o pipefail
+
+journalctl --rotate --vacuum-files=1
+# Nuke all archived journals, so we start with a clean slate
+rm -f "/var/log/journal/$(</etc/machine-id)"/system@*.journal
+rm -f "/var/log/journal/$(</etc/machine-id)"/user-*@*.journal
+journalctl --header | grep path
+
+# Make sure the user instance is active when we rotate journals
+systemd-run --unit user-sleep.service --user -M testuser@ sleep infinity
+
+for _ in {0..9}; do
+    journalctl --rotate
+    journalctl --sync
+    SYSTEMD_LOG_LEVEL=debug journalctl -n1 -q
+    (! journalctl -n0 -q |& grep corrupted)
+done
+
+systemctl stop --user -M testuser@ user-sleep.service
+
+journalctl --sync
+journalctl --rotate --vacuum-files=1
+# Nuke all archived journals, so we start with a clean slate
+rm -f "/var/log/journal/$(</etc/machine-id)"/system@*.journal
+rm -f "/var/log/journal/$(</etc/machine-id)/"user-*@*.journal
+journalctl --header | grep path
+
+for _ in {0..9}; do
+    journalctl --rotate --vacuum-files=1
+    journalctl --sync
+    SYSTEMD_LOG_LEVEL=debug journalctl -n1 -q
+    (! journalctl -n0 -q |& grep corrupted)
+done