core: upgrade /tmp when PrivateTmp=yes/DefaultDeps=no to disconnected

In https://github.com/systemd/systemd/issues/28515, multiple people report that
services that have PrivateTmp=yes and DefaultDependencies=no fail to create the
temporary directories under /tmp, when /tmp is e.g. a bind mount or some other
kind of mount that takes more time.

Before PrivateTmp=disconnected was added, we didn't have a nice solution:
DefaultDependencies=no is used to start services very early, so we wouldn't
want to add a dependency on /tmp automatically. With PrivateTmp=disconnected we
have a fairly nice solution. Let's "upgrade" to this mode automatically.
Strictly speaking, it is a small compat break, but in practice it's unlikely to
matter for early-boot services whether their /tmp is private or disconnected.

The dependency on /tmp that is checked is After. I think this is enough,
since any tmp.mount would be pulled in by local-fs.target and the rest of
the transaction anyway, so we don't need to check more than After.

The asserts are relaxed, because now the two settings can now diverge
in either way.

Resolves https://github.com/systemd/systemd/issues/28515.

[yhndnzj: fix unit_add_exec_dependencies() to handle the new
          combination; add a comment in exec_needs_sys_admin()]

diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml
index c89adad66a4aa3dd896f47c66422591edb83c3f7..3f06f564b33c813cd7dee147dfe07bc6401c3a7a 100644 (file)
--- a/man/systemd.exec.xml
+++ b/man/systemd.exec.xml
@@ -63,10 +63,15 @@
 
       <listitem><para>Units with <varname>PrivateTmp=yes</varname> automatically gain dependencies of type
       <varname>Wants=</varname> and <varname>After=</varname> on all mounts required to access
-      <filename>/tmp/</filename> and <filename>/var/tmp/</filename>. They will also gain an automatic
+      <filename>/tmp/</filename> and <filename>/var/tmp/</filename> and an automatic
       <varname>After=</varname> dependency on
-      <citerefentry><refentrytitle>systemd-tmpfiles-setup.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>.
-      </para></listitem>
+      <citerefentry><refentrytitle>systemd-tmpfiles-setup.service</refentrytitle><manvolnum>8</manvolnum></citerefentry>,
+      unless <varname>DefaultDependencies=no</varname> is specified. If
+      <varname>DefaultDependencies=no</varname> is specified, and a
+      <varname>RequiresMountsFor=/tmp/</varname>, <varname>WantsMountsFor=/tmp/</varname>,
+      <varname>After=tmp.mount</varname>, or <varname>RootDirectory=</varname>/<varname>RootImage=</varname>
+      are not specified, <varname>PrivateTmp=yes</varname> is converted to
+      <varname>PrivateTmp=disconnected</varname>.</para></listitem>
 
       <listitem><para>Units with <varname>PrivateTmp=disconnected</varname> automatically gain dependencies
       of type <varname>Wants=</varname> and <varname>After=</varname> on the mount required to access

diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c
index 10a7b550066ec77282668241a7f1c504dc468da3..25c12ece258fb658114ed27c5e6c7f5e5f6a0d5b 100644 (file)
--- a/src/core/exec-invoke.c
+++ b/src/core/exec-invoke.c
@@ -2248,10 +2248,8 @@ static int build_environment(
         }
 
         assert(c->private_var_tmp >= 0 && c->private_var_tmp < _PRIVATE_TMP_MAX);
