Merge pull request #34719 from poettering/pidref-remote

pidref: add explicit concept of "remote" PidRef

5 files changed, 160 insertions, 48 deletions

diff --git a/src/basic/pidref.c b/src/basic/pidref.c
index 1defe863e2..b13cc96d6a 100644
--- a/src/basic/pidref.c
+++ b/src/basic/pidref.c
@@ -40,6 +40,9 @@ int pidref_acquire_pidfd_id(PidRef *pidref) {
         if (!pidref_is_set(pidref))
                 return -ESRCH;
 
+        if (pidref_is_remote(pidref))
+                return -EREMOTE;
+
         if (pidref->fd < 0)
                 return -ENOMEDIUM;
 
@@ -64,23 +67,36 @@ int pidref_acquire_pidfd_id(PidRef *pidref) {
 
 bool pidref_equal(PidRef *a, PidRef *b) {
 
-        if (pidref_is_set(a)) {
-                if (!pidref_is_set(b))
+        if (!pidref_is_set(a))
+                return !pidref_is_set(b);
+
+        if (!pidref_is_set(b))
+                return false;
+
+        if (a->pid != b->pid)
+                return false;
+
+        if (pidref_is_remote(a)) {
+                /* If one is remote and the other isn't, they are not the same */
+                if (!pidref_is_remote(b))
                         return false;
 
-                if (a->pid != b->pid)
+                /* If both are remote, compare fd IDs if we have both, otherwise don't bother, and cut things short */
+                if (a->fd_id == 0 || b->fd_id == 0)
+                        return true;
+        } else {
+                if (pidref_is_remote(b))
                         return false;
 
-                /* Try to compare pidfds using their inode numbers. This way we can ensure that we don't
-                 * spuriously consider two PidRefs equal if the pid has been reused once. Note that we
-                 * ignore all errors here, not only EOPNOTSUPP, as fstat() might fail due to many reasons. */
+                /* Try to compare pidfds using their inode numbers. This way we can ensure that we
+                 * don't spuriously consider two PidRefs equal if the pid has been reused once. Note
+                 * that we ignore all errors here, not only EOPNOTSUPP, as fstat() might fail due to
+                 * many reasons. */
                 if (pidref_acquire_pidfd_id(a) < 0 || pidref_acquire_pidfd_id(b) < 0)
                         return true;
-
-                return a->fd_id == b->fd_id;
         }
 
-        return !pidref_is_set(b);
+        return a->fd_id == b->fd_id;
 }
 
 int pidref_set_pid(PidRef *pidref, pid_t pid) {
@@ -240,7 +256,9 @@ int pidref_copy(const PidRef *pidref, PidRef *dest) {
         assert(dest);
 
         if (pidref) {
-                if (pidref->fd >= 0) {
+                if (pidref_is_remote(pidref)) /* Propagate remote flag */
+                        dup_fd = -EREMOTE;
+                else if (pidref->fd >= 0) {
                         dup_fd = fcntl(pidref->fd, F_DUPFD_CLOEXEC, 3);
                         if (dup_fd < 0) {
                                 if (!ERRNO_IS_RESOURCE(errno))
@@ -311,6 +329,9 @@ int pidref_kill(const PidRef *pidref, int sig) {
         if (!pidref)
                 return -ESRCH;
 
+        if (pidref_is_remote(pidref))
+                return -EREMOTE;
+
         if (pidref->fd >= 0)
                 return RET_NERRNO(pidfd_send_signal(pidref->fd, sig, NULL, 0));
 
@@ -338,6 +359,9 @@ int pidref_sigqueue(const PidRef *pidref, int sig, int value) {
         if (!pidref)
                 return -ESRCH;
 
+        if (pidref_is_remote(pidref))
+                return -EREMOTE;
+
         if (pidref->fd >= 0) {
                 siginfo_t si;
 
@@ -370,6 +394,9 @@ int pidref_verify(const PidRef *pidref) {
         if (!pidref_is_set(pidref))
                 return -ESRCH;
 
+        if (pidref_is_remote(pidref))
+                return -EREMOTE;
+
         if (pidref->pid == 1)
                 return 1; /* PID 1 can never go away, hence never be recycled to a different process → return 1 */
 
@@ -387,6 +414,9 @@ bool pidref_is_self(const PidRef *pidref) {
         if (!pidref)
                 return false;
 
+        if (pidref_is_remote(pidref))
+                return false;
+
         return pidref->pid == getpid_cached();
 }
 
@@ -396,6 +426,9 @@ int pidref_wait(const PidRef *pidref, siginfo_t *ret, int options) {
         if (!pidref_is_set(pidref))
                 return -ESRCH;
 
+        if (pidref_is_remote(pidref))
+                return -EREMOTE;
+
         if (pidref->pid == 1 || pidref->pid == getpid_cached())
                 return -ECHILD;
 
diff --git a/src/basic/pidref.h b/src/basic/pidref.h
index 8647f4bd23..494bc9d8f1 100644
--- a/src/basic/pidref.h
+++ b/src/basic/pidref.h
@@ -6,10 +6,35 @@ typedef struct PidRef PidRef;
 #include "macro.h"
 #include "process-util.h"
 
-/* An embeddable structure carrying a reference to a process. Supposed to be used when tracking processes continuously. */
+/* An embeddable structure carrying a reference to a process. Supposed to be used when tracking processes
+ * continuously. This combines a PID, a modern Linux pidfd and the 64bit inode number of the pidfd into one
+ * structure. Note that depending on kernel support the pidfd might not be initialized, and if it is
+ * initialized then fd_id might still not be initialized (because the concept was added to the kernel much
+ * later than pidfds themselves).
+ *
+ * There are three special states a PidRef can be in:
+ *
+ * 1. It can be *unset*. Use pidref_is_set() to detect this case. Most operations attempted on such a PidRef
+ *    will fail with -ESRCH. Use PIDREF_NULL for initializing a PidRef in this state.
+ *
+ * 2. It can be marked as *automatic*. This is a special state indicating that a process reference is
+ *    supposed to be derived automatically from the current context. This is used by the Varlink/JSON
+ *    dispatcher as indication that a PidRef shall be derived from the connection peer, but might be
+ *    otherwise used too. When marked *automatic* the PidRef will also be considered *unset*, hence most
+ *    operations will fail with -ESRCH, as above.
+ *
+ * 3. It can be marked as *remote*. This is useful when deserializing a PidRef structure from an IPC message
+ *    or similar, and it has been determined that the given PID definitely doesn't refer to a local
+ *    process. In this case the PidRef logic will refrain from trying to acquire a pidfd for the
+ *    process. Moreover, most operations will fail with -EREMOTE. Only PidRef structures that are not marked
+ *    *unset* can be marked *remote*.
+ */
 struct PidRef {
-        pid_t pid;      /* always valid */
-        int fd;         /* only valid if pidfd are available in the kernel, and we manage to get an fd */
+        pid_t pid;      /* > 0 if the PidRef is set, otherwise set to PID_AUTOMATIC if automatic mode is
+                         * desired, or 0 otherwise. */
+        int fd;         /* only valid if pidfd are available in the kernel, and we manage to get an fd. If we
+                         * know that the PID is not from the local machine we set this to -EREMOTE, otherwise
+                         * we use -EBADF as indicator the fd is invalid. */
         uint64_t fd_id; /* the inode number of pidfd. only useful in kernel 6.9+ where pidfds live in
                            their own pidfs and each process comes with a unique inode number */
 };
@@ -31,6 +56,12 @@ static inline bool pidref_is_set(const PidRef *pidref) {
 
 bool pidref_is_automatic(const PidRef *pidref);
 
+static inline bool pidref_is_remote(const PidRef *pidref) {
+        /* If the fd is set to -EREMOTE we assume PidRef does not refer to a local PID, but on another
+         * machine (and we just got the PidRef initialized due to deserialization of some RPC message) */
+        return pidref_is_set(pidref) && pidref->fd == -EREMOTE;
+}
+
 int pidref_acquire_pidfd_id(PidRef *pidref);
 bool pidref_equal(PidRef *a, PidRef *b);
 
diff --git a/src/libsystemd/sd-json/json-util.c b/src/libsystemd/sd-json/json-util.c
index 4c978850c4..d42c61a69c 100644
--- a/src/libsystemd/sd-json/json-util.c
+++ b/src/libsystemd/sd-json/json-util.c
@@ -151,7 +151,7 @@ int json_dispatch_path(const char *name, sd_json_variant *variant, sd_json_dispa
 }
 
 int json_variant_new_pidref(sd_json_variant **ret, PidRef *pidref) {
-        sd_id128_t boot_id;
+        sd_id128_t boot_id = SD_ID128_NULL;
         int r;
 
         /* Turns a PidRef into a triplet of PID, pidfd inode nr, and the boot ID. The triplet should uniquely
@@ -160,22 +160,24 @@ int json_variant_new_pidref(sd_json_variant **ret, PidRef *pidref) {
         if (!pidref_is_set(pidref))
                 return sd_json_variant_new_null(ret);
 
-        r = pidref_acquire_pidfd_id(pidref);
-        if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r) && r != -ENOMEDIUM)
-                return r;
+        if (!pidref_is_remote(pidref)) {
+                r = pidref_acquire_pidfd_id(pidref);
+                if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r) && r != -ENOMEDIUM)
+                        return r;
 
-        if (pidref->fd_id > 0) {
                 /* If we have the pidfd inode number, also acquire the boot ID, to make things universally unique */
-                r = sd_id128_get_boot(&boot_id);
-                if (r < 0)
-                        return r;
+                if (pidref->fd_id > 0) {
+                        r = sd_id128_get_boot(&boot_id);
+                        if (r < 0)
+                                return r;
+                }
         }
 
         return sd_json_buildo(
                         ret,
                         SD_JSON_BUILD_PAIR_INTEGER("pid", pidref->pid),
                         SD_JSON_BUILD_PAIR_CONDITION(pidref->fd_id > 0, "pidfdId", SD_JSON_BUILD_INTEGER(pidref->fd_id)),
-                        SD_JSON_BUILD_PAIR_CONDITION(pidref->fd_id > 0, "bootId", SD_JSON_BUILD_ID128(boot_id)));
+                        SD_JSON_BUILD_PAIR_CONDITION(!sd_id128_is_null(boot_id), "bootId", SD_JSON_BUILD_ID128(boot_id)));
 }
 
 int json_dispatch_pidref(const char *name, sd_json_variant *variant, sd_json_dispatch_flags_t flags, void *userdata) {
@@ -252,7 +254,7 @@ int json_dispatch_pidref(const char *name, sd_json_variant *variant, sd_json_dis
                 } else {
                         local_boot_id = sd_id128_equal(data.boot_id, my_boot_id);
                         if (!local_boot_id) {
-                                json_log(variant, flags | (FLAGS_SET(flags, SD_JSON_STRICT) ? 0 : SD_JSON_DEBUG), 0, "JSON field '%s' refers to non-local PID.", strna(name));
+                                json_log(variant, flags | (FLAGS_SET(flags, SD_JSON_STRICT) ? 0 : SD_JSON_DEBUG), 0, "JSON field '%s' refers to non-local PID%s.", strna(name), FLAGS_SET(flags, SD_JSON_STRICT) ? "" : ", proceeding");
                                 if (FLAGS_SET(flags, SD_JSON_STRICT))
                                         return -ESRCH;
                         }
@@ -260,35 +262,41 @@ int json_dispatch_pidref(const char *name, sd_json_variant *variant, sd_json_dis
         }
 
         _cleanup_(pidref_done) PidRef np = PIDREF_NULL;
-        if (local_boot_id != 0) {
-                /* Try to acquire a pidfd – unless this is definitely not a local PID */
+        if (local_boot_id == 0)
+                /* If this is definitely not the local boot ID, then mark the PidRef as remote in the sense of pidref_is_remote() */
+                np = (PidRef) {
+                        .pid = data.pid,
+                        .fd = -EREMOTE,
+                        .fd_id = data.fd_id,
+                };
+        else {
+                /* Try to acquire a pidfd if this is or might be a local PID */
                 r = pidref_set_pid(&np, data.pid);
                 if (r < 0) {
                         json_log(variant, flags | (FLAGS_SET(flags, SD_JSON_STRICT) ? 0 : SD_JSON_DEBUG), r, "Unable to get fd for PID in JSON field '%s': %m", strna(name));
                         if (FLAGS_SET(flags, SD_JSON_STRICT))
                                 return r;
-                }
-        }
-
-        /* If the the PID is dead or we otherwise can't get a pidfd of it, then store at least the PID number */
-        if (!pidref_is_set(&np))
-                np = PIDREF_MAKE_FROM_PID(data.pid);
 
-        /* If the pidfd inode nr is specified, validate it or at least state */
-        if (data.fd_id > 0) {
-                if (np.fd >= 0) {
-                        r = pidref_acquire_pidfd_id(&np);
-                        if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r))
-                                return json_log(variant, flags, r, "Unable to get pidfd ID to validate JSON field '%s': %m", strna(name));
+                        /* If the PID is dead or we otherwise can't get a pidfd of it, then store at least the PID number */
+                        np = PIDREF_MAKE_FROM_PID(data.pid);
+                }
 
-                        if (data.fd_id != np.fd_id) {
-                                json_log(variant, flags | (FLAGS_SET(flags, SD_JSON_STRICT) ? 0 : SD_JSON_DEBUG), 0, "JSON field '%s' references PID with non-matching inode number.", strna(name));
-                                if (FLAGS_SET(flags, SD_JSON_STRICT))
-                                        return -ESRCH;
+                /* If the pidfd inode nr is specified, validate it or at least state */
+                if (data.fd_id > 0) {
+                        if (np.fd >= 0) {
+                                r = pidref_acquire_pidfd_id(&np);
+                                if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r))
+                                        return json_log(variant, flags, r, "Unable to get pidfd ID to validate JSON field '%s': %m", strna(name));
+
+                                if (data.fd_id != np.fd_id) {
+                                        json_log(variant, flags | (FLAGS_SET(flags, SD_JSON_STRICT) ? 0 : SD_JSON_DEBUG), 0, "JSON field '%s' references PID with non-matching inode number.", strna(name));
+                                        if (FLAGS_SET(flags, SD_JSON_STRICT))
+                                                return -ESRCH;
+                                }
+                        } else {
+                                json_log(variant, flags|SD_JSON_DEBUG, 0, "Not validating PID inode number on JSON field '%s', because operating without pidfd.", strna(name));
+                                np.fd_id = data.fd_id;
                         }
-                } else if (local_boot_id != 0) {
-                        json_log(variant, flags|SD_JSON_DEBUG, 0, "Not validating PID inode number on JSON field '%s', because operating without pidfd.", strna(name));
-                        np.fd_id = data.fd_id;
                 }
         }
 
diff --git a/src/test/test-json.c b/src/test/test-json.c
index 75dff429d5..c4df19e08d 100644
--- a/src/test/test-json.c
+++ b/src/test/test-json.c
@@ -1272,24 +1272,34 @@ TEST(pidref) {
         assert_se(pidref_set_pid(&pid1, 1) >= 0);
 
         _cleanup_(sd_json_variant_unrefp) sd_json_variant *v = NULL;
+        sd_id128_t randomized_boot_id;
+        assert_se(sd_id128_randomize(&randomized_boot_id) >= 0);
         assert_se(sd_json_buildo(&v,
                                  JSON_BUILD_PAIR_PIDREF("myself", &myself),
-                                 JSON_BUILD_PAIR_PIDREF("pid1", &pid1)) >= 0);
+                                 JSON_BUILD_PAIR_PIDREF("pid1", &pid1),
+                                 SD_JSON_BUILD_PAIR("remote", SD_JSON_BUILD_OBJECT(SD_JSON_BUILD_PAIR_UNSIGNED("pid", 1),
+                                                                                   SD_JSON_BUILD_PAIR_UNSIGNED("pidfdId", 4711),
+                                                                                   SD_JSON_BUILD_PAIR_ID128("bootId", randomized_boot_id))),
+                                 SD_JSON_BUILD_PAIR("automatic", SD_JSON_BUILD_OBJECT(SD_JSON_BUILD_PAIR_UNSIGNED("pid", 0)))) >= 0);
 
         sd_json_variant_dump(v, SD_JSON_FORMAT_COLOR|SD_JSON_FORMAT_PRETTY, NULL, NULL);
 
         struct {
-                PidRef myself, pid1;
+                PidRef myself, pid1, remote, automatic;
         } data = {
                 .myself = PIDREF_NULL,
                 .pid1 = PIDREF_NULL,
+                .remote = PIDREF_NULL,
+                .automatic = PIDREF_NULL,
         };
 
         assert_se(sd_json_dispatch(
                                   v,
                                   (const sd_json_dispatch_field[]) {
-                                          { "myself", _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, voffsetof(data, myself), 0 },
-                                          { "pid1", _SD_JSON_VARIANT_TYPE_INVALID,   json_dispatch_pidref, voffsetof(data, pid1),   0 },
+                                          { "myself",    _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, voffsetof(data, myself),    SD_JSON_STRICT },
+                                          { "pid1",      _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, voffsetof(data, pid1),      SD_JSON_STRICT },
+                                          { "remote",    _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, voffsetof(data, remote),    0              },
+                                          { "automatic", _SD_JSON_VARIANT_TYPE_INVALID, json_dispatch_pidref, voffsetof(data, automatic), SD_JSON_RELAX  },
                                           {},
                                   },
                                   /* flags= */ 0,
@@ -1300,12 +1310,20 @@ TEST(pidref) {
 
         assert_se(!pidref_equal(&myself, &data.pid1));
         assert_se(!pidref_equal(&pid1, &data.myself));
+        assert_se(!pidref_equal(&myself, &data.remote));
+        assert_se(!pidref_equal(&pid1, &data.remote));
 
         assert_se((myself.fd_id > 0) == (data.myself.fd_id > 0));
         assert_se((pid1.fd_id > 0) == (data.pid1.fd_id > 0));
 
+        assert_se(!pidref_is_set(&data.automatic));
+        assert_se(pidref_is_automatic(&data.automatic));
+        assert_se(pidref_is_set(&data.remote));
+        assert_se(pidref_is_remote(&data.remote));
+
         pidref_done(&data.myself);
         pidref_done(&data.pid1);
+        pidref_done(&data.remote);
 }
 
 TEST(devnum) {
diff --git a/src/test/test-pidref.c b/src/test/test-pidref.c
index 7298b36596..5535e98ab0 100644
--- a/src/test/test-pidref.c
+++ b/src/test/test-pidref.c
@@ -240,4 +240,26 @@ TEST(pidref_is_automatic) {
         assert_se(!pid_is_valid(PID_AUTOMATIC));
 }
 
+TEST(pidref_is_remote) {
+        assert_se(!pidref_is_remote(NULL));
+        assert_se(!pidref_is_remote(&PIDREF_NULL));
+        assert_se(!pidref_is_remote(&PIDREF_MAKE_FROM_PID(1)));
+        assert_se(!pidref_is_remote(&PIDREF_MAKE_FROM_PID(getpid_cached())));
+        assert_se(!pidref_is_remote(&PIDREF_AUTOMATIC));
+
+        static const PidRef p = {
+                .pid = 1,
+                .fd = -EREMOTE,
+                .fd_id = 4711,
+        };
+
+        assert_se(pidref_is_set(&p));
+        assert_se(pidref_is_remote(&p));
+        assert_se(!pidref_is_automatic(&p));
+        assert_se(pidref_kill(&p, SIGTERM) == -EREMOTE);
+        assert_se(pidref_kill_and_sigcont(&p, SIGTERM) == -EREMOTE);
+        assert_se(pidref_wait_for_terminate(&p, /* ret= */ NULL) == -EREMOTE);
+        assert_se(pidref_verify(&p) == -EREMOTE);
+}
+
 DEFINE_TEST_MAIN(LOG_DEBUG);