If you wish to utilise a pidfd interface to refer to the current process or thread it is rather cumbersome, requiring something like:
int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
...
close(pidfd);
Or the equivalent call opening /proc/self. It is more convenient to use a sentinel value to indicate to an interface that accepts a pidfd that we simply wish to refer to the current process thread.
This series introduces sentinels for this purposes which can be passed as the pidfd in this instance rather than having to establish a dummy fd for this purpose.
It is useful to refer to both the current thread from the userland's perspective for which we use PIDFD_SELF, and the current process from the userland's perspective, for which we use PIDFD_SELF_PROCESS.
There is unfortunately some confusion between the kernel and userland as to what constitutes a process - a thread from the userland perspective is a process in userland, and a userland process is a thread group (more specifically the thread group leader from the kernel perspective). We therefore alias things thusly:
* PIDFD_SELF_THREAD aliased by PIDFD_SELF - use PIDTYPE_PID. * PIDFD_SELF_THREAD_GROUP alised by PIDFD_SELF_PROCESS - use PIDTYPE_TGID.
In all of the kernel code we refer to PIDFD_SELF_THREAD and PIDFD_SELF_THREAD_GROUP. However we expect users to use PIDFD_SELF and PIDFD_SELF_PROCESS.
This matters for cases where, for instance, a user unshare()'s FDs or does thread-specific signal handling and where the user would be hugely confused if the FDs referenced or signal processed referred to the thread group leader rather than the individual thread.
We ensure that pidfd_send_signal() and pidfd_getfd() work correctly, and assert as much in selftests. All other interfaces except setns() will work implicitly with this new interface, however it doesn't make sense to test waitid(P_PIDFD, ...) as waiting on ourselves is a blocking operation.
In the case of setns() we explicitly disallow use of PIDFD_SELF* as it doesn't make sense to obtain the namespaces of our own process, and it would require work to implement this functionality there that would be of no use.
We also do not provide the ability to utilise PIDFD_SELF* in ordinary fd operations such as open() or poll(), as this would require extensive work and be of no real use.
v3: * Do not fput() an invalid fd as reported by kernel test bot. * Fix unintended churn from moving variable declaration.
v2: * Fix tests as reported by Shuah. * Correct RFC version lore link. https://lore.kernel.org/linux-mm/cover.1728643714.git.lorenzo.stoakes@oracle...
Non-RFC v1: * Removed RFC tag - there seems to be general consensus that this change is a good idea, but perhaps some debate to be had on implementation. It seems sensible then to move forward with the RFC flag removed. * Introduced PIDFD_SELF_THREAD, PIDFD_SELF_THREAD_GROUP and their aliases PIDFD_SELF and PIDFD_SELF_PROCESS respectively. * Updated testing accordingly. https://lore.kernel.org/linux-mm/cover.1728578231.git.lorenzo.stoakes@oracle...
RFC version: https://lore.kernel.org/linux-mm/cover.1727644404.git.lorenzo.stoakes@oracle...
Lorenzo Stoakes (3): pidfd: extend pidfd_get_pid() and de-duplicate pid lookup pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process selftests: pidfd: add tests for PIDFD_SELF_*
include/linux/pid.h | 43 +++++- include/uapi/linux/pidfd.h | 15 ++ kernel/exit.c | 3 +- kernel/nsproxy.c | 1 + kernel/pid.c | 73 ++++++--- kernel/signal.c | 26 +--- tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 10 files changed, 342 insertions(+), 55 deletions(-)
-- 2.46.2
The means by which a pid is determined from a pidfd is duplicated, with some callers holding a reference to the (pid)fd, and others explicitly pinning the pid.
Introduce __pidfd_get_pid() which abstracts both approaches and provide optional output parameters for file->f_flags and the fd (the latter of which, if provided, prevents the function from decrementing the fd's refernce count).
Additionally, allow the ability to open a pidfd by opening a /proc/<pid> directory, utilised by the pidfd_send_signal() system call, providing a pidfd_get_pid_proc() helper function to do so.
Doing this allows us to eliminate open-coded pidfd pid lookup and to consistently handle this in one place.
This lays the groundwork for a subsequent patch which adds a new sentinel pidfd to explicitly reference the current process (i.e. thread group leader) without the need for a pidfd.
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com --- include/linux/pid.h | 42 +++++++++++++++++++++++++++++++- kernel/pid.c | 58 ++++++++++++++++++++++++++++++--------------- kernel/signal.c | 26 ++++---------------- 3 files changed, 85 insertions(+), 41 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index a3aad9b4074c..68b02eab7509 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -2,6 +2,7 @@ #ifndef _LINUX_PID_H #define _LINUX_PID_H
+#include <linux/file.h> #include <linux/pid_types.h> #include <linux/rculist.h> #include <linux/rcupdate.h> @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
struct file;
+ +/** + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd. + * + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if + * @alloc_proc is also set. + * @pin_pid: If set, then the reference counter of the returned pid is + * incremented. If not set, then @fd should be provided to pin the + * pidfd. + * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead + * of a pidfd, and this will be used to determine the pid. + * @flags: Output variable, if non-NULL, then the file->f_flags of the + * pidfd will be set here. + * @fd: Output variable, if non-NULL, then the pidfd reference will + * remain elevated and the caller will need to decrement it + * themselves. + * + * Returns: If successful, the pid associated with the pidfd, otherwise an + * error. + */ +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid, + bool allow_proc, unsigned int *flags, + struct fd *fd); + +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags) +{ + return __pidfd_get_pid(pidfd, /* pin_pid = */ true, + /* allow_proc = */ false, + flags, /* fd = */ NULL); +} + +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd, + unsigned int *flags, + struct fd *fd) +{ + return __pidfd_get_pid(pidfd, /* pin_pid = */ false, + /* allow_proc = */ true, + flags, fd); +} + struct pid *pidfd_pid(const struct file *file); -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags); struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags); int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret); void do_notify_pidfd(struct task_struct *task); diff --git a/kernel/pid.c b/kernel/pid.c index 2715afb77eab..25cc1c36a1b1 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -36,6 +36,7 @@ #include <linux/pid_namespace.h> #include <linux/init_task.h> #include <linux/syscalls.h> +#include <linux/proc_fs.h> #include <linux/proc_ns.h> #include <linux/refcount.h> #include <linux/anon_inodes.h> @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns) } EXPORT_SYMBOL_GPL(find_ge_pid);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags) +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid, + bool allow_proc, unsigned int *flags, + struct fd *fd) { - struct fd f; + struct file *file; struct pid *pid; + struct fd f = fdget(pidfd);
- f = fdget(fd); - if (!fd_file(f)) + file = fd_file(f); + if (!file) return ERR_PTR(-EBADF);
- pid = pidfd_pid(fd_file(f)); - if (!IS_ERR(pid)) { - get_pid(pid); - *flags = fd_file(f)->f_flags; + pid = pidfd_pid(file); + /* If we allow opening a pidfd via /proc/<pid>, do so. */ + if (IS_ERR(pid) && allow_proc) + pid = tgid_pidfd_to_pid(file); + + if (IS_ERR(pid)) { + fdput(f); + return pid; }
- fdput(f); + if (pin_pid) + get_pid(pid); + else + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */ + + if (flags) + *flags = file->f_flags; + + /* + * If the user provides an fd output then it will handle decrementing + * its reference counter. + */ + if (fd) + *fd = f; + else + /* Otherwise we release it. */ + fdput(f); + return pid; }
@@ -747,23 +772,18 @@ SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd, unsigned int, flags) { struct pid *pid; - struct fd f; int ret;
/* flags is currently unused - make sure it's unset */ if (flags) return -EINVAL;
- f = fdget(pidfd); - if (!fd_file(f)) - return -EBADF; - - pid = pidfd_pid(fd_file(f)); + pid = pidfd_get_pid(pidfd, NULL); if (IS_ERR(pid)) - ret = PTR_ERR(pid); - else - ret = pidfd_getfd(pid, fd); + return PTR_ERR(pid);
- fdput(f); + ret = pidfd_getfd(pid, fd); + + put_pid(pid); return ret; } diff --git a/kernel/signal.c b/kernel/signal.c index 4344860ffcac..e64da346a90b 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -3875,17 +3875,6 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, return copy_siginfo_from_user(kinfo, info); }
-static struct pid *pidfd_to_pid(const struct file *file) -{ - struct pid *pid; - - pid = pidfd_pid(file); - if (!IS_ERR(pid)) - return pid; - - return tgid_pidfd_to_pid(file); -} - #define PIDFD_SEND_SIGNAL_FLAGS \ (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \ PIDFD_SIGNAL_PROCESS_GROUP) @@ -3912,6 +3901,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, struct pid *pid; kernel_siginfo_t kinfo; enum pid_type type; + unsigned int f_flags;
/* Enforce flags be set to 0 until we add an extension. */ if (flags & ~PIDFD_SEND_SIGNAL_FLAGS) @@ -3921,16 +3911,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1) return -EINVAL;
- f = fdget(pidfd); - if (!fd_file(f)) - return -EBADF; - /* Is this a pidfd? */ - pid = pidfd_to_pid(fd_file(f)); - if (IS_ERR(pid)) { - ret = PTR_ERR(pid); - goto err; - } + pid = pidfd_to_pid_proc(pidfd, &f_flags, &f); + if (IS_ERR(pid)) + return PTR_ERR(pid);
ret = -EINVAL; if (!access_pidfd_pidns(pid)) @@ -3939,7 +3923,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, switch (flags) { case 0: /* Infer scope from the type of pidfd. */ - if (fd_file(f)->f_flags & PIDFD_THREAD) + if (f_flags & PIDFD_THREAD) type = PIDTYPE_PID; else type = PIDTYPE_TGID;
It is useful to be able to utilise pidfd mechanisms to reference the current thread or process (from a userland point of view - thread group leader from the kernel's point of view).
Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
For convenience and to avoid confusion from userland's perspective we alias these:
* PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what the user will want to use, as they would find it surprising if for instance fd's were unshared()'d and they wanted to invoke pidfd_getfd() and that failed.
* PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users have no concept of thread groups or what a thread group leader is, and from userland's perspective and nomenclature this is what userland considers to be a process.
Due to the refactoring of the central __pidfd_get_pid() function we can implement this functionality centrally, providing the use of this sentinel in most functionality which utilises pidfd's.
We need to explicitly adjust kernel_waitid_prepare() to permit this (though it wouldn't really make sense to use this there, we provide the ability for consistency).
We explicitly disallow use of this in setns(), which would otherwise have required explicit custom handling, as it doesn't make sense to set the current calling thread to join the namespace of itself.
As the callers of pidfd_get_pid() expect an increased reference count on the pid we do so in the self case, reducing churn and avoiding any breakage from existing logic which decrements this reference count.
In the pidfd_send_signal() system call, we can continue to fdput() the struct fd output by pidfs_to_pid_proc() even if PIDFD_SELF_* is specified, as this will be empty and the invocation will be a no-op.
This change implicitly provides PIDFD_SELF_* support in the waitid(P_PIDFS, ...), process_madvise(), process_mrelease(), pidfd_send_signal(), and pidfd_getfd() system calls.
Things such as polling a pidfs and general fd operations are not supported, this strictly provides the sentinel for APIs which explicitly accept a pidfd.
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com --- include/linux/pid.h | 9 +++--- include/uapi/linux/pidfd.h | 15 +++++++++ kernel/exit.c | 3 +- kernel/nsproxy.c | 1 + kernel/pid.c | 65 +++++++++++++++++++++++--------------- 5 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index 68b02eab7509..7c9ed1b5d16f 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -77,18 +77,19 @@ struct file; /** * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd. * - * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if - * @alloc_proc is also set. + * @pidfd: The pidfd whose pid we want, the fd of a /proc/<pid> file if + * @alloc_proc is also set, or PIDFD_SELF_* to refer to the current + * thread or thread group leader. * @pin_pid: If set, then the reference counter of the returned pid is * incremented. If not set, then @fd should be provided to pin the * pidfd. * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead * of a pidfd, and this will be used to determine the pid. * @flags: Output variable, if non-NULL, then the file->f_flags of the - * pidfd will be set here. + * pidfd will be set here. If PIDFD_SELF_* set, this is zero. * @fd: Output variable, if non-NULL, then the pidfd reference will * remain elevated and the caller will need to decrement it - * themselves. + * themselves. If PIDFD_SELF_* set, this is empty. * * Returns: If successful, the pid associated with the pidfd, otherwise an * error. diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h index 565fc0629fff..f4db20d76f4b 100644 --- a/include/uapi/linux/pidfd.h +++ b/include/uapi/linux/pidfd.h @@ -29,4 +29,19 @@ #define PIDFD_GET_USER_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 9) #define PIDFD_GET_UTS_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 10)
+/* + * Special sentinel values which can be used to refer to the current thread or + * thread group leader (which from a userland perspective is the process). + */ +#define PIDFD_SELF PIDFD_SELF_THREAD +#define PIDFD_SELF_PROCESS PIDFD_SELF_THREAD_GROUP + +#define PIDFD_SELF_THREAD -100 /* Current thread. */ +#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */ + +static inline bool pidfd_is_self_sentinel(pid_t pid) +{ + return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP; +} + #endif /* _UAPI_LINUX_PIDFD_H */ diff --git a/kernel/exit.c b/kernel/exit.c index 619f0014c33b..3eb20f8252ee 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -71,6 +71,7 @@ #include <linux/user_events.h> #include <linux/uaccess.h>
+#include <uapi/linux/pidfd.h> #include <uapi/linux/wait.h>
#include <asm/unistd.h> @@ -1739,7 +1740,7 @@ int kernel_waitid_prepare(struct wait_opts *wo, int which, pid_t upid, break; case P_PIDFD: type = PIDTYPE_PID; - if (upid < 0) + if (upid < 0 && !pidfd_is_self_sentinel(upid)) return -EINVAL;
pid = pidfd_get_pid(upid, &f_flags); diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c index dc952c3b05af..d239f7eeaa1f 100644 --- a/kernel/nsproxy.c +++ b/kernel/nsproxy.c @@ -550,6 +550,7 @@ SYSCALL_DEFINE2(setns, int, fd, int, flags) struct nsset nsset = {}; int err = 0;
+ /* If fd is PIDFD_SELF_*, implicitly fail here, as invalid. */ if (!fd_file(f)) return -EBADF;
diff --git a/kernel/pid.c b/kernel/pid.c index 25cc1c36a1b1..0f8943ecc471 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -539,22 +539,31 @@ struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid, bool allow_proc, unsigned int *flags, struct fd *fd) { - struct file *file; + struct file *file = NULL; struct pid *pid; - struct fd f = fdget(pidfd); - - file = fd_file(f); - if (!file) - return ERR_PTR(-EBADF); - - pid = pidfd_pid(file); - /* If we allow opening a pidfd via /proc/<pid>, do so. */ - if (IS_ERR(pid) && allow_proc) - pid = tgid_pidfd_to_pid(file); - - if (IS_ERR(pid)) { - fdput(f); - return pid; + unsigned int f_flags = 0; + struct fd f = {}; + + if (pidfd == PIDFD_SELF_THREAD) { + pid = *task_pid_ptr(current, PIDTYPE_PID); + f_flags = PIDFD_THREAD; + } else if (pidfd == PIDFD_SELF_THREAD_GROUP) { + pid = *task_pid_ptr(current, PIDTYPE_TGID); + } else { + f = fdget(pidfd); + file = fd_file(f); + if (!file) + return ERR_PTR(-EBADF); + + pid = pidfd_pid(file); + /* If we allow opening a pidfd via /proc/<pid>, do so. */ + if (IS_ERR(pid) && allow_proc) + pid = tgid_pidfd_to_pid(file); + + if (IS_ERR(pid)) { + fdput(f); + return pid; + } }
if (pin_pid) @@ -562,18 +571,22 @@ struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid, else WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
- if (flags) - *flags = file->f_flags; + if (file) { + f_flags = file->f_flags;
- /* - * If the user provides an fd output then it will handle decrementing - * its reference counter. - */ - if (fd) - *fd = f; - else - /* Otherwise we release it. */ - fdput(f); + /* + * If the user provides an fd output then it will handle decrementing + * its reference counter. + */ + if (fd) + *fd = f; + else + /* Otherwise we release it. */ + fdput(f); + } + + if (flags) + *flags = f_flags;
return pid; }
Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process.
This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying).
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com --- tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif
+/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif + /* * The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c * That means, when it wraps around any pid < 300 will be skipped. diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c index cd51d547b751..48d224b13c01 100644 --- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c +++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c @@ -6,6 +6,7 @@ #include <limits.h> #include <linux/types.h> #include <poll.h> +#include <pthread.h> #include <sched.h> #include <signal.h> #include <stdio.h> @@ -15,6 +16,7 @@ #include <sys/prctl.h> #include <sys/wait.h> #include <unistd.h> +#include <sys/mman.h> #include <sys/socket.h> #include <linux/kcmp.h>
@@ -114,6 +116,94 @@ static int child(int sk) return ret; }
+static int __pidfd_self_thread_worker(unsigned long page_size) +{ + int memfd; + int newfd; + char *ptr; + int err = 0; + + /* + * Unshare our FDs so we have our own set. This means + * PIDFD_SELF_THREAD_GROUP will fal. + */ + if (unshare(CLONE_FILES) < 0) { + err = -errno; + goto exit; + } + + /* Truncate, map in and write to our memfd. */ + memfd = sys_memfd_create("test_self_child", 0); + if (memfd < 0) { + err = -errno; + goto exit; + } + + if (ftruncate(memfd, page_size)) { + err = -errno; + goto exit_close_memfd; + } + + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE, + MAP_SHARED, memfd, 0); + if (ptr == MAP_FAILED) { + err = -errno; + goto exit_close_memfd; + } + ptr[0] = 'y'; + if (munmap(ptr, page_size)) { + err = -errno; + goto exit_close_memfd; + } + + /* Get a thread-local duplicate of our memfd. */ + newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0); + if (newfd < 0) { + err = -errno; + goto exit_close_memfd; + } + + if (memfd == newfd) { + err = -EINVAL; + goto exit_close_fds; + } + + /* Map in new fd and make sure that the data is as expected. */ + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE, + MAP_SHARED, newfd, 0); + if (ptr == MAP_FAILED) { + err = -errno; + goto exit_close_fds; + } + + if (ptr[0] != 'y') { + err = -EINVAL; + goto exit_close_fds; + } + + if (munmap(ptr, page_size)) { + err = -errno; + goto exit_close_fds; + } + +exit_close_fds: + close(newfd); +exit_close_memfd: + close(memfd); +exit: + return err; +} + +static void *pidfd_self_thread_worker(void *arg) +{ + unsigned long page_size = (unsigned long)arg; + int ret; + + /* We forward any errors for the caller to handle. */ + ret = __pidfd_self_thread_worker(page_size); + return (void *)(intptr_t)ret; +} + FIXTURE(child) { /* @@ -264,6 +354,57 @@ TEST_F(child, no_strange_EBADF) EXPECT_EQ(errno, ESRCH); }
+TEST(pidfd_self) +{ + int memfd = sys_memfd_create("test_self", 0); + unsigned long page_size = sysconf(_SC_PAGESIZE); + int newfd; + char *ptr; + pthread_t thread; + void *res; + int err; + + ASSERT_GE(memfd, 0); + ASSERT_EQ(ftruncate(memfd, page_size), 0); + + /* + * Map so we can assert that the duplicated fd references the same + * memory. + */ + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE, + MAP_SHARED, memfd, 0); + ASSERT_NE(ptr, MAP_FAILED); + ptr[0] = 'x'; + ASSERT_EQ(munmap(ptr, page_size), 0); + + /* Now get a duplicate of our memfd. */ + newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD_GROUP, memfd, 0); + ASSERT_GE(newfd, 0); + ASSERT_NE(memfd, newfd); + + /* Now map duplicate fd and make sure it references the same memory. */ + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE, + MAP_SHARED, newfd, 0); + ASSERT_NE(ptr, MAP_FAILED); + ASSERT_EQ(ptr[0], 'x'); + ASSERT_EQ(munmap(ptr, page_size), 0); + + /* Cleanup. */ + close(memfd); + close(newfd); + + /* + * Fire up the thread and assert that we can lookup the thread-specific + * PIDFD_SELF_THREAD (also aliased by PIDFD_SELF). + */ + ASSERT_EQ(pthread_create(&thread, NULL, pidfd_self_thread_worker, + (void *)page_size), 0); + ASSERT_EQ(pthread_join(thread, &res), 0); + err = (int)(intptr_t)res; + + ASSERT_EQ(err, 0); +} + #if __NR_pidfd_getfd == -1 int main(void) { diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c index 7c2a4349170a..bbd39dc5ceb7 100644 --- a/tools/testing/selftests/pidfd/pidfd_setns_test.c +++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c @@ -752,4 +752,15 @@ TEST(setns_einval) close(fd); }
+TEST(setns_pidfd_self_disallowed) +{ + ASSERT_EQ(setns(PIDFD_SELF_THREAD, 0), -1); + EXPECT_EQ(errno, EBADF); + + errno = 0; + + ASSERT_EQ(setns(PIDFD_SELF_THREAD_GROUP, 0), -1); + EXPECT_EQ(errno, EBADF); +} + TEST_HARNESS_MAIN diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c index 9faa686f90e4..440447cf89ba 100644 --- a/tools/testing/selftests/pidfd/pidfd_test.c +++ b/tools/testing/selftests/pidfd/pidfd_test.c @@ -42,12 +42,41 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *)) #endif }
-static int signal_received; +static pthread_t signal_received;
static void set_signal_received_on_sigusr1(int sig) { if (sig == SIGUSR1) - signal_received = 1; + signal_received = pthread_self(); +} + +static int send_signal(int pidfd) +{ + int ret = 0; + + if (sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0) < 0) { + ret = -EINVAL; + goto exit; + } + + if (signal_received != pthread_self()) { + ret = -EINVAL; + goto exit; + } + +exit: + signal_received = 0; + return ret; +} + +static void *send_signal_worker(void *arg) +{ + int pidfd = (int)(intptr_t)arg; + int ret; + + /* We forward any errors for the caller to handle. */ + ret = send_signal(pidfd); + return (void *)(intptr_t)ret; }
/* @@ -56,8 +85,11 @@ static void set_signal_received_on_sigusr1(int sig) */ static int test_pidfd_send_signal_simple_success(void) { - int pidfd, ret; + int pidfd; const char *test_name = "pidfd_send_signal send SIGUSR1"; + pthread_t thread; + void *thread_res; + int err;
if (!have_pidfd_send_signal) { ksft_test_result_skip( @@ -66,25 +98,45 @@ static int test_pidfd_send_signal_simple_success(void) return 0; }
+ signal(SIGUSR1, set_signal_received_on_sigusr1); + + /* Try sending a signal to ourselves via /proc/self. */ pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC); if (pidfd < 0) ksft_exit_fail_msg( "%s test: Failed to open process file descriptor\n", test_name); + err = send_signal(pidfd); + if (err) + ksft_exit_fail_msg( + "%s test: Error %d on sending pidfd signal\n", + test_name, err); + close(pidfd);
- signal(SIGUSR1, set_signal_received_on_sigusr1); + /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */ + err = send_signal(PIDFD_SELF_THREAD_GROUP); + if (err) + ksft_exit_fail_msg( + "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n", + test_name, err);
- ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0); - close(pidfd); - if (ret < 0) - ksft_exit_fail_msg("%s test: Failed to send signal\n", + /* + * Now try the same thing in a thread and assert thread ID is equal to + * worker thread ID. + */ + if (pthread_create(&thread, NULL, send_signal_worker, + (void *)(intptr_t)PIDFD_SELF_THREAD)) + ksft_exit_fail_msg("%s test: Failed to create thread\n", test_name); - - if (signal_received != 1) - ksft_exit_fail_msg("%s test: Failed to receive signal\n", + if (pthread_join(thread, &thread_res)) + ksft_exit_fail_msg("%s test: Failed to join thread\n", test_name); + err = (int)(intptr_t)thread_res; + if (err) + ksft_exit_fail_msg( + "%s test: Error %d on PIDFD_SELF_THREAD signal\n", + test_name, err);
- signal_received = 0; ksft_test_result_pass("%s test: Sent signal\n", test_name); return 0; }
On 10/16/24 04:20, Lorenzo Stoakes wrote:
Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process.
This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying).
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later.
The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working.
Please revise this patch to include the header file and remove these local defines.
thanks, -- Shuah
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process.
This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying).
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
Unfortunately this seems broken on my system anyway :( - see below.
These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later.
I don't agree, tests have to be maintained alongside the core code, and if these values change (seems unlikely) then the tests will fail and can easily be updated.
This was the approach already taken in this file with other linux header-defined values, so we'll also be breaking the precendence.
The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working.
I just tried this and it's not fine :) it immediately broke the build as pidfd.h imports linux/fcntl.h which conflicts horribly with system headers on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct f_owner_ex’ 155 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from pidfd_test.c:6: /usr/include/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~
It seems only one other test tries to do this as far as I can tell (I only did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the <fcntl.h> include and that resulted in <sys/mount.h> conflicting:
In file included from /usr/include/fcntl.h:35, from /usr/include/sys/mount.h:24, from pidfd.h:17, from pidfd_test.c:22: /usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’ 35 | struct flock | ^~~~~ In file included from /tmp/hdr/include/asm/fcntl.h:1, from /tmp/hdr/include/linux/fcntl.h:5, from /tmp/hdr/include/linux/pidfd.h:7, from pidfd.h:6: /usr/include/asm-generic/fcntl.h:195:8: note: originally defined here 195 | struct flock { | ^~~~~
So I don't think I can actually work around this, at least on my system, and I can't really sensibly submit a patch that I can't run on my own machine :)
I may be missing something here.
Please revise this patch to include the header file and remove these local defines.
I'm a little stuck because of the above, but I _could_ do the following in the test pidfd.h header.:
#define _LINUX_FCNTL_H #include "../../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
Which prevents the problematic linux/fcntl.h header from being included and includes the right header.
But I'm not sure this is hugely better than what we already have maintinability-wise? Either way if something changes to break it it'll break the test build.
Let me know if this is what you want me to do. Otherwise I'm not sure how to proceed - this header just seems broken at least on my system (arch linux at 6.11.1).
An aside:
The existing code already taken the approach I take (this is partly why I did it), I think it'd be out of the scope of my series to change that, for instance in pidfd.h:
#ifndef PIDFD_NONBLOCK #define PIDFD_NONBLOCK O_NONBLOCK #endif
Alongside a number of other defines. So those will have to stay at least for now for being out of scope, but obviously if people would prefer to move the whole thing that can be followed up later.
thanks, -- Shuah
On Wed, Oct 16, 2024 at 11:06:34PM +0100, Lorenzo Stoakes wrote: [sniip]
The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working.
I just tried this and it's not fine :) it immediately broke the build as pidfd.h imports linux/fcntl.h which conflicts horribly with system headers on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct f_owner_ex’ 155 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from pidfd_test.c:6: /usr/include/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~
It seems only one other test tries to do this as far as I can tell (I only did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the <fcntl.h> include and that resulted in <sys/mount.h> conflicting:
In file included from /usr/include/fcntl.h:35, from /usr/include/sys/mount.h:24, from pidfd.h:17, from pidfd_test.c:22: /usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’ 35 | struct flock | ^~~~~ In file included from /tmp/hdr/include/asm/fcntl.h:1, from /tmp/hdr/include/linux/fcntl.h:5, from /tmp/hdr/include/linux/pidfd.h:7, from pidfd.h:6: /usr/include/asm-generic/fcntl.h:195:8: note: originally defined here 195 | struct flock { | ^~~~~
So I don't think I can actually work around this, at least on my system, and I can't really sensibly submit a patch that I can't run on my own machine :)
I may be missing something here.
[snip]
Some added data:
OK so I asked people on fedi to compile the following locally (also a variant with _GNU_SOURCE being defined):
#include <linux/pidfd.h> #include <fcntl.h>
int main(void) {}
And they are all encountering the same issue as I am on a number of different distros (ordering of includes doesn't seem to matter either).
So this seems like a known-broken thing.
And we can't really isolate inclusion of this file since all the tests interact directly with defines from it.
So it seems the only solution is the workaround I suggested previously I think with the header guard define hack.
[snip]
On 10/16/24 16:06, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process.
This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying).
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
Yes that is exactly what we do. kselftest build depends on headers install. The way it works for qemu is either using vitme-ng or building tests and installing them in your vm.. This is what CIs do.
This is a use case I use all the time so not at all theoretical.
This is what CIs do. Yes - it works for them to build and install headers. You don't have to install them on the build system. You run "make headers" in your repo. You could use O= option for relocatable build.
Unfortunately this seems broken on my system anyway :( - see below.
These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later.
I don't agree, tests have to be maintained alongside the core code, and if these values change (seems unlikely) then the tests will fail and can easily be updated.
This was the approach already taken in this file with other linux header-defined values, so we'll also be breaking the precendence.
Some of these defines were added a while back. Often these defines need cleaning up. I would rather not see new ones added unless it is absolutely necessary.
The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working.
I just tried this and it's not fine :) it immediately broke the build as pidfd.h imports linux/fcntl.h which conflicts horribly with system headers on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct f_owner_ex’ 155 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from pidfd_test.c:6: /usr/include/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~
It seems only one other test tries to do this as far as I can tell (I only did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the <fcntl.h> include and that resulted in <sys/mount.h> conflicting:
In file included from /usr/include/fcntl.h:35, from /usr/include/sys/mount.h:24, from pidfd.h:17, from pidfd_test.c:22: /usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’ 35 | struct flock | ^~~~~ In file included from /tmp/hdr/include/asm/fcntl.h:1, from /tmp/hdr/include/linux/fcntl.h:5, from /tmp/hdr/include/linux/pidfd.h:7, from pidfd.h:6: /usr/include/asm-generic/fcntl.h:195:8: note: originally defined here 195 | struct flock { | ^~~~~
So I don't think I can actually work around this, at least on my system, and I can't really sensibly submit a patch that I can't run on my own machine :)
I may be missing something here.
Please revise this patch to include the header file and remove these local defines.
I'm a little stuck because of the above, but I _could_ do the following in the test pidfd.h header.:
#define _LINUX_FCNTL_H #include "../../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
Does this test really need fcntl.h is another question. This is another problem with too many includes. The test built just fine on my system on 6.12-rc3 with
+/* #include <fcntl.h> */
Which prevents the problematic linux/fcntl.h header from being included and includes the right header.
But I'm not sure this is hugely better than what we already have maintinability-wise? Either way if something changes to break it it'll break the test build.
If these defines are in a header file - tests include them. Part of test development is figuring out these problems.
Let me know if this is what you want me to do. Otherwise I'm not sure how to proceed - this header just seems broken at least on my system (arch linux at 6.11.1).
An aside:
The existing code already taken the approach I take (this is partly why I did it), I think it'd be out of the scope of my series to change that, for instance in pidfd.h:
#ifndef PIDFD_NONBLOCK #define PIDFD_NONBLOCK O_NONBLOCK #endif
Alongside a number of other defines. So those will have to stay at least for now for being out of scope, but obviously if people would prefer to move the whole thing that can be followed up later.
I would like us to explore before giving up and saying these will stay.
thanks, -- Shuah
On Wed, Oct 16, 2024 at 04:38:50PM -0600, Shuah Khan wrote:
On 10/16/24 16:06, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
Add tests to assert that PIDFD_SELF_* correctly refers to the current thread and process.
This is only practically meaningful to pidfd_send_signal() and pidfd_getfd(), but also explicitly test that we disallow this feature for setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely process_madvise() and process_mrelease() to mm testing (though note the latter can not be sensibly tested as it would require the testing process to be dying).
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/testing/selftests/pidfd/pidfd.h | 8 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
Yes that is exactly what we do. kselftest build depends on headers install. The way it works for qemu is either using vitme-ng or building tests and installing them in your vm.. This is what CIs do.
This is a use case I use all the time so not at all theoretical.
This is what CIs do. Yes - it works for them to build and install headers. You don't have to install them on the build system. You run "make headers" in your repo. You could use O= option for relocatable build.
Right but I'm talking about my local builds in order to test the kernel. See John's response.
Unfortunately this seems broken on my system anyway :( - see below.
These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later.
I don't agree, tests have to be maintained alongside the core code, and if these values change (seems unlikely) then the tests will fail and can easily be updated.
This was the approach already taken in this file with other linux header-defined values, so we'll also be breaking the precendence.
Some of these defines were added a while back. Often these defines need cleaning up. I would rather not see new ones added unless it is absolutely necessary.
OK, but just to note that I am now not doing a PIDFD_SELF series, I'm doing a 'PIDFD_SELF and completely change how pidfd does testing' series.
To me the right thing to do would be to send 2 series and not block this one on this issue.
The import will be fine and you can control that with -I flag in the makefile. Remove these and try to get including linux/pidfd.h working.
I just tried this and it's not fine :) it immediately broke the build as pidfd.h imports linux/fcntl.h which conflicts horribly with system headers on my machine.
For instance f_owner_ex gets redefined among others and fails the build e..g:
/usr/include/asm-generic/fcntl.h:155:8: error: redefinition of ‘struct f_owner_ex’ 155 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from pidfd_test.c:6: /usr/include/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~
It seems only one other test tries to do this as far as I can tell (I only did a quick grep), so it's not at all standard it seems.
This issue occurred even when I used make headers_install to create sanitised user headers and added them to the include path.
A quick google suggests linux/fcntl.h (imported by this pidfd.h uapi header) and system fcntl.h is a known thing. Slightly bizarre...
I tried removing the <fcntl.h> include and that resulted in <sys/mount.h> conflicting:
In file included from /usr/include/fcntl.h:35, from /usr/include/sys/mount.h:24, from pidfd.h:17, from pidfd_test.c:22: /usr/include/bits/fcntl.h:35:8: error: redefinition of ‘struct flock’ 35 | struct flock | ^~~~~ In file included from /tmp/hdr/include/asm/fcntl.h:1, from /tmp/hdr/include/linux/fcntl.h:5, from /tmp/hdr/include/linux/pidfd.h:7, from pidfd.h:6: /usr/include/asm-generic/fcntl.h:195:8: note: originally defined here 195 | struct flock { | ^~~~~
So I don't think I can actually work around this, at least on my system, and I can't really sensibly submit a patch that I can't run on my own machine :)
I may be missing something here.
Please revise this patch to include the header file and remove these local defines.
I'm a little stuck because of the above, but I _could_ do the following in the test pidfd.h header.:
#define _LINUX_FCNTL_H #include "../../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
Does this test really need fcntl.h is another question. This is another problem with too many includes. The test built just fine on my system on 6.12-rc3 with
+/* #include <fcntl.h> */
Like I said to you above (maybe I wasn't clear?) I tried this and doing this doesn't work for me, as sys/mount.h implicitly includes this header, and we need things from that, so we're just broken.
And I cannot submit a series that literally breaks on my machine obviously.
So simply including this header is a no-go here.
I've provided a workaround above. Also John has suggested using the tools/ directory as previously agreed upon. I could remove the linux/fcntl.h dependency from that and place the header there which is probably the neatest solution.
Which prevents the problematic linux/fcntl.h header from being included and includes the right header.
But I'm not sure this is hugely better than what we already have maintinability-wise? Either way if something changes to break it it'll break the test build.
If these defines are in a header file - tests include them. Part of test development is figuring out these problems.
Right but part of a series introducing a new feature isn't to permanently break tests from working.
And the includes are in that UAPI-exposed header file they're pretty much set in stone or risk breaking userland.
Let me know if this is what you want me to do. Otherwise I'm not sure how to proceed - this header just seems broken at least on my system (arch linux at 6.11.1).
An aside:
The existing code already taken the approach I take (this is partly why I did it), I think it'd be out of the scope of my series to change that, for instance in pidfd.h:
#ifndef PIDFD_NONBLOCK #define PIDFD_NONBLOCK O_NONBLOCK #endif
Alongside a number of other defines. So those will have to stay at least for now for being out of scope, but obviously if people would prefer to move the whole thing that can be followed up later.
I would like us to explore before giving up and saying these will stay.
I'm not sure how I'm meant to explore 'this breaks the build on my system'. The sys/mount.h is a deal-breaker, there are things in there we _need_.
thanks, -- Shuah
In any case I think copying the header to the tools/ directory with this linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h there?) is the sensible way forward.
A 'just include the header' is simply not an option as it breaks the tests.
+cc John, sorry I forgot to cc you on other replies!!
On Thu, Oct 17, 2024 at 09:08:19AM +0100, Lorenzo Stoakes wrote: [snip]
In any case I think copying the header to the tools/ directory with this linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h there?) is the sensible way forward.
A 'just include the header' is simply not an option as it breaks the tests.
Ohhh ok I think maybe we could have a good compromise that should (hopefully!) satisfy both you and John.
I can introduce tools/include/linux/pidfd.h that is a stub wrapper around the pidfd.h file.
So it can be something like:
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote:
+cc John, sorry I forgot to cc you on other replies!!
On Thu, Oct 17, 2024 at 09:08:19AM +0100, Lorenzo Stoakes wrote: [snip]
In any case I think copying the header to the tools/ directory with this linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h there?) is the sensible way forward.
A 'just include the header' is simply not an option as it breaks the tests.
I should have read this one first, this morning, but I missed it initially. :)
Ohhh ok I think maybe we could have a good compromise that should (hopefully!) satisfy both you and John.
I can introduce tools/include/linux/pidfd.h that is a stub wrapper around the pidfd.h file.
So it can be something like:
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
Yes.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Very nice!
thanks,
On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote:
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote:
+cc John, sorry I forgot to cc you on other replies!!
On Thu, Oct 17, 2024 at 09:08:19AM +0100, Lorenzo Stoakes wrote: [snip]
In any case I think copying the header to the tools/ directory with this linux/fcntl.h in some way stubbed out (we could even stub out fcntl.h there?) is the sensible way forward.
A 'just include the header' is simply not an option as it breaks the tests.
I should have read this one first, this morning, but I missed it initially. :)
No worries easily done! :)
Ohhh ok I think maybe we could have a good compromise that should (hopefully!) satisfy both you and John.
I can introduce tools/include/linux/pidfd.h that is a stub wrapper around the pidfd.h file.
So it can be something like:
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
Yes.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Very nice!
Thanks!
I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here.
Or would you say the wrapper should regardless be in the uapi/linux directory?
Thanks!
thanks,
John Hubbard
On 10/17/24 10:28 AM, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote:
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote:
...
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
Yes.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Very nice!
Thanks!
I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory
Actually, I'm not running scripts, because the only time things need to change is when new selftests require a new include, or when something changes that selftests depend on.
tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here.
This is the first time I've actually looked at tools/include/linux. That sounds about right, though.
Or would you say the wrapper should regardless be in the uapi/linux directory?
No, not if there is already a better location, as you pointed out.
thanks,
On Thu, Oct 17, 2024 at 10:37:00AM -0700, John Hubbard wrote:
On 10/17/24 10:28 AM, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote:
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote:
...
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
Yes.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Very nice!
Thanks!
I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory
Actually, I'm not running scripts, because the only time things need to change is when new selftests require a new include, or when something changes that selftests depend on.
tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here.
This is the first time I've actually looked at tools/include/linux. That sounds about right, though.
Or would you say the wrapper should regardless be in the uapi/linux directory?
No, not if there is already a better location, as you pointed out.
OK perfect, I have a patch series ready to go with this (and addressing Christian's comments).
Shuah - if you are open to this approach then we should be good to go!
Thanks
thanks,
John Hubbard
On 10/17/24 11:38, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 10:37:00AM -0700, John Hubbard wrote:
On 10/17/24 10:28 AM, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote:
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote:
...
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
I like this solution. I should have read this message first before handling the others.
Yes.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Very nice!
Thanks!
I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory
Actually, I'm not running scripts, because the only time things need to change is when new selftests require a new include, or when something changes that selftests depend on.
tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here.
This is the first time I've actually looked at tools/include/linux. That sounds about right, though.
Or would you say the wrapper should regardless be in the uapi/linux directory?
No, not if there is already a better location, as you pointed out.
OK perfect, I have a patch series ready to go with this (and addressing Christian's comments).
Shuah - if you are open to this approach then we should be good to go!
I am caught up with the discussion now. I am good with this change.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
On Thu, Oct 17, 2024 at 01:37:06PM -0600, Shuah Khan wrote:
On 10/17/24 11:38, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 10:37:00AM -0700, John Hubbard wrote:
On 10/17/24 10:28 AM, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 10:17:54AM -0700, John Hubbard wrote:
On 10/17/24 5:06 AM, Lorenzo Stoakes wrote:
...
#ifndef __TOOLS_LINUX_PIDFD_H #define __TOOLS_LINUX_PIDFD_H
/* * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so * work around this by setting the header guard. */ #define _LINUX_FCNTL_H #include "../../../include/uapi/linux/pidfd.h" #undef _LINUX_FCNTL_H
#endif /* __TOOLS_LINUX_PIDFD_H */
Then the test code needs only to update the pidfd.h file to #include <linux/pidfd.h> and add a simple $(TOOLS_INCLUDES) to the CFLAGS += line in the pidfd self tests Makefile and we should be all good.
I like this solution. I should have read this message first before handling the others.
Thanks!
Yes.
That way we always import everything in this header correctly, we directly document this issue, we include the header as you would in userland and we should cover off all the issues?
Very nice!
Thanks!
I saw from your other thread the idea was to take snapshots and to run scripts to compare etc. but I suppose putting this into the known-stub directory
Actually, I'm not running scripts, because the only time things need to change is when new selftests require a new include, or when something changes that selftests depend on.
tools/include/linux rather than tools/include/uapi/linux would avoid a conflict here.
This is the first time I've actually looked at tools/include/linux. That sounds about right, though.
Or would you say the wrapper should regardless be in the uapi/linux directory?
No, not if there is already a better location, as you pointed out.
OK perfect, I have a patch series ready to go with this (and addressing Christian's comments).
Shuah - if you are open to this approach then we should be good to go!
I am caught up with the discussion now. I am good with this change.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org
Perfect thanks very much, I will send out the new version of the series with this applied, much appreciated! :)
thanks, -- Shuah
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests.
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
But seriously...user feedback is rare and valuable. We have some, to the effect of, "lose that requirement". And we also have an agreement, and an initial implementation in selftests/mm, on *how* to avoid it [2].
So...let's do it that way? Please?
[1] https://lore.kernel.org/lkml/20231103121652.GA6217@noisy.programming.kicks-a... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
thanks,
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests.
It's a typical response for good reason... :)
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
But seriously...user feedback is rare and valuable. We have some, to the effect of, "lose that requirement". And we also have an agreement, and an initial implementation in selftests/mm, on *how* to avoid it [2].
So...let's do it that way? Please?
I'd be happy to but we can't because the uapi header is just broken with this test due to the linux/fcntl.h vs. system header fcntl.h issue.
We could work around it by copying the header without the linux/fcntl.h include however...
[1] https://lore.kernel.org/lkml/20231103121652.GA6217@noisy.programming.kicks-a... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
thanks,
John Hubbard
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests.
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
Let me re-try this.
This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing.
This is absolute atrocious crap and is costing me valuable time.
Please fix this fucking selftests shit to just build. This is unusable garbage.
On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote:
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests.
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
Let me re-try this.
This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing.
This is absolute atrocious crap and is costing me valuable time.
Please fix this fucking selftests shit to just build. This is unusable garbage.
So after spending more time trying to remember how to debug Makefiles (I hate my life), I found that not only do I need this headers shit, the kvm selftests Makefile is actively broken if you use: make O=foo
-INSTALL_HDR_PATH = $(top_srcdir)/usr +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr
And then finally, I can do:
make O=foo headers_install make O=foo -C tools/testing/selftests/kvm/
So yeah, thank you very much for wasting my time *AGAIN*.
Seriously, I want to be able to do:
cd tools/testing/selftests/foo; make
and have it just work. I would strongly suggest every subsystem to reclaim their selftests and make it so again.
And on that, let me go merge the fixes I need to have x86 and futex build without this headers shit.
On Thu, May 01, 2025, Peter Zijlstra wrote:
On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote:
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
Let me re-try this.
This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing.
This is absolute atrocious crap and is costing me valuable time.
Please fix this fucking selftests shit to just build. This is unusable garbage.
So after spending more time trying to remember how to debug Makefiles (I hate my life), I found that not only do I need this headers shit, the kvm selftests Makefile is actively broken if you use: make O=foo
-INSTALL_HDR_PATH = $(top_srcdir)/usr +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr
And then finally, I can do:
make O=foo headers_install make O=foo -C tools/testing/selftests/kvm/
This doesn't actually work either, because for whatever reason, the selftests infrastructure uses OUTPUT, not O, for the output directory.
And the whole top_srcdir crud doesn't work if O/OUTPUT is completely out-of-tree, e.g. I use absolute paths that have nothing to do with the source tree.
I am more than happy to support any cleanup of KVM selftests, but I've more or less given up myself because so much of the ugliness is inhereted from selftests. I've resorted to hacked wrappers to make it work for my setup. E.g. I force KHDR_INCLUDES and INSTALL_HDR_PATH so that make doesn't try to grab usr/ files from the source tree.
On Thu, May 01, 2025 at 02:46:46PM +0200, Peter Zijlstra wrote:
On Thu, May 01, 2025 at 01:42:35PM +0200, Peter Zijlstra wrote:
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests.
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
Let me re-try this.
This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing.
This is absolute atrocious crap and is costing me valuable time.
Please fix this fucking selftests shit to just build. This is unusable garbage.
So after spending more time trying to remember how to debug Makefiles (I hate my life), I found that not only do I need this headers shit, the kvm selftests Makefile is actively broken if you use: make O=foo
-INSTALL_HDR_PATH = $(top_srcdir)/usr +INSTALL_HDR_PATH = $(top_srcdir)/$(O)/usr
And then finally, I can do:
make O=foo headers_install make O=foo -C tools/testing/selftests/kvm/
So yeah, thank you very much for wasting my time *AGAIN*.
Seriously, I want to be able to do:
cd tools/testing/selftests/foo; make
and have it just work. I would strongly suggest every subsystem to reclaim their selftests and make it so again.
And on that, let me go merge the fixes I need to have x86 and futex build without this headers shit.
I'm completely lost as to what's happening here or whether the test here is somehow at fault for something.
The pidfd.h head explicitly has no dependency on the pidfd uapi header itself and I will NAK anything that makes it so. It's just a giant pain.
On Mon, May 05, 2025 at 03:35:13PM +0200, Christian Brauner wrote:
I'm completely lost as to what's happening here or whether the test here is somehow at fault for something.
The pidfd.h head explicitly has no dependency on the pidfd uapi header itself and I will NAK anything that makes it so. It's just a giant pain.
There was a debate in my series here about my having to make things work with 'make headers', but thanks to John we resolved it the sane way by using the tools/ stuff.
I _believe_ Peter is just using this thread as an example of a recent case of people being asked to do this insanity (correct me if I'm wrong Peter) and it's unrelated to pidfs in general.
On 5/1/25 05:42, Peter Zijlstra wrote:
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Right but that assumes you install the kernel headers on the build system, which is quite a painful thing to have to do when you are quickly iterating on a qemu setup.
This is a use case I use all the time so not at all theoretical.
This is turning out to be a fairly typical reaction from kernel developers, when presented with the "you must first run make headers" requirement for kselftests.
Peter Zijlstra's "NAK NAK NAK" response [1] last year was the most colorful, so I'll helpfully cite it here. :)
Let me re-try this.
This is driving me insane. I've spend the past _TWO_ days trying to build KVM selftests and I'm still failing.
This is absolute atrocious crap and is costing me valuable time.
Please fix this fucking selftests shit to just build. This is unusable garbage.
I don't recall all the reasons why kselftests needed "make headers" One reason I could think of was that when a new test depends on a header change, the test won't build unless headers are installed.
If this requirement is causing problems for tests that don't fall into the category and we probably have more of them mow, we can clean that up.
John, you mentioned you got mm tests working without headers? Can you share the commit here.
thanks, -- Shuah
On 5/6/25 2:18 PM, Shuah Khan wrote:
On 5/1/25 05:42, Peter Zijlstra wrote:
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
Please fix this fucking selftests shit to just build. This is unusable garbage.
I don't recall all the reasons why kselftests needed "make headers" One reason I could think of was that when a new test depends on a header change, the test won't build unless headers are installed.
...or until an updated copy of that updated header file is copied somewhere, and then included in the kselftests. That's the approach that I ultimately settled upon, after some discussion and negotion.
Details below.
If this requirement is causing problems for tests that don't fall into the category and we probably have more of them mow, we can clean that up.
John, you mentioned you got mm tests working without headers? Can you share the commit here.
Yes. This one sets up the general approach, which is available to all kselftests: TOOLS_INCLUDES. It also changes selftests/mm to set TOOLS_INCLUDES in that build:
e076eaca5906 ("selftests: break the dependency upon local header files")
And here is a representative application of the above, to selftests/mm. In other words, taking advantage of the new file location pointed to by TOOLS_INCLUDES:
580ea358af0a ("selftests/mm: fix additional build errors for selftests")
thanks,
On 5/6/25 15:34, John Hubbard wrote:
On 5/6/25 2:18 PM, Shuah Khan wrote:
On 5/1/25 05:42, Peter Zijlstra wrote:
On Wed, Oct 16, 2024 at 07:14:34PM -0700, John Hubbard wrote:
On 10/16/24 3:06 PM, Lorenzo Stoakes wrote:
On Wed, Oct 16, 2024 at 02:00:27PM -0600, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
Please fix this fucking selftests shit to just build. This is unusable garbage.
Peter, John,
There seems to be confusion regarding KHDR_INCLUDES. Tests don't have to use KHDR_INCLUDES if they don't want to.
There are 4623 test Makefiles (excluding the main Makefile) under selftests/. Out of those 73 Makefiles reference KHDR_INCLUDES exported by lib.mk and selftests/Makefile. The rest are happy with system headers.
The support for this KHDR_INCLUDES was added just for the case when a new test depends on header change. This is the reason why only a few test Makefiles use it. When test rings ran into issues related to dependencies between header changes, we recommended installing headers to solve the problem and introduced KHDR_INCLUDES so test Makefiles can use it in their Makefiles overriding the framework defaults.
If your test doesn't need it, you can simply stop referencing it or use the approach used in mm test.
It is a manual step. Works well for developers who know what they are doing. This isn't ideal for test rings. This isn't an ideal solution really. It works for the mm developers.
# In order to use newer items that haven't yet been added to the user's system # header files, add $(TOOLS_INCLUDES) to the compiler invocation in each # each selftest. # You may need to add files to that location, or to refresh an existing file. In # order to do that, run "make headers" from $(top_srcdir), then copy the # header file that you want from $(top_srcdir)/usr/include/... , to the matching # subdir in $(TOOLS_INCLUDE). TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
The issues Peter is seeing regarding KHDR_INCLUDES in the following tests can be easily fixed by simply changing the test Makefile. These aren't framework related.
kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) x86/Makefile:CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) futex/functional/Makefile:INCLUDES := -I../include -I../../ $(KHDR_INCLUDES) futex/functional/Makefile:CFLAGS := $(CFLAGS) -g -O2 -Wall -pthread $(INCLUDES) $(KHDR_INCLUDES)
You can make the change to remove the reference to KHDR_INCLUDES. If don't have the time/bandwidth to do it, I will take care of it.
If test build fails, you can then figure out how to address that. Hopefully build issues related to header changes are infrequent.
thanks, -- Shuah
On 10/16/24 1:00 PM, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Wait, what?! Noooo!
Hi, Shuah! :)
We have had this conversation before. And there were fireworks coming from various core kernel developers who found that requirement to be unacceptable.
And in response, I made at selftests/mm tests buildable *without* requiring a "make headers" first, in [1].
I haven't followed up with other subsystems, but...maybe I should. Because otherwise we're just going to keep having this discussion.
The requirement to do "make headers" is not a keeper. Really.
These local make it difficult to maintain these tests in the longer term. Somebody has to go clean these up later.
There are other approaches to making things work. Again, please see [1].
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
thanks,
On 10/16/24 20:01, John Hubbard wrote:
On 10/16/24 1:00 PM, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..1640b711889b 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -50,6 +50,14 @@ #define PIDFD_NONBLOCK O_NONBLOCK #endif +/* System header file may not have this available. */ +#ifndef PIDFD_SELF_THREAD +#define PIDFD_SELF_THREAD -100 +#endif +#ifndef PIDFD_SELF_THREAD_GROUP +#define PIDFD_SELF_THREAD_GROUP -200 +#endif
As mentioned in my response to v1 patch:
kselftest has dependency on "make headers" and tests include headers from linux/ directory
Wait, what?! Noooo!
Hi, Shuah! :)
We have had this conversation before. And there were fireworks coming from various core kernel developers who found that requirement to be unacceptable.
And in response, I made at selftests/mm tests buildable *without* requiring a "make headers" first, in [1].
I haven't followed up with other subsystems, but...maybe I should. Because otherwise we're just going to keep having this discussion.
The requirement to do "make headers" is not a keeper. Really.
The reason we added the requirement to avoid duplicate defines such as this one added to kselftest source files. These are error prone and hard to resolve.
In some cases, these don't become uapi and don't make it into system headers. selftests are in a category of depending on kernel headers to be able to test some features.
Getting rid of this dependency mean, tests will be full of local defines such as this one which will become unmanageable overtime.
The discussion should be: "How do we get rid of the dependency without introducing local defines?" not just "Let's get rid of the dependency"
thanks, -- Shuah
On 10/17/24 9:33 AM, Shuah Khan wrote:
On 10/16/24 20:01, John Hubbard wrote:
On 10/16/24 1:00 PM, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
The requirement to do "make headers" is not a keeper. Really.
The reason we added the requirement to avoid duplicate defines such as this one added to kselftest source files. These are error prone and hard to resolve.
In some cases, these don't become uapi and don't make it into system headers. selftests are in a category of depending on kernel headers to be able to test some features.
Getting rid of this dependency mean, tests will be full of local defines such as this one which will become unmanageable overtime.
Not if we do it correctly...Please do look at the reference I provided for how that works. Here is is again: [1].
The basic idea, which has been discussed and reviewed, is to take very occasional snapshots and drop them into a static location where they are available for kselftests, without disurbing other things: $(top_srcdir)/tools/include/uapi
This has worked well so far.
The discussion should be: "How do we get rid of the dependency without introducing local defines?" not just "Let's get rid of the dependency"
Yes. Good. We are apparently in violent agreement, because a few lines above, I wrote:
The requirement to do "make headers" is not a keeper.
The "make headers" is the problem, not the fact that we need to depend on various includes. And so the solution stops requiring "make headers". It gets the includes from a less volatile location.
Yes?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
thanks,
On 10/17/24 10:47, John Hubbard wrote:
On 10/17/24 9:33 AM, Shuah Khan wrote:
On 10/16/24 20:01, John Hubbard wrote:
On 10/16/24 1:00 PM, Shuah Khan wrote:
On 10/16/24 04:20, Lorenzo Stoakes wrote:
...
The requirement to do "make headers" is not a keeper. Really.
The reason we added the requirement to avoid duplicate defines such as this one added to kselftest source files. These are error prone and hard to resolve.
In some cases, these don't become uapi and don't make it into system headers. selftests are in a category of depending on kernel headers to be able to test some features.
Getting rid of this dependency mean, tests will be full of local defines such as this one which will become unmanageable overtime.
Not if we do it correctly...Please do look at the reference I provided for how that works. Here is is again: [1].
The basic idea, which has been discussed and reviewed, is to take very occasional snapshots and drop them into a static location where they are available for kselftests, without disurbing other things: $(top_srcdir)/tools/include/uapi
This has worked well so far.
The discussion should be: "How do we get rid of the dependency without introducing local defines?" not just "Let's get rid of the dependency"
Yes. Good. We are apparently in violent agreement, because a few lines above, I wrote:
The requirement to do "make headers" is not a keeper.
The "make headers" is the problem, not the fact that we need to depend on various includes. And so the solution stops requiring "make headers". It gets the includes from a less volatile location.
Yes?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
thanks,
Posting this on this thread as well -
Peter, John,
There seems to be confusion regarding KHDR_INCLUDES. Tests don't have to use KHDR_INCLUDES if they don't want to.
There are 4623 test Makefiles (excluding the main Makefile) under selftests/. Out of those 73 Makefiles reference KHDR_INCLUDES exported by lib.mk and selftests/Makefile. The rest are happy with system headers.
The support for this KHDR_INCLUDES was added just for the case when a new test depends on header change. This is the reason why only a few test Makefiles use it. When test rings ran into issues related to dependencies between header changes, we recommended installing headers to solve the problem and introduced KHDR_INCLUDES so test Makefiles can use it in their Makefiles overriding the framework defaults.
If your test doesn't need it, you can simply stop referencing it or use the approach used in mm test.
It is a manual step. Works well for developers who know what they are doing. This isn't ideal for test rings. This isn't an ideal solution really. It works for the mm developers.
# In order to use newer items that haven't yet been added to the user's system # header files, add $(TOOLS_INCLUDES) to the compiler invocation in each # each selftest. # You may need to add files to that location, or to refresh an existing file. In # order to do that, run "make headers" from $(top_srcdir), then copy the # header file that you want from $(top_srcdir)/usr/include/... , to the matching # subdir in $(TOOLS_INCLUDE). TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
The issues Peter is seeing regarding KHDR_INCLUDES in the following tests can be easily fixed by simply changing the test Makefile. These aren't framework related.
kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) x86/Makefile:CFLAGS := -O2 -g -std=gnu99 -pthread -Wall $(KHDR_INCLUDES) futex/functional/Makefile:INCLUDES := -I../include -I../../ $(KHDR_INCLUDES) futex/functional/Makefile:CFLAGS := $(CFLAGS) -g -O2 -Wall -pthread $(INCLUDES) $(KHDR_INCLUDES)
You can make the change to remove the reference to KHDR_INCLUDES. If don't have the time/bandwidth to do it, I will take care of it.
If test build fails, you can then figure out how to address that. Hopefully build issues related to header changes are infrequent.
thanks, -- Shuah
On Wed, May 07, 2025, Shuah Khan wrote:
The issues Peter is seeing regarding KHDR_INCLUDES in the following tests can be easily fixed by simply changing the test Makefile. These aren't framework related.
kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
...
You can make the change to remove the reference to KHDR_INCLUDES. If don't have the time/bandwidth to do it, I will take care of it.
Please don't remove the KHDR_INCLUDES usage in KVM's selftests, KVM routinely adds tests for new uAPI. Having to manually install headers is annoying, but IMO it's the least awful solution we have.
On 5/8/25 08:04, Sean Christopherson wrote:
On Wed, May 07, 2025, Shuah Khan wrote:
The issues Peter is seeing regarding KHDR_INCLUDES in the following tests can be easily fixed by simply changing the test Makefile. These aren't framework related.
kvm/Makefile.kvm: -I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
...
You can make the change to remove the reference to KHDR_INCLUDES. If don't have the time/bandwidth to do it, I will take care of it.
Please don't remove the KHDR_INCLUDES usage in KVM's selftests, KVM routinely adds tests for new uAPI. Having to manually install headers is annoying, but IMO it's the least awful solution we have.
Thank you for confirming that KHDR_INCLUDES customization is necessary for some tests such as kvm.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
Christian Brauner -
John Hubbard -
Lorenzo Stoakes -
Peter Zijlstra -
Sean Christopherson -
Shuah Khan