From: Lennart Poettering Date: Tue, 8 Oct 2024 08:01:22 +0000 (+0200) Subject: fd-util: use F_DUPFD_QUERY for same_fd() X-Git-Tag: v257-rc1~294^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F34675%2Fhead;p=thirdparty%2Fsystemd.git fd-util: use F_DUPFD_QUERY for same_fd() Catch up with the nice little toys the kernel fs developers have added for us. Preferably, let's make use of the new F_DUPFD_QUERY fcntl() call that checks whether two fds are just duplicates of each other (duplicates as in dup(), not as in open() of the same inode, i.e. whether they share a single file offset and so on). This API is much nicer, since it is a core kernel feature, unlike the kcmp() call we so far used, which is part of the (optional) checkpoint/restore stuff. F_DUPFD_QUERY is available since kernel 6.10. --- diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 3f8c5b92d32..c112f8dbad5 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -530,25 +530,57 @@ int same_fd(int a, int b) { assert(b >= 0); /* Compares two file descriptors. Note that semantics are quite different depending on whether we - * have kcmp() or we don't. If we have kcmp() this will only return true for dup()ed file - * descriptors, but not otherwise. If we don't have kcmp() this will also return true for two fds of - * the same file, created by separate open() calls. Since we use this call mostly for filtering out - * duplicates in the fd store this difference hopefully doesn't matter too much. */ + * have F_DUPFD_QUERY/kcmp() or we don't. If we have F_DUPFD_QUERY/kcmp() this will only return true + * for dup()ed file descriptors, but not otherwise. If we don't have F_DUPFD_QUERY/kcmp() this will + * also return true for two fds of the same file, created by separate open() calls. Since we use this + * call mostly for filtering out duplicates in the fd store this difference hopefully doesn't matter + * too much. + * + * Guarantees that if either of the passed fds is not allocated we'll return -EBADF. */ + + if (a == b) { + /* Let's validate that the fd is valid */ + r = fd_validate(a); + if (r < 0) + return r; - if (a == b) return true; + } + + /* Try to use F_DUPFD_QUERY if we have it first, as it is the nicest API */ + r = fcntl(a, F_DUPFD_QUERY, b); + if (r > 0) + return true; + if (r == 0) { + /* The kernel will return 0 in case the first fd is allocated, but the 2nd is not. (Which is different in the kcmp() case) Explicitly validate it hence. */ + r = fd_validate(b); + if (r < 0) + return r; + + return false; + } + /* On old kernels (< 6.10) that do not support F_DUPFD_QUERY this will return EINVAL for regular fds, and EBADF on O_PATH fds. Confusing. */ + if (errno == EBADF) { + /* EBADF could mean two things: the first fd is not valid, or it is valid and is O_PATH and + * F_DUPFD_QUERY is not supported. Let's validate the fd explicitly, to distinguish this + * case. */ + r = fd_validate(a); + if (r < 0) + return r; + + /* If the fd is valid, but we got EBADF, then let's try kcmp(). */ + } else if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno) && errno != EINVAL) + return -errno; /* Try to use kcmp() if we have it. */ pid = getpid_cached(); r = kcmp(pid, pid, KCMP_FILE, a, b); - if (r == 0) - return true; - if (r > 0) - return false; + if (r >= 0) + return !r; if (!ERRNO_IS_NOT_SUPPORTED(errno) && !ERRNO_IS_PRIVILEGE(errno)) return -errno; - /* We don't have kcmp(), use fstat() instead. */ + /* We have neither F_DUPFD_QUERY nor kcmp(), use fstat() instead. */ if (fstat(a, &sta) < 0) return -errno; diff --git a/src/basic/missing_fcntl.h b/src/basic/missing_fcntl.h index 1248eb7e4df..a6188879c1f 100644 --- a/src/basic/missing_fcntl.h +++ b/src/basic/missing_fcntl.h @@ -7,6 +7,10 @@ #define F_LINUX_SPECIFIC_BASE 1024 #endif +#ifndef F_DUPFD_QUERY +#define F_DUPFD_QUERY (F_LINUX_SPECIFIC_BASE + 3) +#endif + #ifndef F_SETPIPE_SZ #define F_SETPIPE_SZ (F_LINUX_SPECIFIC_BASE + 7) #endif diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c index e49a5dde457..20cf7b7627b 100644 --- a/src/test/test-fd-util.c +++ b/src/test/test-fd-util.c @@ -72,12 +72,14 @@ TEST(fd_validate) { TEST(same_fd) { _cleanup_close_pair_ int p[2]; - _cleanup_close_ int a, b, c; + _cleanup_close_ int a, b, c, d, e; assert_se(pipe2(p, O_CLOEXEC) >= 0); assert_se((a = fcntl(p[0], F_DUPFD, 3)) >= 0); assert_se((b = open("/dev/null", O_RDONLY|O_CLOEXEC)) >= 0); assert_se((c = fcntl(a, F_DUPFD, 3)) >= 0); + assert_se((d = open("/dev/null", O_RDONLY|O_CLOEXEC|O_PATH)) >= 0); /* O_PATH changes error returns in F_DUPFD_QUERY, let's test explicitly */ + assert_se((e = fcntl(d, F_DUPFD, 3)) >= 0); assert_se(same_fd(p[0], p[0]) > 0); assert_se(same_fd(p[1], p[1]) > 0); @@ -102,6 +104,20 @@ TEST(same_fd) { assert_se(same_fd(a, b) == 0); assert_se(same_fd(b, a) == 0); + + assert_se(same_fd(a, d) == 0); + assert_se(same_fd(d, a) == 0); + assert_se(same_fd(d, d) > 0); + assert_se(same_fd(d, e) > 0); + assert_se(same_fd(e, d) > 0); + + /* Let's now compare with a valid fd nr, that is definitely closed, and verify it returns the right error code */ + safe_close(d); + assert_se(same_fd(d, d) == -EBADF); + assert_se(same_fd(e, d) == -EBADF); + assert_se(same_fd(d, e) == -EBADF); + assert_se(same_fd(e, e) > 0); + TAKE_FD(d); } TEST(open_serialization_fd) {