(f)sync missing when writing files

[pohly]

I'm using systemd on a custom OS where IMA (https://sourceforge.net/p/linux-ima/wiki/Home/) is active with an IMA policy that allows locally created files. In that policy, the kernel adds the security.ima xattr when files get created. Without the proper security.ima, data cannot be read from these files after a reboot.

I found that files created by systemd are susceptible to a sudden power loss because ext4 creates the files, but their security.ima xattr does not get flushed to disk when powering off hard.

The following two changes make the system more resilient:

diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c
index 145ba2a..f710d30 100644
--- a/src/core/machine-id-setup.c
+++ b/src/core/machine-id-setup.c
@@ -356,6 +356,7 @@ int machine_id_commit(const char *root) {
         if (r < 0)
                 return log_error_errno(r, "Cannot write %s: %m", etc_machine_id);

+        fsync(fd);
         fd = safe_close(fd);

         /* Return to initial namespace and proceed a lazy tmpfs unmount */
diff --git a/src/random-seed/random-seed.c b/src/random-seed/random-seed.c
index d857ade..19f8122 100644
--- a/src/random-seed/random-seed.c
+++ b/src/random-seed/random-seed.c
@@ -161,6 +161,7 @@ int main(int argc, char *argv[]) {
                 r = loop_write(seed_fd, buf, (size_t) k, false);
                 if (r < 0)
                         log_error_errno(r, "Failed to write new random seed file: %m");
+                fsync(seed_fd);
         }

 finish:

I'm aware of the missing error handling - this is just a proof-of-concept to check whether adding fsyncs() would help. It does, and it looks like the right thing to do in any case, so I'm wondering whether adding it to systemd would make sense.

[poettering]

Hmm, but this sounds like something to fix in ext4, no? I am really not keen on adding fsync()s all over systemd just to make ext4 work nicer...

If they sync the other data on close, then they should really sync the xattrs too, no?

[crrodriguez]

This is is the expected behaviour of both systemd and kernel (which defers writes, this can be tuned with sysctl knobs)

[ohsix]

http://danluu.com/file-consistency/ relevant to filesystem consistency / weirdness

[pohly]

I agree that this should better be fixed in the IMA subsystem. Enabling it for "normal" files considerably weakens filesystem semantics. I have a discussion on-going with the upstream developers here: https://sourceforge.net/p/linux-ima/mailman/message/34850685/

With this ticket here I wanted to find out what systemd's assumption about fs consistency are. I remember reading the http://danluu.com/file-consistency/ article and as pointed out there, getting this right in all cases (even without IMA) is hard. Sometimes, calling fsync() is necessary.

I'm fine with getting this ticket here closed as "works as intended". My current solution (workaround for IMA?) is based on ExecStart/StopPost=/bin/sync in .d files for the two services.

[poettering]

I'd be fine with add the fsync() to the machine id case, after all that's a one-time thing, and really is a case where some identitiy is "burned into" the file system, hence an fsync() invocation might be OK (but cast to (void) please). But the random-seed case seems off...

[pohly]

Why should random-seed be treated differently? It also writes to the filesystem.

FWIW, here's the current version of the patch, with commit message and error handling:

From b563f27f190ba105ed56fc626c12f510fe67cd60 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Mon, 15 Feb 2016 15:10:48 +0100
Subject: [PATCH] machine-id-setup.c, random-seed.c: fsync new file content

Both /etc/machine-id and /var/lib/systemd/random-seed are critical
files where systemd should do its best to ensure that they get written
to disk properly. Explicitly calling fsync() helps with that.

In practice, this really makes a difference for the problem
described here:
https://sourceforge.net/p/linux-ima/mailman/message/34850685/
---
 src/core/machine-id-setup.c   | 3 +++
 src/random-seed/random-seed.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c
index 145ba2a..1b14317 100644
--- a/src/core/machine-id-setup.c
+++ b/src/core/machine-id-setup.c
@@ -356,6 +356,9 @@ int machine_id_commit(const char *root) {
         if (r < 0)
                 return log_error_errno(r, "Cannot write %s: %m", etc_machine_id);

+        r = fsync(fd);
+        if (!r)
+                log_error_errno(errno, "Cannot fsync %s: %m", etc_machine_id);
         fd = safe_close(fd);

         /* Return to initial namespace and proceed a lazy tmpfs unmount */
diff --git a/src/random-seed/random-seed.c b/src/random-seed/random-seed.c
index d857ade..66be07f 100644
--- a/src/random-seed/random-seed.c
+++ b/src/random-seed/random-seed.c
@@ -161,6 +161,9 @@ int main(int argc, char *argv[]) {
                 r = loop_write(seed_fd, buf, (size_t) k, false);
                 if (r < 0)
                         log_error_errno(r, "Failed to write new random seed file: %m");
+                r = fsync(seed_fd);
+                if (!r)
+                        log_error_errno(errno, "Failed to fsync new random seed file: %m");
         }

 finish:
-- 
2.1.4

[poettering]

the random seed file is a safety net (after all, normally, the kernel should be good enough at generating randomness, hopefully), and it's updated on every boot and shutdown. As such it is very different from the machine id, which is a one-time thing, has a very strict "commit" character, and where it is pretty important that the setting isn't lost (after all, it's about making the machine id stable, hence it better do it).

hence, please, let's add the fsync to the machine-id, but not to the random stuff...

[pohly]

In my experience, systemd did not recover from a "random-seed without security.ima xattr" failure well: opening the file then fails with "permission denied" and random-seed bails out early instead of doing anything.

If the file content is considered non-essential, then a corruption issue of the failure should be dealt with by wiping it out and behaving as if it hadn't been present at all, while at most warning about the situation.

Here's a corresponding patch which worked for me:

From 10e736cb970f285e09bf9180f6edc1259a3eb437 Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Tue, 16 Feb 2016 17:39:24 +0100
Subject: [PATCH] random-seed.c: ignore unusable random-seed file

When the existing random-seed file becomes unusable, for example when
access gets denied because IMA did not manage to update the
security.ima xattr, systemd should try to ignore the existing file
and start from scratch after deleting it.
---
 src/random-seed/random-seed.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/random-seed/random-seed.c b/src/random-seed/random-seed.c
index d857ade..4140836 100644
--- a/src/random-seed/random-seed.c
+++ b/src/random-seed/random-seed.c
@@ -90,11 +90,20 @@ int main(int argc, char *argv[]) {
                 if (seed_fd < 0) {
                         seed_fd = open(RANDOM_SEED, O_RDONLY|O_CLOEXEC|O_NOCTTY);
                         if (seed_fd < 0) {
-                                r = log_error_errno(errno, "Failed to open " RANDOM_SEED ": %m");
-                                goto finish;
-                        }
-
-                        refresh_seed_file = false;
+                                log_error_errno(errno, "Failed to open " RANDOM_SEED ", will replace it: %m");
+                                r = unlink(RANDOM_SEED);
+                                if (r) {
+                                        r = log_error_errno(errno, "Failed to unlink " RANDOM_SEED ": %m");
+                                        goto finish;
+
+                                }
+                                seed_fd = open(RANDOM_SEED, O_RDWR|O_CLOEXEC|O_NOCTTY|O_CREAT, 0600);
+                                if (seed_fd < 0) {
+                                        r = log_error_errno(errno, "Failed to create " RANDOM_SEED ": %m");
+                                        goto finish;
+                                }
+                        } else
+                                refresh_seed_file = false;
                 }

                 random_fd = open("/dev/urandom", O_RDWR|O_CLOEXEC|O_NOCTTY, 0600);
-- 
2.1.4

And here's the reduced machine-id patch:

From 1adfca6b93bf95a57231d0ad92d531943f1beccd Mon Sep 17 00:00:00 2001
From: Patrick Ohly <patrick.ohly@intel.com>
Date: Mon, 15 Feb 2016 15:10:48 +0100
Subject: [PATCH] machine-id-setup.c: fsync new file content

/etc/machine-id is a critical file where systemd should do its best to
ensure that it gets written to disk properly. Explicitly calling
fsync() helps with that.

In practise, this really makes a difference for the problem
described here:
https://sourceforge.net/p/linux-ima/mailman/message/34850685/
---
 src/core/machine-id-setup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/core/machine-id-setup.c b/src/core/machine-id-setup.c
index 145ba2a..1b14317 100644
--- a/src/core/machine-id-setup.c
+++ b/src/core/machine-id-setup.c
@@ -356,6 +356,9 @@ int machine_id_commit(const char *root) {
         if (r < 0)
                 return log_error_errno(r, "Cannot write %s: %m", etc_machine_id);

+        r = fsync(fd);
+        if (!r)
+                log_error_errno(errno, "Cannot fsync %s: %m", etc_machine_id);
         fd = safe_close(fd);

         /* Return to initial namespace and proceed a lazy tmpfs unmount */
-- 
2.1.4

[poettering]

The IMA thing should be fixed in IMA/ext4, systemd is not the place to work around problems in behaviour with that.

I am OK with adding the machine-id fsync patch independently of the IMA thing, because it appears like something where "commiting" this reliably sounds like an OK idea. But could you please submit that part as PR?

Also, the failure to sync should proably result in failure of the tool (i.e. a "return" is missing).