From: Lennart Poettering Date: Fri, 11 Oct 2024 09:33:42 +0000 (+0200) Subject: pidref: add explicit concept of "remote" PidRef X-Git-Tag: v257-rc1~213^2~1 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=7e3e540b88db5546d0c63103619d96b033871b7b;p=thirdparty%2Fsystemd.git pidref: add explicit concept of "remote" PidRef This PidRef just track some data, but cannot be used for any active operation. Background: for https://github.com/systemd/systemd/pull/34703 it makes sense to track explicitly if some PidRef is not a local one, so that we never attempt to for example "kill a remote process" and thus acccidentally hit the wrong process (i.e. a local one by the same PID). --- diff --git a/src/basic/pidref.c b/src/basic/pidref.c index 1defe863e2b..b13cc96d6a1 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 8647f4bd23b..494bc9d8f19 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/test/test-pidref.c b/src/test/test-pidref.c index 7298b365968..5535e98ab07 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);