-        if (needs_sandboxing && c->private_tmp != c->private_var_tmp) {
-                assert(c->private_tmp == PRIVATE_TMP_DISCONNECTED);
-                assert(c->private_var_tmp == PRIVATE_TMP_NO);
-
+        if (needs_sandboxing &&
+            c->private_var_tmp == PRIVATE_TMP_NO && c->private_tmp != PRIVATE_TMP_NO) {
                 /* When private tmpfs is enabled only on /tmp/, then explicitly set $TMPDIR to suggest the
                  * service to use /tmp/. */
 
@@ -3907,14 +3905,16 @@ static int apply_mount_namespace(
         if (needs_sandboxing) {
                 /* The runtime struct only contains the parent of the private /tmp, which is non-accessible
                  * to world users. Inside of it there's a /tmp that is sticky, and that's the one we want to
-                 * use here.  This does not apply when we are using /run/systemd/empty as fallback. */
+                 * use here. This does not apply when we are using /run/systemd/empty as fallback. */
 
                 if (context->private_tmp == PRIVATE_TMP_CONNECTED && runtime->shared) {
                         if (streq_ptr(runtime->shared->tmp_dir, RUN_SYSTEMD_EMPTY))
                                 tmp_dir = runtime->shared->tmp_dir;
                         else if (runtime->shared->tmp_dir)
                                 tmp_dir = strjoina(runtime->shared->tmp_dir, "/tmp");
+                }
 
+                if (context->private_var_tmp == PRIVATE_TMP_CONNECTED && runtime->shared) {
                         if (streq_ptr(runtime->shared->var_tmp_dir, RUN_SYSTEMD_EMPTY))
                                 var_tmp_dir = runtime->shared->var_tmp_dir;
                         else if (runtime->shared->var_tmp_dir)
@@ -4624,7 +4624,7 @@ static bool exec_needs_cap_sys_admin(const ExecContext *context, const ExecParam
                 return false;
 
         return context->private_users != PRIVATE_USERS_NO ||
-               context->private_tmp != PRIVATE_TMP_NO ||
+               context->private_tmp != PRIVATE_TMP_NO || /* no need to check for private_var_tmp here, private_tmp is never demoted to "no" */
                context->private_devices ||
                context->private_network ||
                context->user_namespace_path ||

diff --git a/src/core/execute.h b/src/core/execute.h
index 094b0a3a448d49453c59d453855c5727fee3f83e..3d5e92cccf0fefbdce856f73567331ddba5628b3 100644 (file)
--- a/src/core/execute.h
+++ b/src/core/execute.h
@@ -333,9 +333,9 @@ typedef struct ExecContext {
         int bind_log_sockets;
         int memory_ksm;
         MemoryTHP memory_thp;
-        PrivateTmp private_tmp;
-        PrivateTmp private_var_tmp; /* This is not an independent parameter, but calculated from other
-                                     * parameters in unit_patch_contexts(). */
+        PrivateTmp private_tmp;     /* Those are not independent parameters, but are calculated from */
+        PrivateTmp private_var_tmp; /* other parameters in unit_patch_contexts(). */
+
         bool private_network;
         bool private_devices;
         PrivateUsers private_users;

diff --git a/src/core/namespace.c b/src/core/namespace.c
index f899fa241c0ce69629a725357d112392e18600d4..54504564aba9959a56cf6476530b59c466ba3597 100644 (file)
--- a/src/core/namespace.c
+++ b/src/core/namespace.c
@@ -756,8 +756,8 @@ static int append_private_tmp(MountList *ml, const NamespaceParameters *p) {
 
         assert(ml);
         assert(p);
-        assert(p->private_tmp == p->private_var_tmp ||
-               (p->private_tmp == PRIVATE_TMP_DISCONNECTED && p->private_var_tmp == PRIVATE_TMP_NO));
+        assert(p->private_tmp >= 0 && p->private_tmp < _PRIVATE_TMP_MAX);
+        assert(p->private_var_tmp >= 0 && p->private_var_tmp < _PRIVATE_TMP_MAX);
 
         if (p->tmp_dir) {
                 assert(p->private_tmp == PRIVATE_TMP_CONNECTED);

diff --git a/src/core/unit.c b/src/core/unit.c
index bf4542c08074c0446c599f3a98ca5f8e33c9a5a4..dc158fb335b8492e04ffb4ea965d565ea97a0f0d 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1310,20 +1310,15 @@ int unit_add_exec_dependencies(Unit *u, ExecContext *c) {
         assert(c->private_var_tmp >= 0 && c->private_var_tmp < _PRIVATE_TMP_MAX);
 
         if (c->private_tmp == PRIVATE_TMP_CONNECTED) {
-                assert(c->private_var_tmp == PRIVATE_TMP_CONNECTED);
-
                 r = unit_add_mounts_for(u, "/tmp/", UNIT_DEPENDENCY_FILE, UNIT_MOUNT_WANTS);
                 if (r < 0)
                         return r;
+        }
 
+        if (c->private_var_tmp == PRIVATE_TMP_CONNECTED) {
                 r = unit_add_mounts_for(u, "/var/tmp/", UNIT_DEPENDENCY_FILE, UNIT_MOUNT_WANTS);
                 if (r < 0)
                         return r;
-
-                r = unit_add_dependency_by_name(u, UNIT_AFTER, SPECIAL_TMPFILES_SETUP_SERVICE, true, UNIT_DEPENDENCY_FILE);
-                if (r < 0)
-                        return r;
-
         } else if (c->private_var_tmp == PRIVATE_TMP_DISCONNECTED && !exec_context_with_rootfs(c)) {
                 /* Even if PrivateTmp=disconnected, we still require /var/tmp/ mountpoint to be present,
                  * i.e. /var/ needs to be mounted. See comments in unit_patch_contexts(). */
@@ -1332,6 +1327,12 @@ int unit_add_exec_dependencies(Unit *u, ExecContext *c) {
                         return r;
         }
 
+        if (c->private_tmp == PRIVATE_TMP_CONNECTED || c->private_var_tmp == PRIVATE_TMP_CONNECTED) {
+                r = unit_add_dependency_by_name(u, UNIT_AFTER, SPECIAL_TMPFILES_SETUP_SERVICE, true, UNIT_DEPENDENCY_FILE);
+                if (r < 0)
+                        return r;
+        }
+
         if (c->root_image || c->root_mstack) {
                 /* We need to wait for /dev/loopX to appear when doing RootImage=, hence let's add an
                  * implicit dependency on udev. (And for RootMStack= we might need it) */
@@ -4349,8 +4350,8 @@ static PrivateTmp unit_get_private_var_tmp(const Unit *u, const ExecContext *c)
         assert(c->private_tmp >= 0 && c->private_tmp < _PRIVATE_TMP_MAX);
 
         /* Disable disconnected private tmpfs on /var/tmp/ when DefaultDependencies=no and
-         * RootImage=/RootDirectory= are not set, as /var/ may be a separated partition.
-         * See issue #37258. */
+         * RootImage=/RootDirectory= are not set, as /var/ may be a separate partition.
+         * See https://github.com/systemd/systemd/issues/37258. */
 
         /* PrivateTmp=yes/no also enables/disables private tmpfs on /var/tmp/. */
         if (c->private_tmp != PRIVATE_TMP_DISCONNECTED)
@@ -4388,6 +4389,42 @@ static PrivateTmp unit_get_private_var_tmp(const Unit *u, const ExecContext *c)
         return PRIVATE_TMP_NO;
 }
 
+static PrivateTmp unit_get_private_tmp(const Unit *u, const ExecContext *c) {
+        assert(u);
+        assert(c);
+        assert(c->private_tmp >= 0 && c->private_tmp < _PRIVATE_TMP_MAX);
+
+        /* Upgrade "PrivateTmp=yes" (a.k.a. 'connected') to 'disconnected' when
+         * DefaultDependencies=no and RootImage=/RootDirectory= are not set, as /tmp/ may be a
+         * separate partition. See https://github.com/systemd/systemd/issues/28515.
+         *
+         * Note that the change goes in the opposite direction than unit_get_private_var_tmp()
+         * above. For /var/tmp/, we need to disable the setting, because we don't want to create
+         * the /var/tmp/ directory if /var/ is a mount point. We don't have this problem with
+         * /tmp/ because there is no nesting. */
+
+        if (c->private_tmp != PRIVATE_TMP_CONNECTED ||
+            u->default_dependencies ||
+            exec_context_with_rootfs(c))
+                return c->private_tmp;
+
+        /* Even if DefaultDependencies=no, honour tmpfs setting when
+         * RequiresMountsFor=/WantsMountsFor=/tmp/ is explicitly set. */
+        for (UnitMountDependencyType t = 0; t < _UNIT_MOUNT_DEPENDENCY_TYPE_MAX; t++)
+                if (hashmap_contains(u->mounts_for[t], "/tmp/"))
+                        return c->private_tmp;
+
+        /* Check the same but for After=. */
+        Unit *m = manager_get_unit(u->manager, "tmp.mount");
+        if (!m)
+                return c->private_tmp;
+
+        if (unit_has_dependency(u, UNIT_ATOM_AFTER, m))
+                return c->private_tmp;
+
+        return PRIVATE_TMP_DISCONNECTED;
+}
+
 int unit_patch_contexts(Unit *u) {
         CGroupContext *cc;
         ExecContext *ec;
@@ -4464,7 +4501,14 @@ int unit_patch_contexts(Unit *u) {
                         ec->restrict_suid_sgid = true;
                 }
 
+                /* Table of possible combinations:
+                 *                           /var/tmp          /tmp
+                 * PrivateTmp=no             no                no
+                 * PrivateTmp=connected      connected         connected,disconnected
+                 * PrivateTmp=disconnected   disconnected,no   disconnected
+                 */
                 ec->private_var_tmp = unit_get_private_var_tmp(u, ec);
+                ec->private_tmp = unit_get_private_tmp(u, ec);
 
                 FOREACH_ARRAY(d, ec->directories, _EXEC_DIRECTORY_TYPE_MAX)
                         exec_directory_sort(d);