From 76e15a9cab0e0b0b8a166c14f60b170ff5cb58fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Jan 2019 15:51:13 +0100 Subject: [PATCH 1/6] tmpfiles: clean up restoring of access times after aging a bit Let's minimize file scope, use compund literals and only use LOG_WARN for errors we ignore. --- src/tmpfiles/tmpfiles.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index b66765b407..50a0243d98 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -525,7 +525,6 @@ static int dir_cleanup( bool keep_this_level) { struct dirent *dent; - struct timespec times[2]; bool deleted = false; int r = 0; @@ -722,21 +721,22 @@ static int dir_cleanup( finish: if (deleted) { - usec_t age1, age2; char a[FORMAT_TIMESTAMP_MAX], b[FORMAT_TIMESTAMP_MAX]; - - /* Restore original directory timestamps */ - times[0] = ds->st_atim; - times[1] = ds->st_mtim; + usec_t age1, age2; age1 = timespec_load(&ds->st_atim); age2 = timespec_load(&ds->st_mtim); + log_debug("Restoring access and modification time on \"%s\": %s, %s", p, format_timestamp_us(a, sizeof(a), age1), format_timestamp_us(b, sizeof(b), age2)); - if (futimens(dirfd(d), times) < 0) - log_error_errno(errno, "utimensat(%s): %m", p); + + /* Restore original directory timestamps */ + if (futimens(dirfd(d), (struct timespec[]) { + ds->st_atim, + ds->st_mtim }) < 0) + log_warning_errno(errno, "Failed to revert timestamps of '%s', ignoring: %m", p); } return r; From a23fabc6978c85ad128e3c74424b7254928513d6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Jan 2019 15:52:31 +0100 Subject: [PATCH 2/6] tmpfiles: break comment blocks according to our current editor settings --- src/tmpfiles/tmpfiles.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 50a0243d98..4bf148ef3d 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -606,12 +606,10 @@ static int dir_cleanup( r = q; } - /* Note: if you are wondering why we don't - * support the sticky bit for excluding - * directories from cleaning like we do it for - * other file system objects: well, the sticky - * bit already has a meaning for directories, - * so we don't want to overload that. */ + /* Note: if you are wondering why we don't support the sticky bit for excluding + * directories from cleaning like we do it for other file system objects: well, the + * sticky bit already has a meaning for directories, so we don't want to overload + * that. */ if (keep_this_level) { log_debug("Keeping \"%s\".", sub_path); @@ -644,10 +642,8 @@ static int dir_cleanup( r = log_error_errno(errno, "rmdir(%s): %m", sub_path); } else { - /* Skip files for which the sticky bit is - * set. These are semantics we define, and are - * unknown elsewhere. See XDG_RUNTIME_DIR - * specification for details. */ + /* Skip files for which the sticky bit is set. These are semantics we define, and are + * unknown elsewhere. See XDG_RUNTIME_DIR specification for details. */ if (s.st_mode & S_ISVTX) { log_debug("Skipping \"%s\": sticky bit set.", sub_path); continue; @@ -674,8 +670,7 @@ static int dir_cleanup( continue; } - /* Keep files on this level around if this is - * requested */ + /* Keep files on this level around if this is requested */ if (keep_this_level) { log_debug("Keeping \"%s\".", sub_path); continue; From 781bc44e831e8bc114d2e1d979e244ee774df647 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Jan 2019 16:07:45 +0100 Subject: [PATCH 3/6] tmpfiles: make some log messages a tiny bit less cryptic for mere mortals --- src/tmpfiles/tmpfiles.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 4bf148ef3d..c1506710e6 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -583,7 +583,7 @@ static int dir_cleanup( if (mountpoint && streq(dent->d_name, "lost+found") && s.st_uid == 0) { - log_debug("Ignoring \"%s\".", sub_path); + log_debug("Ignoring directory \"%s\".", sub_path); continue; } @@ -596,7 +596,7 @@ static int dir_cleanup( sub_dir = xopendirat_nomod(dirfd(d), dent->d_name); if (!sub_dir) { if (errno != ENOENT) - r = log_error_errno(errno, "opendir(%s) failed: %m", sub_path); + r = log_warning_errno(errno, "Opening directory \"%s\" failed, ignoring: %m", sub_path); continue; } @@ -612,7 +612,7 @@ static int dir_cleanup( * that. */ if (keep_this_level) { - log_debug("Keeping \"%s\".", sub_path); + log_debug("Keeping directory \"%s\".", sub_path); continue; } @@ -639,7 +639,7 @@ static int dir_cleanup( log_debug("Removing directory \"%s\".", sub_path); if (unlinkat(dirfd(d), dent->d_name, AT_REMOVEDIR) < 0) if (!IN_SET(errno, ENOENT, ENOTEMPTY)) - r = log_error_errno(errno, "rmdir(%s): %m", sub_path); + r = log_warning_errno(errno, "Failed to remove directory \"%s\", ignoring: %m", sub_path); } else { /* Skip files for which the sticky bit is set. These are semantics we define, and are @@ -704,11 +704,10 @@ static int dir_cleanup( continue; } - log_debug("unlink \"%s\"", sub_path); - + log_debug("Removing \"%s\".", sub_path); if (unlinkat(dirfd(d), dent->d_name, 0) < 0) if (errno != ENOENT) - r = log_error_errno(errno, "unlink(%s): %m", sub_path); + r = log_warning_errno(errno, "Failed to remove \"%s\", ignoring: %m", sub_path); deleted = true; } From 52b32b2aac96562ae8c380d8869b667f806471e0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Jan 2019 16:09:30 +0100 Subject: [PATCH 4/6] tmpfiles: while aging, take a BSD file lock on each directory we descent into Let's add a fully safe way to exclude certain directories from aging, by taking a BSD file lock on them before aging them. This is useful for clients that untar tarballs into /tmp or /var/tmp, which might have really old timestamps, and to which the aging logic would be very harsh: they can simply take a BSD file lock on any directory they like and thus exclude it from automatic aging, and thus need not to be afraid of untarring stuff below it. Previously, similar functionality was already available through the sticky bit on non-directories, but it's problematic, since as soon as the bit is set no clean-up is done for it at all anymore, forever. Also, it is not suitable for untarring stuff, since the sticky bit after all is a concept denoted in the tarball itself. BSD file locking semantics are much much nicer there, as they are automatically released when the application that has them dies, and they are entirely orthogonal to data encoded in tarballs. This patch takes BSD file locks only on *directories* while descending down the tree, not on regular files. Moreover, it will do so in non-blocking mode only, i.e. if anyone else has a lock the aging for a dir and everything below it is immediately skipped for the current clean-up iteration. Of course applications might take BSD file locks for other reasons than just prevent aging (i.e for their own reasons), but that should be entirely OK, as in that case tmpfiles should step away from those files anyway too: it's a good idea to stay away from any such locked file anyway since it's apparently curretnly being manipulated. This allows us to fix bugs like this: https://github.com/systemd/mkosi/issues/252 --- src/tmpfiles/tmpfiles.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index c1506710e6..04cd0bfa33 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -579,6 +580,7 @@ static int dir_cleanup( } if (S_ISDIR(s.st_mode)) { + _cleanup_closedir_ DIR *sub_dir = NULL; if (mountpoint && streq(dent->d_name, "lost+found") && @@ -590,7 +592,6 @@ static int dir_cleanup( if (maxdepth <= 0) log_warning("Reached max depth on \"%s\".", sub_path); else { - _cleanup_closedir_ DIR *sub_dir; int q; sub_dir = xopendirat_nomod(dirfd(d), dent->d_name); @@ -601,6 +602,11 @@ static int dir_cleanup( continue; } + if (flock(dirfd(sub_dir), LOCK_EX|LOCK_NB) < 0) { + log_debug_errno(errno, "Couldn't acquire shared BSD lock on directory \"%s\", skipping: %m", p); + continue; + } + q = dir_cleanup(i, sub_path, sub_dir, &s, cutoff, rootdev, false, maxdepth-1, false); if (q < 0) r = q; From aa1f2e52e8445c8059c3793dfd779e2296eb5f2a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Jan 2019 16:34:45 +0100 Subject: [PATCH 5/6] man: document the new BSD file lock logic in tmpfiles --- man/tmpfiles.d.xml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/man/tmpfiles.d.xml b/man/tmpfiles.d.xml index 3f2ef7ee97..e5ea2bec8b 100644 --- a/man/tmpfiles.d.xml +++ b/man/tmpfiles.d.xml @@ -545,6 +545,14 @@ w- /proc/sys/vm/swappiness - - - - 10 (ctime). Any of these three (or two) values will prevent cleanup if it is more recent than the current time minus the age field. + + Note that while the aging algorithm is run a 'shared' BSD file lock (see flock2) is + taken on each directory the algorithm descends into (and each directory below that, and so on). If the + aging algorithm finds a lock is already taken on some directory, it (and everything below it) is + skipped. Applications may use this to temporarily exclude certain directory subtrees from the aging + algorithm: the applications can take a BSD file lock themselves, and as long as they keep it aging of + the directory and everything below it is disabled. From e5358401b5df8d395e99815b7a69b8424887472c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 18 Jan 2019 16:34:52 +0100 Subject: [PATCH 6/6] tmpfiles: let's bump RLIMIT_NOFILE for tmpfiles We potentially might descent into quite deep directory trees. Let's hence make sure we can allocate a lot of fds. (This reflects the fact that glibc nftw() and friends have some logic in place to reduce fd usage while descending into directory trees. Doing so is a bit nasty I think, and given that fds are basically free now, if we ask for them, lte's just protect ourselves and make use of that) (No, I am not aware of a real-world case where this was necessary, but let's better be safe than sorry) --- src/tmpfiles/tmpfiles.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/tmpfiles/tmpfiles.c b/src/tmpfiles/tmpfiles.c index 04cd0bfa33..6296a8acdf 100644 --- a/src/tmpfiles/tmpfiles.c +++ b/src/tmpfiles/tmpfiles.c @@ -49,6 +49,7 @@ #include "path-lookup.h" #include "path-util.h" #include "pretty-print.h" +#include "rlimit-util.h" #include "rm-rf.h" #include "selinux-util.h" #include "set.h" @@ -3175,6 +3176,9 @@ static int run(int argc, char *argv[]) { log_setup_service(); + /* Descending down file system trees might take a lot of fds */ + (void) rlimit_nofile_bump(HIGH_RLIMIT_NOFILE); + if (arg_user) { r = user_config_paths(&config_dirs); if (r < 0)