Merge pull request #32872 from YHNdnzj/pidref-inode

pidref: record pidfd inode number in PidRef struct

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,9 +15,55 @@
 #include "signal-util.h"
 #include "stat-util.h"
 
-bool pidref_equal(const PidRef *a, const PidRef *b) {
+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));
+}
+
+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;
@@ -24,17 +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.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. */
-                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);

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.) */

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)));
 }