From fb8b0869a7bc30e23be175cf978df23192d59118 Mon Sep 17 00:00:00 2001 From: Ivan Shapovalov Date: Tue, 13 Sep 2016 03:04:35 +0300 Subject: update-done, condition: write the timestamp to the file as well and use it to prevent false-positives This fixes https://bugs.freedesktop.org/show_bug.cgi?id=90192 and #4130 for real. Also, remove timestamp check in update-done.c altogether since the whole operation is idempotent. --- src/shared/condition.c | 42 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) (limited to 'src/shared/condition.c') diff --git a/src/shared/condition.c b/src/shared/condition.c index 6bb42c0692..f13fa6a9fd 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -37,6 +37,7 @@ #include "condition.h" #include "extract-word.h" #include "fd-util.h" +#include "fileio.h" #include "glob-util.h" #include "hostname-util.h" #include "ima-util.h" @@ -309,8 +310,45 @@ static int condition_test_needs_update(Condition *c) { if (lstat("/usr/", &usr) < 0) return true; - return usr.st_mtim.tv_sec > other.st_mtim.tv_sec || - (usr.st_mtim.tv_sec == other.st_mtim.tv_sec && usr.st_mtim.tv_nsec > other.st_mtim.tv_nsec); + /* + * First, compare seconds as they are always accurate... + */ + if (usr.st_mtim.tv_sec != other.st_mtim.tv_sec) + return usr.st_mtim.tv_sec > other.st_mtim.tv_sec; + + /* + * ...then compare nanoseconds. + * + * A false positive is only possible when /usr's nanoseconds > 0 + * (otherwise /usr cannot be strictly newer than the target file) + * AND the target file's nanoseconds == 0 + * (otherwise the filesystem supports nsec timestamps, see stat(2)). + */ + if (usr.st_mtim.tv_nsec > 0 && other.st_mtim.tv_nsec == 0) { + _cleanup_free_ char *timestamp_str = NULL; + uint64_t timestamp; + int r; + + r = parse_env_file(p, NULL, "TimestampNSec", ×tamp_str, NULL); + if (r < 0) { + log_error_errno(-r, "Failed to parse timestamp file '%s', using mtime: %m", p); + return true; + } else if (r == 0) { + log_debug("No data in timestamp file '%s', using mtime", p); + return true; + } + + r = safe_atou64(timestamp_str, ×tamp); + if (r < 0) { + log_error_errno(-r, "Failed to parse timestamp value '%s' in file '%s', using mtime: %m", + timestamp_str, p); + return true; + } + + other.st_mtim.tv_nsec = timestamp % NSEC_PER_SEC; + } + + return usr.st_mtim.tv_nsec > other.st_mtim.tv_nsec; } static int condition_test_first_boot(Condition *c) { -- cgit v1.2.3-54-g00ecf From ec2ebfd524447caf14e63b49b3f63117b93c7c78 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 21 Oct 2016 18:26:30 +0200 Subject: update-done: minor clean-ups This is a follow-up for fb8b0869a7bc30e23be175cf978df23192d59118, and makes a couple of minor clean-up changes: - The field name in the timestamp file is changed from "TimestampNSec=" to "TIMESTAMP_NSEC=". This is done simply to reflect the fact that we parse the file with the env var file parser, and hence the contents should better follow the usual capitalization of env vars, i.e. be all uppercase. - Needless negation of the errno parameter log_error_errno() and friends has been removed. - Instead of manually calculating the nsec remainder of the timestamp, use timespec_store(). - We now check whether we were able to write the timestamp file in full with fflush_and_check() the way we usually do it. --- src/shared/condition.c | 9 ++++----- src/update-done/update-done.c | 15 +++++++++------ 2 files changed, 13 insertions(+), 11 deletions(-) (limited to 'src/shared/condition.c') diff --git a/src/shared/condition.c b/src/shared/condition.c index f13fa6a9fd..69b4837e1f 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -329,9 +329,9 @@ static int condition_test_needs_update(Condition *c) { uint64_t timestamp; int r; - r = parse_env_file(p, NULL, "TimestampNSec", ×tamp_str, NULL); + r = parse_env_file(p, NULL, "TIMESTAMP_NSEC", ×tamp_str, NULL); if (r < 0) { - log_error_errno(-r, "Failed to parse timestamp file '%s', using mtime: %m", p); + log_error_errno(r, "Failed to parse timestamp file '%s', using mtime: %m", p); return true; } else if (r == 0) { log_debug("No data in timestamp file '%s', using mtime", p); @@ -340,12 +340,11 @@ static int condition_test_needs_update(Condition *c) { r = safe_atou64(timestamp_str, ×tamp); if (r < 0) { - log_error_errno(-r, "Failed to parse timestamp value '%s' in file '%s', using mtime: %m", - timestamp_str, p); + log_error_errno(r, "Failed to parse timestamp value '%s' in file '%s', using mtime: %m", timestamp_str, p); return true; } - other.st_mtim.tv_nsec = timestamp % NSEC_PER_SEC; + timespec_store(&other.st_mtim, timestamp); } return usr.st_mtim.tv_nsec > other.st_mtim.tv_nsec; diff --git a/src/update-done/update-done.c b/src/update-done/update-done.c index 5cc5abfddf..48c2a3fff4 100644 --- a/src/update-done/update-done.c +++ b/src/update-done/update-done.c @@ -18,6 +18,7 @@ ***/ #include "fd-util.h" +#include "fileio.h" #include "io-util.h" #include "selinux-util.h" #include "util.h" @@ -32,8 +33,8 @@ static int apply_timestamp(const char *path, struct timespec *ts) { *ts, *ts }; - int fd = -1; _cleanup_fclose_ FILE *f = NULL; + int fd = -1; int r; assert(path); @@ -59,18 +60,20 @@ static int apply_timestamp(const char *path, struct timespec *ts) { return log_error_errno(errno, "Failed to create/open timestamp file %s: %m", path); } - f = fdopen(fd, "w"); + f = fdopen(fd, "we"); if (!f) { safe_close(fd); return log_error_errno(errno, "Failed to fdopen() timestamp file %s: %m", path); } (void) fprintf(f, - "%s" - "TimestampNSec=" NSEC_FMT "\n", - MESSAGE, timespec_load_nsec(ts)); + MESSAGE + "TIMESTAMP_NSEC=" NSEC_FMT "\n", + timespec_load_nsec(ts)); - fflush(f); + r = fflush_and_check(f); + if (r < 0) + return log_error_errno(r, "Failed to write timestamp file: %m"); if (futimens(fd, twice) < 0) return log_error_errno(errno, "Failed to update timestamp on %s: %m", path); -- cgit v1.2.3-54-g00ecf From 239a5707e1bd7740b075e78a4837a77f1129cdaa Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Fri, 21 Oct 2016 22:56:58 -0400 Subject: shared/condition: add ConditionVirtualization=[!]private-users This can be useful to silence warnings about units which fail in userns container. --- src/shared/condition.c | 3 +++ src/test/test-condition.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) (limited to 'src/shared/condition.c') diff --git a/src/shared/condition.c b/src/shared/condition.c index f13fa6a9fd..376606e004 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -146,6 +146,9 @@ static int condition_test_virtualization(Condition *c) { assert(c->parameter); assert(c->type == CONDITION_VIRTUALIZATION); + if (streq(c->parameter, "private-users")) + return running_in_userns(); + v = detect_virtualization(); if (v < 0) return v; diff --git a/src/test/test-condition.c b/src/test/test-condition.c index 6f7d71ef9a..dd985f5863 100644 --- a/src/test/test-condition.c +++ b/src/test/test-condition.c @@ -31,6 +31,8 @@ #include "macro.h" #include "selinux-util.h" #include "smack-util.h" +#include "strv.h" +#include "virt.h" #include "util.h" static void test_condition_test_path(void) { @@ -265,7 +267,64 @@ static void test_condition_test_security(void) { condition_free(condition); } +static void test_condition_test_virtualization(void) { + Condition *condition; + const char *virt; + int r; + + condition = condition_new(CONDITION_VIRTUALIZATION, "garbage oifdsjfoidsjoj", false, false); + assert_se(condition); + r = condition_test(condition); + log_info("ConditionVirtualization=garbage → %i", r); + assert_se(r == 0); + condition_free(condition); + + condition = condition_new(CONDITION_VIRTUALIZATION, "container", false, false); + assert_se(condition); + r = condition_test(condition); + log_info("ConditionVirtualization=container → %i", r); + assert_se(r == !!detect_container()); + condition_free(condition); + + condition = condition_new(CONDITION_VIRTUALIZATION, "vm", false, false); + assert_se(condition); + r = condition_test(condition); + log_info("ConditionVirtualization=vm → %i", r); + assert_se(r == (detect_vm() && !detect_container())); + condition_free(condition); + + condition = condition_new(CONDITION_VIRTUALIZATION, "private-users", false, false); + assert_se(condition); + r = condition_test(condition); + log_info("ConditionVirtualization=private-users → %i", r); + assert_se(r == !!running_in_userns()); + condition_free(condition); + + NULSTR_FOREACH(virt, + "kvm\0" + "qemu\0" + "bochs\0" + "xen\0" + "uml\0" + "vmware\0" + "oracle\0" + "microsoft\0" + "zvm\0" + "parallels\0" + "bhyve\0" + "vm_other\0") { + + condition = condition_new(CONDITION_VIRTUALIZATION, virt, false, false); + assert_se(condition); + r = condition_test(condition); + log_info("ConditionVirtualization=%s → %i", virt, r); + assert_se(r >= 0); + condition_free(condition); + } +} + int main(int argc, char *argv[]) { + log_set_max_level(LOG_DEBUG); log_parse_environment(); log_open(); @@ -276,6 +335,7 @@ int main(int argc, char *argv[]) { test_condition_test_kernel_command_line(); test_condition_test_null(); test_condition_test_security(); + test_condition_test_virtualization(); return 0; } -- cgit v1.2.3-54-g00ecf From 0809d7740c5cf988e049781b4b80e14a3bbefb70 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Mon, 24 Oct 2016 22:53:07 -0400 Subject: condition: simplify condition_test_virtualization Rewrite the function to be slightly simpler. In particular, if a specific match is found (like ConditionVirtualization=yes), simply return an answer immediately, instead of relying that "yes" will not be matched by any of the virtualization names below. No functional change. --- src/shared/condition.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) (limited to 'src/shared/condition.c') diff --git a/src/shared/condition.c b/src/shared/condition.c index 376606e004..17b80d9e0c 100644 --- a/src/shared/condition.c +++ b/src/shared/condition.c @@ -155,19 +155,15 @@ static int condition_test_virtualization(Condition *c) { /* First, compare with yes/no */ b = parse_boolean(c->parameter); - - if (v > 0 && b > 0) - return true; - - if (v == 0 && b == 0) - return true; + if (b >= 0) + return b == !!v; /* Then, compare categorization */ - if (VIRTUALIZATION_IS_VM(v) && streq(c->parameter, "vm")) - return true; + if (streq(c->parameter, "vm")) + return VIRTUALIZATION_IS_VM(v); - if (VIRTUALIZATION_IS_CONTAINER(v) && streq(c->parameter, "container")) - return true; + if (streq(c->parameter, "container")) + return VIRTUALIZATION_IS_CONTAINER(v); /* Finally compare id */ return v != VIRTUALIZATION_NONE && streq(c->parameter, virtualization_to_string(v)); -- cgit v1.2.3-54-g00ecf