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.
v4: * Avoid returning an fd in the __pidfd_get_pid() function as pointed out by Christian, instead simply always pin the pid and maintain fd scope in the helper alone. * Add wrapper header file in tools/include/linux to allow for import of UAPI pidfd.h header without encountering the collision between system fcntl.h and linux/fcntl.h as discussed with Shuah and John. * Fixup tests to import the UAPI pidfd.h header working around conflicts between system fcntl.h and linux/fcntl.h which the UAPI pidfd.h imports, as reported by Shuah. * Use an int for pidfd_is_self_sentinel() to avoid any dependency on stdbool.h in userland.
v3: * Do not fput() an invalid fd as reported by kernel test bot. * Fix unintended churn from moving variable declaration. https://lore.kernel.org/linux-mm/cover.1729073310.git.lorenzo.stoakes@oracle...
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 (4): pidfd: extend pidfd_get_pid() and de-duplicate pid lookup pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process selftests: pidfd: add pidfd.h UAPI wrapper selftests: pidfd: add tests for PIDFD_SELF_*
include/linux/pid.h | 34 ++++- include/uapi/linux/pidfd.h | 15 ++ kernel/exit.c | 3 +- kernel/nsproxy.c | 1 + kernel/pid.c | 65 +++++--- kernel/signal.c | 29 +--- tools/include/linux/pidfd.h | 14 ++ tools/testing/selftests/pidfd/Makefile | 3 +- tools/testing/selftests/pidfd/pidfd.h | 2 + .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 12 files changed, 333 insertions(+), 61 deletions(-) create mode 100644 tools/include/linux/pidfd.h
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 narrows this to one approach of pinning the pid, with an optional output parameters for file->f_flags to avoid the need to hold onto a file to retrieve this.
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 | 30 +++++++++++++++++++++++++++++- kernel/pid.c | 42 ++++++++++++++++++++++++------------------ kernel/signal.c | 29 ++++++----------------------- 3 files changed, 59 insertions(+), 42 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index a3aad9b4074c..d466890e1b35 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,35 @@ 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. + * @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. + * + * Returns: If successful, the pid associated with the pidfd, otherwise an + * error. + */ +struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc, + unsigned int *flags); + +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags) +{ + return __pidfd_get_pid(pidfd, /* allow_proc = */ false, flags); +} + +static inline struct pid *pidfd_get_pid_proc(unsigned int pidfd, + unsigned int *flags) +{ + return __pidfd_get_pid(pidfd, /* allow_proc = */ true, flags); +} + 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..94c97559e5c5 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,32 @@ 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 allow_proc, + unsigned int *flags) { - struct fd f; struct pid *pid; + struct fd f = fdget(pidfd); + struct file *file = fd_file(f);
- f = fdget(fd); - if (!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; }
+ /* Pin pid before we release fd. */ + get_pid(pid); + if (flags) + *flags = file->f_flags; fdput(f); + return pid; }
@@ -747,23 +758,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..9a35b1cf40ad 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) @@ -3908,10 +3897,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, siginfo_t __user *, info, unsigned int, flags) { int ret; - struct fd f; 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 +3910,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_get_pid_proc(pidfd, &f_flags); + if (IS_ERR(pid)) + return PTR_ERR(pid);
ret = -EINVAL; if (!access_pidfd_pidns(pid)) @@ -3939,7 +3922,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; @@ -3978,7 +3961,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, else ret = kill_pid_info_type(sig, &kinfo, pid, type); err: - fdput(f); + put_pid(pid); return ret; }
On Thu, Oct 17, 2024 at 10:05:49PM GMT, Lorenzo Stoakes wrote:
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 narrows this to one approach of pinning the pid, with an optional output parameters for file->f_flags to avoid the need to hold onto a file to retrieve this.
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
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev
It is useful to be able to utilise the pidfd mechanism 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.
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 | 8 ++++-- include/uapi/linux/pidfd.h | 15 +++++++++++ kernel/exit.c | 3 ++- kernel/nsproxy.c | 1 + kernel/pid.c | 51 ++++++++++++++++++++++++-------------- 5 files changed, 57 insertions(+), 21 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h index d466890e1b35..3b2ac7567a88 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h @@ -78,11 +78,15 @@ 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. + * @alloc_proc is also set, or PIDFD_SELF_* to refer to the current + * thread or thread group leader. * @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 or If PIDFD_SELF_THREAD is set, this is + * set to PIDFD_THREAD, otherwise if PIDFD_SELF_THREAD_GROUP then + * this is set to zero. * * 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..0ca2ebf906fd 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 int 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 94c97559e5c5..8742157b36f8 100644 --- a/kernel/pid.c +++ b/kernel/pid.c @@ -535,33 +535,48 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns) } EXPORT_SYMBOL_GPL(find_ge_pid);
+static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags) +{ + bool is_thread = pidfd == PIDFD_SELF_THREAD; + enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID; + struct pid *pid = *task_pid_ptr(current, type); + + /* The caller expects an elevated reference count. */ + get_pid(pid); + return pid; +} + struct pid *__pidfd_get_pid(unsigned int pidfd, bool allow_proc, unsigned int *flags) { - struct pid *pid; - struct fd f = fdget(pidfd); - struct file *file = fd_file(f); + if (pidfd_is_self_sentinel(pidfd)) { + return pidfd_get_pid_self(pidfd, flags); + } else { + struct pid *pid; + struct fd f = fdget(pidfd); + struct file *file = fd_file(f);
- if (!file) - return ERR_PTR(-EBADF); + 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); + 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)) { + if (IS_ERR(pid)) { + fdput(f); + return pid; + } + + /* Pin pid before we release fd. */ + get_pid(pid); + if (flags) + *flags = file->f_flags; fdput(f); + return pid; } - - /* Pin pid before we release fd. */ - get_pid(pid); - if (flags) - *flags = file->f_flags; - fdput(f); - - return pid; }
/**
On Thu, Oct 17, 2024 at 10:05:50PM +0100, Lorenzo Stoakes wrote:
It is useful to be able to utilise the pidfd mechanism 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.
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.
We need a:
Suggested-by: Pedro Falcato pedro.falcato@gmail.com
here, will add if respin, otherwise could you add Christian?
My apologies Pedro, this was wholly an oversight and my mistake!
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
On Thu, Oct 17, 2024 at 10:05:50PM GMT, Lorenzo Stoakes wrote:
It is useful to be able to utilise the pidfd mechanism 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.
Should users use PIDFD_SELF_PROCESS in process_madvise() for self madvise() (once the support is added)?
[...]
+static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags) +{
- bool is_thread = pidfd == PIDFD_SELF_THREAD;
- enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
- struct pid *pid = *task_pid_ptr(current, type);
- /* The caller expects an elevated reference count. */
- get_pid(pid);
Do you want this helper to work for scenarios where pid is used across context? Otherwise can't we get rid of this get and later put for self?
- return pid;
+}
Overall looks good to me.
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev
On Tue, Oct 22, 2024 at 05:53:00PM -0700, Shakeel Butt wrote:
On Thu, Oct 17, 2024 at 10:05:50PM GMT, Lorenzo Stoakes wrote:
It is useful to be able to utilise the pidfd mechanism 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.
Should users use PIDFD_SELF_PROCESS in process_madvise() for self madvise() (once the support is added)?
You can use either it will make no difference as both will get you to current->mm which has to be shared. So I'd go with PIDFD_SELF for brevity :)
This series and the prerequisites I already added to process_madvise() already provide support so with this in you can just use this for that.
[...]
+static struct pid *pidfd_get_pid_self(unsigned int pidfd, unsigned int *flags) +{
- bool is_thread = pidfd == PIDFD_SELF_THREAD;
- enum pid_type type = is_thread ? PIDTYPE_PID : PIDTYPE_TGID;
- struct pid *pid = *task_pid_ptr(current, type);
- /* The caller expects an elevated reference count. */
- get_pid(pid);
Do you want this helper to work for scenarios where pid is used across context? Otherwise can't we get rid of this get and later put for self?
Yeah I hate doing this but it's to keep things as generic as possible and to ensure that no user of this logic accidentally drops the reference count unintentionally + to reduce churn.
I think doing it this way is better than trying to special case the put, and in any case if you did the 'old' way of referencing yourself you'd have ended up doing this anyway so it's at least no delta!
- return pid;
+}
Overall looks good to me.
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev
Thanks!
On Wed, Oct 23, 2024 at 08:18:35AM GMT, Lorenzo Stoakes wrote:
On Tue, Oct 22, 2024 at 05:53:00PM -0700, Shakeel Butt wrote:
On Thu, Oct 17, 2024 at 10:05:50PM GMT, Lorenzo Stoakes wrote:
It is useful to be able to utilise the pidfd mechanism 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.
Should users use PIDFD_SELF_PROCESS in process_madvise() for self madvise() (once the support is added)?
You can use either it will make no difference as both will get you to current->mm which has to be shared. So I'd go with PIDFD_SELF for brevity :)
This series and the prerequisites I already added to process_madvise() already provide support so with this in you can just use this for that.
Thanks a lot, this is awesome. Is the plan for this series to go through mm-tree or through Christian?
On Wed, Oct 23, 2024 at 10:18:28AM -0700, Shakeel Butt wrote:
On Wed, Oct 23, 2024 at 08:18:35AM GMT, Lorenzo Stoakes wrote:
On Tue, Oct 22, 2024 at 05:53:00PM -0700, Shakeel Butt wrote:
On Thu, Oct 17, 2024 at 10:05:50PM GMT, Lorenzo Stoakes wrote:
It is useful to be able to utilise the pidfd mechanism 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.
Should users use PIDFD_SELF_PROCESS in process_madvise() for self madvise() (once the support is added)?
You can use either it will make no difference as both will get you to current->mm which has to be shared. So I'd go with PIDFD_SELF for brevity :)
This series and the prerequisites I already added to process_madvise() already provide support so with this in you can just use this for that.
Thanks a lot, this is awesome. Is the plan for this series to go through mm-tree or through Christian?
Thanks!
I am assuming this will go through Christian's tree as entirely within the pidfd realm :)
Christian - I am assuming this is your expectation too right?
I cc'd linux-mm more for convenience as obviously am hoping to use this for an mm thing myself.
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to tools/include/ which sets the linux/fcntl.h header guard ahead of importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and put it at a higher precidence than any make header installed headers to ensure the wrapper is preferred.
This way we can directly import the UAPI header file without issue, use the latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com --- tools/include/linux/pidfd.h | 14 ++++++++++++++ tools/testing/selftests/pidfd/Makefile | 3 +-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h new file mode 100644 index 000000000000..113c8023072d --- /dev/null +++ b/tools/include/linux/pidfd.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#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 */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \ pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test
include ../lib.mk -
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to tools/include/ which sets the linux/fcntl.h header guard ahead of importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and put it at a higher precidence than any make header installed headers to ensure the wrapper is preferred.
...but we are not actually using the installed headers, now. And we intend to continue avoiding them. So the ordering shouldn't matter. More below:
This way we can directly import the UAPI header file without issue, use the latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/include/linux/pidfd.h | 14 ++++++++++++++ tools/testing/selftests/pidfd/Makefile | 3 +-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h new file mode 100644 index 000000000000..113c8023072d --- /dev/null +++ b/tools/include/linux/pidfd.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */
+#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
Oh shoot, I think you, Shuah and I were referring to different uapi locations, the whole time. And so the basic approach is different after all.
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h
...but what I was intending was to copy a snapshot of that file (or a snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.h
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk, for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
The reasoning for this directory is further explained here:
tools/include/uapi/README
(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's progress.)
And now, it's possible to change fcntl.h in place, instead of using a wrapper. Although either way seems OK to me. (I'm sort of ignoring the details of the actual header file conflict itself, for now.)
+#endif /* _TOOLS_LINUX_PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile, which is also helpful, because eventually this can be factored into a common piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
I apologize for just now noticing this! And these kselftests shouldn't require so much fussing around, I know. But once we get this just right, it will work well and last a long time. :)
thanks,
On 10/17/24 15:45, John Hubbard wrote:
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to tools/include/ which sets the linux/fcntl.h header guard ahead of importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and put it at a higher precidence than any make header installed headers to ensure the wrapper is preferred.
...but we are not actually using the installed headers, now. And we intend to continue avoiding them. So the ordering shouldn't matter. More below:
This way we can directly import the UAPI header file without issue, use the latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/include/linux/pidfd.h | 14 ++++++++++++++ tools/testing/selftests/pidfd/Makefile | 3 +-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h new file mode 100644 index 000000000000..113c8023072d --- /dev/null +++ b/tools/include/linux/pidfd.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */
+#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
Oh shoot, I think you, Shuah and I were referring to different uapi locations, the whole time. And so the basic approach is different after all.
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h
Correct. I am glad we are on the same page now.
...but what I was intending was to copy a snapshot of that file (or a snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.h
So why do the copy and snapshot. Anytime you build userspace in the reoo - you will need to run "make headers: whether you install them under tools/include or include.
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk, for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
Yes some tests do include that.
The reasoning for this directory is further explained here:
tools/include/uapi/README
(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's progress.)
Yes the same problems apply here - what complicates this more is selftests are supposed to test kernel changes, hence the need to include latest kernel headers. The simple solution is adding a dependency so we don't have to duplicate the headers. I don't believe the perf solution works here. We will have to figure out a solution.
And now, it's possible to change fcntl.h in place, instead of using a wrapper. Although either way seems OK to me. (I'm sort of ignoring the details of the actual header file conflict itself, for now.)
+#endif /* _TOOLS_LINUX_PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile, which is also helpful, because eventually this can be factored into a common piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
KHDR_INCLUDES is "make headers" location under the root directory. So what happens when you add TOOLS_INCLUDES to it.
Does "make kselftest-all" work as it is supposed to? If it and all tests can build then I am all for it.
I apologize for just now noticing this! And these kselftests shouldn't require so much fussing around, I know. But once we get this just right, it will work well and last a long time. :)
On the contrary if we don't discuss/fuss and get this right, we have to deal with changes like adding local defines and adhoc approaches in individual tests - that is one reason we made the "make headers" as a dependency. I would like to solve the problem of proliferation of local defines and even system calls in some cases.
For now I am going let this patch go through as it is important to add tests.
My goals are simple:
- no local defines unless it is abslulutely necessary - be able to build tests that add coverage for new kernel api and features before we release the kernel. - make it easier for CIs to build and run tests - continue to have tests works for kernel developres e.g: mm developers build tests in mm directory. They don't see the issues that crop up in CIs or running the entire kselftest default run like CIs do.
Adhoc changes break some use-cases.
thanks, -- Shuah
On 10/17/24 3:11 PM, Shuah Khan wrote:
On 10/17/24 15:45, John Hubbard wrote:
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to tools/include/ which sets the linux/fcntl.h header guard ahead of importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and put it at a higher precidence than any make header installed headers to ensure the wrapper is preferred.
...but we are not actually using the installed headers, now. And we intend to continue avoiding them. So the ordering shouldn't matter. More below:
This way we can directly import the UAPI header file without issue, use the latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/include/linux/pidfd.h | 14 ++++++++++++++ tools/testing/selftests/pidfd/Makefile | 3 +-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h new file mode 100644 index 000000000000..113c8023072d --- /dev/null +++ b/tools/include/linux/pidfd.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */
+#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
Oh shoot, I think you, Shuah and I were referring to different uapi locations, the whole time. And so the basic approach is different after all.
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h
Correct. I am glad we are on the same page now.
...but what I was intending was to copy a snapshot of that file (or a snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.h
So why do the copy and snapshot. Anytime you build userspace in the reoo - you will need to run "make headers: whether you install them under tools/include or include.
No, you only do "make headers" once, and that's a temporary thing too. After the author of the new selftest runs "make headers", the author will typically remove those headers, in order to verify that the selftests still build without the kernel generated header files.
That's it! No one else has to deal with it. And that's what is being asked for here.
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk, for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapi
Yes some tests do include that.
The reasoning for this directory is further explained here:
tools/include/uapi/README
(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's progress.)
Yes the same problems apply here - what complicates this more is selftests are supposed to test kernel changes, hence the need to include latest kernel headers. The simple solution is adding a dependency so we don't have to duplicate the headers. I don't believe the perf solution works here. We will have to figure out a solution.
That's the key point: the fact that the selftests are intended to test kernel changes does *not* mean that we have to include the dynamically generated header files. That's going further than necessary. And it leads to problems and complaints for kernel developers, who are really one of the main user groups here.
And now, it's possible to change fcntl.h in place, instead of using a wrapper. Although either way seems OK to me. (I'm sort of ignoring the details of the actual header file conflict itself, for now.)
+#endif /* _TOOLS_LINUX_PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile, which is also helpful, because eventually this can be factored into a common piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
KHDR_INCLUDES is "make headers" location under the root directory. So what happens when you add TOOLS_INCLUDES to it.
Does "make kselftest-all" work as it is supposed to? If it and all tests can build then I am all for it.
I apologize for just now noticing this! And these kselftests shouldn't require so much fussing around, I know. But once we get this just right, it will work well and last a long time. :)
On the contrary if we don't discuss/fuss and get this right, we have to deal with changes like adding local defines and adhoc approaches in individual tests - that is one reason we made the "make headers" as a dependency. I would like to solve the problem of proliferation of local defines and even system calls in some cases.
Yes, let's work through this.
For now I am going let this patch go through as it is important to add tests.
Sure, we can iterate on it as necessary.
My goals are simple:
- no local defines unless it is abslulutely necessary
Agreed.
- be able to build tests that add coverage for new kernel
api and features before we release the kernel.
- make it easier for CIs to build and run tests
Up to a point! And that point should be somewhere *before* it inflicts extra pain and annoyance on the kernel developers.
Let's not let CI drive this all by itself. CI is not writing these tests, and furthermore it can adapt a little bit if necessary.
But even more to the point, avoiding "make headers" simplifies CI, rather than complicating it.
- continue to have tests works for kernel developres
e.g: mm developers build tests in mm directory. They don't see the issues that crop up in CIs or running the entire kselftest default run like CIs do.
Adhoc changes break some use-cases.
thanks, -- Shuah
thanks,
On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to tools/include/ which sets the linux/fcntl.h header guard ahead of importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and put it at a higher precidence than any make header installed headers to ensure the wrapper is preferred.
...but we are not actually using the installed headers, now. And we intend to continue avoiding them. So the ordering shouldn't matter. More below:
This way we can directly import the UAPI header file without issue, use the latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com
tools/include/linux/pidfd.h | 14 ++++++++++++++ tools/testing/selftests/pidfd/Makefile | 3 +-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h new file mode 100644 index 000000000000..113c8023072d --- /dev/null +++ b/tools/include/linux/pidfd.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0-only */
+#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
Oh shoot, I think you, Shuah and I were referring to different uapi locations, the whole time. And so the basic approach is different after all.
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h...but what I was intending was to copy a snapshot of that file (or a snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.h
Yeah my first version of this used the uapi one but I thought doing that might conflict with snapshotting? Also it'd mean you'd absolutely have to have the $(TOOLS_INCLUDES) earlier in the include priority list and better maybe to special case in this instance.
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk, for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapiThe reasoning for this directory is further explained here:
tools/include/uapi/README(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's progress.)
And now, it's possible to change fcntl.h in place, instead of using a wrapper. Although either way seems OK to me. (I'm sort of ignoring the details of the actual header file conflict itself, for now.)
The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known horror show. It's a difficult one to resolve as the UAPI pidfd.h header needs O_xxx defines but we also need to include this header in kernel code.
An #ifdef __KERNEL__ block might be a solution here but fixing that is out of scope for these changes.
+#endif /* _TOOLS_LINUX_PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile, which is also helpful, because eventually this can be factored into a common piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -WallI apologize for just now noticing this! And these kselftests shouldn't require so much fussing around, I know. But once we get this just right, it will work well and last a long time. :)
Yeah I know, but this won't work due to the header conflict, I was doing this previously.
Also doing it this way means that uapi snapshot doesn't override the usr/ one if you have that, which I guess you want?
thanks,
John Hubbard
On 10/17/24 11:49 PM, Lorenzo Stoakes wrote:
On Thu, Oct 17, 2024 at 02:45:43PM -0700, John Hubbard wrote:
On 10/17/24 2:05 PM, Lorenzo Stoakes wrote:
...
Your include path above actually refers to:
$(top_srcdir)/include/uapi/linux/fcntl.h...but what I was intending was to copy a snapshot of that file (or a snapshot from the one generated by "make headers"), to here:
$(top_srcdir)/tools/include/uapi/linux/fcntl.hYeah my first version of this used the uapi one but I thought doing that might conflict with snapshotting? Also it'd mean you'd absolutely have to have the $(TOOLS_INCLUDES) earlier in the include priority list and better maybe to special case in this instance.
Actually, I think the goal is to just stop using KHDR_INCLUDES (./usr/include) entirely!
More below...
...and then use $(TOOLS_INCLUDES), which is already in selftests/lib.mk, for that reason: to be available to all of the kselftests:
TOOLS_INCLUDES := -isystem $(top_srcdir)/tools/include/uapiThe reasoning for this directory is further explained here:
tools/include/uapi/README(And I see that selftests/proc has started using $(TOOLS_INCLUDES), that's progress.)
And now, it's possible to change fcntl.h in place, instead of using a wrapper. Although either way seems OK to me. (I'm sort of ignoring the details of the actual header file conflict itself, for now.)
The fcntl.h and linux/fcntl.h conflict is apparently a rather well-known horror show. It's a difficult one to resolve as the UAPI pidfd.h header needs O_xxx defines but we also need to include this header in kernel code.
An #ifdef __KERNEL__ block might be a solution here but fixing that is out of scope for these changes.
Certainly out of scope! Your patch already avoids the biggest issue: it no longer requires "make headers", in order to build it. That's fine for now. Sorry to put you into the middle of a pre-existing kselftests debate.
And the #ifdef __KERNEL__ sounds like a potential solution, or at least a building block for one. I need to take a closer look at this particular header file mess, the fcntl.h situation is new to me.
+#endif /* _TOOLS_LINUX_PIDFD_H */ diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index d731e3e76d5b..f5038c9dae14 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,8 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only -CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall +CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
Instead, it would look like this, which now mostly matches selftests/mm/Makefile, which is also helpful, because eventually this can be factored into a common piece for all selftests:
CFLAGS += -g -isystem $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -WallI apologize for just now noticing this! And these kselftests shouldn't require so much fussing around, I know. But once we get this just right, it will work well and last a long time. :)
Yeah I know, but this won't work due to the header conflict, I was doing this previously.
Also doing it this way means that uapi snapshot doesn't override the usr/ one if you have that, which I guess you want?
Actually, given that we want (or should want, so I claim) to build without first running "make headers", and given that "make headers" populates ./usr/include/, which in turn is what $(KHDR_INCLUDES) points to, this means that eventually we should end up with approximately:
CFLAGS += -g -isystem $(TOOLS_INCLUDES) -pthread -Wall
And I just checked, today's selftests/mm builds just fine, with a similar diff applied, so I'm not totally crazy:
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 02e1204971b0..b004a8edcba5 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -33,7 +33,7 @@ endif # LDLIBS. MAKEFLAGS += --no-builtin-rules
-CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES) +CFLAGS = -Wall -I $(top_srcdir) $(EXTRA_CFLAGS) $(TOOLS_INCLUDES) LDLIBS = -lrt -lpthread -lm
TEST_GEN_FILES = cow
thanks,
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).
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com --- tools/testing/selftests/pidfd/pidfd.h | 2 + .../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, 218 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..cbe1a2be3cec 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -16,6 +16,8 @@ #include <sys/types.h> #include <sys/wait.h>
+#include <linux/pidfd.h> + #include "../kselftest.h"
#ifndef P_PIDFD 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; }
Hello,
kernel test robot noticed "kernel-selftests.cgroup.make.fail" on:
commit: 930cb1423ee2522760ffde43455b14df5c0d5487 ("[PATCH v4 4/4] selftests: pidfd: add tests for PIDFD_SELF_*") url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-... base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next patch link: https://lore.kernel.org/all/b9851fa9f87d22f352f960b847d99459ef7d74a1.1729198... patch subject: [PATCH v4 4/4] selftests: pidfd: add tests for PIDFD_SELF_*
in testcase: kernel-selftests version: kernel-selftests-x86_64-977d51cf-1_20240508 with following parameters:
group: cgroup
config: x86_64-rhel-8.3-kselftests compiler: gcc-12 test machine: 36 threads 1 sockets Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz (Cascade Lake) with 32G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot oliver.sang@intel.com | Closes: https://lore.kernel.org/oe-lkp/202410251504.707d78fc-oliver.sang@intel.com
KERNEL SELFTESTS: linux_headers_dir is /usr/src/linux-headers-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487 '/usr/sbin/iptables' -> '/usr/sbin/iptables-nft' '/usr/sbin/iptables-restore' -> '/usr/sbin/iptables-nft-restore' '/usr/sbin/iptables-save' -> '/usr/sbin/iptables-nft-save' '/usr/sbin/ip6tables' -> '/usr/sbin/ip6tables-nft' '/usr/sbin/ip6tables-restore' -> '/usr/sbin/ip6tables-nft-restore' '/usr/sbin/ip6tables-save' -> '/usr/sbin/ip6tables-nft-save' 2024-10-23 12:53:55 sed -i s/default_timeout=45/default_timeout=300/ kselftest/runner.sh 2024-10-23 12:53:55 make -j36 -C cgroup make: Entering directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup' CC test_core CC test_cpu CC test_cpuset CC test_freezer CC test_hugetlb_memcg CC test_kill CC test_kmem CC test_memcontrol CC test_pids CC test_zswap CC wait_inotify In file included from /usr/x86_64-linux-gnu/include/asm/fcntl.h:1, from /usr/x86_64-linux-gnu/include/linux/fcntl.h:5, from /usr/x86_64-linux-gnu/include/linux/pidfd.h:7, from ../pidfd/pidfd.h:19, from test_kill.c:13: /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:156:8: error: redefinition of ‘struct f_owner_ex’ 156 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/x86_64-linux-gnu/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from ../pidfd/pidfd.h:8: /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:196:8: error: redefinition of ‘struct flock’ 196 | struct flock { | ^~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:35:8: note: originally defined here 35 | struct flock | ^~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:210:8: error: redefinition of ‘struct flock64’ 210 | struct flock64 { | ^~~~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:50:8: note: originally defined here 50 | struct flock64 | ^~~~~~~ make: *** [../lib.mk:221: /usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup/test_kill] Error 1 make: *** Waiting for unfinished jobs.... make: Leaving directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup' 2024-10-23 12:53:56 make quicktest=1 run_tests -C cgroup make: Entering directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup' CC test_kill In file included from /usr/x86_64-linux-gnu/include/asm/fcntl.h:1, from /usr/x86_64-linux-gnu/include/linux/fcntl.h:5, from /usr/x86_64-linux-gnu/include/linux/pidfd.h:7, from ../pidfd/pidfd.h:19, from test_kill.c:13: /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:156:8: error: redefinition of ‘struct f_owner_ex’ 156 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/x86_64-linux-gnu/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from ../pidfd/pidfd.h:8: /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:196:8: error: redefinition of ‘struct flock’ 196 | struct flock { | ^~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:35:8: note: originally defined here 35 | struct flock | ^~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:210:8: error: redefinition of ‘struct flock64’ 210 | struct flock64 { | ^~~~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:50:8: note: originally defined here 50 | struct flock64 | ^~~~~~~ make: *** [../lib.mk:222: /usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup/test_kill] Error 1 make: Leaving directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup'
The kernel config and materials to reproduce are available at: https://download.01.org/0day-ci/archive/20241025/202410251504.707d78fc-olive...
On Fri, Oct 25, 2024 at 04:10:37PM +0800, kernel test robot wrote:
Hello,
kernel test robot noticed "kernel-selftests.cgroup.make.fail" on:
commit: 930cb1423ee2522760ffde43455b14df5c0d5487 ("[PATCH v4 4/4] selftests: pidfd: add tests for PIDFD_SELF_*") url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-...
Thanks.
This issue is because, incredibly, the cgroup tests import ../pidfd/pidfd.h solely to use a helper, but in doing so inadvertently pull in linux/pidfd.h _without_ the tools wrapper.
Adding a tools wrapper to cgroup fails too because of some other dependency.
I will separate out a header with this helper in it to work around this and respin.
A gentle point on this - in my view, adding/updating tests shouldn't hold up a series, rather we should do everything we can to encourage kernel developers to add/expand tests.
So I'd say, in future, it might be best - if the tests already do something considered 'bad' - to defer fixing that badness to a dedicated series, rather than forcing an unrelated one to have to include commits to fixup pre-existing problems like this.
I don't think anyone is really going to understand why a PIDFD_SELF series is touching a cgroup test at this point.
Thanks!
base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next patch link: https://lore.kernel.org/all/b9851fa9f87d22f352f960b847d99459ef7d74a1.1729198... patch subject: [PATCH v4 4/4] selftests: pidfd: add tests for PIDFD_SELF_*
in testcase: kernel-selftests version: kernel-selftests-x86_64-977d51cf-1_20240508 with following parameters:
group: cgroup
config: x86_64-rhel-8.3-kselftests compiler: gcc-12 test machine: 36 threads 1 sockets Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz (Cascade Lake) with 32G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot oliver.sang@intel.com | Closes: https://lore.kernel.org/oe-lkp/202410251504.707d78fc-oliver.sang@intel.com
KERNEL SELFTESTS: linux_headers_dir is /usr/src/linux-headers-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487 '/usr/sbin/iptables' -> '/usr/sbin/iptables-nft' '/usr/sbin/iptables-restore' -> '/usr/sbin/iptables-nft-restore' '/usr/sbin/iptables-save' -> '/usr/sbin/iptables-nft-save' '/usr/sbin/ip6tables' -> '/usr/sbin/ip6tables-nft' '/usr/sbin/ip6tables-restore' -> '/usr/sbin/ip6tables-nft-restore' '/usr/sbin/ip6tables-save' -> '/usr/sbin/ip6tables-nft-save' 2024-10-23 12:53:55 sed -i s/default_timeout=45/default_timeout=300/ kselftest/runner.sh 2024-10-23 12:53:55 make -j36 -C cgroup make: Entering directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup' CC test_core CC test_cpu CC test_cpuset CC test_freezer CC test_hugetlb_memcg CC test_kill CC test_kmem CC test_memcontrol CC test_pids CC test_zswap CC wait_inotify In file included from /usr/x86_64-linux-gnu/include/asm/fcntl.h:1, from /usr/x86_64-linux-gnu/include/linux/fcntl.h:5, from /usr/x86_64-linux-gnu/include/linux/pidfd.h:7, from ../pidfd/pidfd.h:19, from test_kill.c:13: /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:156:8: error: redefinition of ‘struct f_owner_ex’ 156 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/x86_64-linux-gnu/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from ../pidfd/pidfd.h:8: /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:196:8: error: redefinition of ‘struct flock’ 196 | struct flock { | ^~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:35:8: note: originally defined here 35 | struct flock | ^~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:210:8: error: redefinition of ‘struct flock64’ 210 | struct flock64 { | ^~~~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:50:8: note: originally defined here 50 | struct flock64 | ^~~~~~~ make: *** [../lib.mk:221: /usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup/test_kill] Error 1 make: *** Waiting for unfinished jobs.... make: Leaving directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup' 2024-10-23 12:53:56 make quicktest=1 run_tests -C cgroup make: Entering directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup' CC test_kill In file included from /usr/x86_64-linux-gnu/include/asm/fcntl.h:1, from /usr/x86_64-linux-gnu/include/linux/fcntl.h:5, from /usr/x86_64-linux-gnu/include/linux/pidfd.h:7, from ../pidfd/pidfd.h:19, from test_kill.c:13: /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:156:8: error: redefinition of ‘struct f_owner_ex’ 156 | struct f_owner_ex { | ^~~~~~~~~~ In file included from /usr/include/x86_64-linux-gnu/bits/fcntl.h:61, from /usr/include/fcntl.h:35, from ../pidfd/pidfd.h:8: /usr/include/x86_64-linux-gnu/bits/fcntl-linux.h:274:8: note: originally defined here 274 | struct f_owner_ex | ^~~~~~~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:196:8: error: redefinition of ‘struct flock’ 196 | struct flock { | ^~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:35:8: note: originally defined here 35 | struct flock | ^~~~~ /usr/x86_64-linux-gnu/include/asm-generic/fcntl.h:210:8: error: redefinition of ‘struct flock64’ 210 | struct flock64 { | ^~~~~~~ /usr/include/x86_64-linux-gnu/bits/fcntl.h:50:8: note: originally defined here 50 | struct flock64 | ^~~~~~~ make: *** [../lib.mk:222: /usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup/test_kill] Error 1 make: Leaving directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-930cb1423ee2522760ffde43455b14df5c0d5487/tools/testing/selftests/cgroup'
The kernel config and materials to reproduce are available at: https://download.01.org/0day-ci/archive/20241025/202410251504.707d78fc-olive...
-- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
linux-kselftest-mirror@lists.linaro.org
-
John Hubbard -
kernel test robot -
Lorenzo Stoakes -
Shakeel Butt -
Shuah Khan