From 1b6239632dd9697c435d5862a9e2417ee990c6c0 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Wed, 22 May 2024 19:27:36 +0800 Subject: [PATCH 1/2] pidref: introduce pidfd_inode_ids_supported helper Also, correct the comment about pidfs (added in kernel 6.9 rather than 6.8). Co-authored-by: Lennart Poettering --- src/basic/pidref.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/basic/pidref.c b/src/basic/pidref.c index 69a010210dd..11abadff826 100644 --- a/src/basic/pidref.c +++ b/src/basic/pidref.c @@ -6,6 +6,7 @@ #include "errno-util.h" #include "fd-util.h" +#include "missing_magic.h" #include "missing_syscall.h" #include "missing_wait.h" #include "parse-util.h" @@ -14,6 +15,23 @@ #include "signal-util.h" #include "stat-util.h" +static int pidfd_inode_ids_supported(void) { + static int cached = -1; + + if (cached >= 0) + return cached; + + _cleanup_close_ int fd = pidfd_open(getpid_cached(), 0); + if (fd < 0) { + if (ERRNO_IS_NOT_SUPPORTED(errno)) + return (cached = false); + + return -errno; + } + + return (cached = fd_is_fs_type(fd, PID_FS_MAGIC)); +} + bool pidref_equal(const PidRef *a, const PidRef *b) { int r; @@ -28,8 +46,11 @@ bool pidref_equal(const PidRef *a, const PidRef *b) { return true; /* pidfds live in their own pidfs and each process comes with a unique inode number since - * kernel 6.8. We can safely do this on older kernels too though, as previously anonymous - * inode was used and inode number was the same for all pidfds. */ + * kernel 6.9. */ + + if (pidfd_inode_ids_supported() <= 0) + return true; + r = fd_inode_same(a->fd, b->fd); if (r < 0) log_debug_errno(r, "Failed to check whether pidfds for pid " PID_FMT " are equal, assuming yes: %m", From 15930d5d9fc674eebbf3e82326ffa0e71bcff4df Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Thu, 23 May 2024 22:19:05 +0800 Subject: [PATCH 2/2] pidref: record pidfd inode number in PidRef struct Besides internal comparisons, the inode number of pidfds might be interesting directly to users, too. In the future this field should also be exposed, so that it can serve as a unique identifier of a process (but only for display, as there's no method to map this back to a pid or pidfd). --- src/basic/pidref.c | 48 ++++++++++++++++++++++++++++++------------ src/basic/pidref.h | 9 +++++--- src/test/test-pidref.c | 12 ++++++----- 3 files changed, 48 insertions(+), 21 deletions(-) diff --git a/src/basic/pidref.c b/src/basic/pidref.c index 11abadff826..d9380a84ab4 100644 --- a/src/basic/pidref.c +++ b/src/basic/pidref.c @@ -32,9 +32,38 @@ static int pidfd_inode_ids_supported(void) { return (cached = fd_is_fs_type(fd, PID_FS_MAGIC)); } -bool pidref_equal(const PidRef *a, const PidRef *b) { +int pidref_acquire_pidfd_id(PidRef *pidref) { int r; + assert(pidref); + + if (!pidref_is_set(pidref)) + return -ESRCH; + + if (pidref->fd < 0) + return -ENOMEDIUM; + + if (pidref->fd_id > 0) + return 0; + + r = pidfd_inode_ids_supported(); + if (r < 0) + return r; + if (r == 0) + return -EOPNOTSUPP; + + struct stat st; + + if (fstat(pidref->fd, &st) < 0) + return log_debug_errno(errno, "Failed to get inode number of pidfd for pid " PID_FMT ": %m", + pidref->pid); + + pidref->fd_id = (uint64_t) st.st_ino; + return 0; +} + +bool pidref_equal(PidRef *a, PidRef *b) { + if (pidref_is_set(a)) { if (!pidref_is_set(b)) return false; @@ -42,20 +71,13 @@ bool pidref_equal(const PidRef *a, const PidRef *b) { if (a->pid != b->pid) return false; - if (a->fd < 0 || b->fd < 0) + /* 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; - /* pidfds live in their own pidfs and each process comes with a unique inode number since - * kernel 6.9. */ - - if (pidfd_inode_ids_supported() <= 0) - return true; - - r = fd_inode_same(a->fd, b->fd); - if (r < 0) - log_debug_errno(r, "Failed to check whether pidfds for pid " PID_FMT " are equal, assuming yes: %m", - a->pid); - return r != 0; + return a->fd_id == b->fd_id; } return !pidref_is_set(b); diff --git a/src/basic/pidref.h b/src/basic/pidref.h index 9920ebb9b3b..0581f1af1e5 100644 --- a/src/basic/pidref.h +++ b/src/basic/pidref.h @@ -5,8 +5,10 @@ /* An embeddable structure carrying a reference to a process. Supposed to be used when tracking processes continuously. */ typedef 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; /* always valid */ + int fd; /* only valid if pidfd are available in the kernel, and we manage to get an fd */ + 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 */ } PidRef; #define PIDREF_NULL (const PidRef) { .fd = -EBADF } @@ -19,7 +21,8 @@ static inline bool pidref_is_set(const PidRef *pidref) { return pidref && pidref->pid > 0; } -bool pidref_equal(const PidRef *a, const PidRef *b); +int pidref_acquire_pidfd_id(PidRef *pidref); +bool pidref_equal(PidRef *a, PidRef *b); /* This turns a pid_t into a PidRef structure, and acquires a pidfd for it, if possible. (As opposed to * PIDREF_MAKE_FROM_PID() above, which does not acquire a pidfd.) */ diff --git a/src/test/test-pidref.c b/src/test/test-pidref.c index 2c4d894e77f..53ed10d1537 100644 --- a/src/test/test-pidref.c +++ b/src/test/test-pidref.c @@ -7,6 +7,8 @@ #include "stdio-util.h" #include "tests.h" +#define PIDREF_NULL_NONCONST (PidRef) { .fd = -EBADF } + TEST(pidref_is_set) { assert_se(!pidref_is_set(NULL)); assert_se(!pidref_is_set(&PIDREF_NULL)); @@ -15,14 +17,14 @@ TEST(pidref_is_set) { TEST(pidref_equal) { assert_se(pidref_equal(NULL, NULL)); - assert_se(pidref_equal(NULL, &PIDREF_NULL)); - assert_se(pidref_equal(&PIDREF_NULL, NULL)); - assert_se(pidref_equal(&PIDREF_NULL, &PIDREF_NULL)); + assert_se(pidref_equal(NULL, &PIDREF_NULL_NONCONST)); + assert_se(pidref_equal(&PIDREF_NULL_NONCONST, NULL)); + assert_se(pidref_equal(&PIDREF_NULL_NONCONST, &PIDREF_NULL_NONCONST)); assert_se(!pidref_equal(NULL, &PIDREF_MAKE_FROM_PID(1))); assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), NULL)); - assert_se(!pidref_equal(&PIDREF_NULL, &PIDREF_MAKE_FROM_PID(1))); - assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_NULL)); + assert_se(!pidref_equal(&PIDREF_NULL_NONCONST, &PIDREF_MAKE_FROM_PID(1))); + assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_NULL_NONCONST)); assert_se(pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_MAKE_FROM_PID(1))); assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_MAKE_FROM_PID(2))); }