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.
v5: * Fixup self test dependencies on pidfd/pidfd.h.
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. https://lore.kernel.org/linux-mm/cover.1729198898.git.lorenzo.stoakes@oracle...
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 (5): pidfd: extend pidfd_get_pid() and de-duplicate pid lookup pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process tools: testing: separate out wait_for_pid() into helper header 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/cgroup/test_kill.c | 2 +- .../pid_namespace/regression_enomem.c | 2 +- tools/testing/selftests/pidfd/Makefile | 3 +- tools/testing/selftests/pidfd/pidfd.h | 28 +--- .../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++ tools/testing/selftests/pidfd/pidfd_helpers.h | 39 +++++ .../selftests/pidfd/pidfd_setns_test.c | 11 ++ tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++-- 15 files changed, 375 insertions(+), 88 deletions(-) create mode 100644 tools/include/linux/pidfd.h create mode 100644 tools/testing/selftests/pidfd/pidfd_helpers.h
-- 2.47.0
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.
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev 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; }
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.
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev 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 Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com 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.
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev 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. */
This conflicts with AT_FDCWD, might be worth changing?
+#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */
We might want to pick some range outside of the negative errno space (-4096 IIRC), since we have plenty of values to pick from (2^31 at least).
+static inline int pidfd_is_self_sentinel(pid_t pid) +{
return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;+}
Do we want this in the uapi header? Even if this is useful, it might come with several drawbacks such as breaking scripts that parse kernel headers (and a quick git grep suggests we do have static inlines in headers, but in rather obscure ones) and breaking C89:
<source>:8:8: error: unknown type name 'inline' 8 | static inline int pidfd_is_self_sentinel(pid_t pid)
:)
#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);
It would be really really nice to avoid the get here, but I imagine it'll take some refactoring around put_pid's?
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 {
Skipping the else here might make the rest of the code more legible (since the sentinel branch returns anyway...).
On Fri, Oct 25, 2024 at 01:50:12PM +0100, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com 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.
Reviewed-by: Shakeel Butt shakeel.butt@linux.dev 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. */
This conflicts with AT_FDCWD, might be worth changing?
+#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */
We might want to pick some range outside of the negative errno space (-4096 IIRC), since we have plenty of values to pick from (2^31 at least).
This is entirely up to Christian, I used the values he suggested in review. But I agree we should probably find one that doesn't conflict and is outside that range.
+static inline int pidfd_is_self_sentinel(pid_t pid) +{
return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;+}
Do we want this in the uapi header? Even if this is useful, it might come with several drawbacks such as breaking scripts that parse kernel headers (and a quick git grep suggests we do have static inlines in headers, but in rather obscure ones) and breaking C89:
<source>:8:8: error: unknown type name 'inline' 8 | static inline int pidfd_is_self_sentinel(pid_t pid)
:)
It doesn't really make sense to put it anywhere else I don't think.
I'm not sure 'support compilers that don't know what inline is' is a requirement for UAPI. Nor do I suspect people using such strict ansi-c89 compilers will be importing linux/pidfd.h... :)
Also:
[~/kerndev/kernels/mm/include/uapi/linux]$ ag inline | wc -l 382
I mean yeah 'obscure' or not it seems this is an acceptable thing to do :)
#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);It would be really really nice to avoid the get here, but I imagine it'll take some refactoring around put_pid's?
I cover this in the commit message and have addressed it on review already, but to risk repeating myself :)
Yes it'd be nice, but then you would have to make sure you _always_ unpinned correctly _everywhere_ from here on in, and it makes the behaviour different for these self modes.
You'd need to change how everyone everywhere puts and... yeah. It's not a big deal to do a useless ref inc here I don't think, eliminates a class of bug, and importantly it keeps behaviour identical to if you do a self-pidfd in the 'manual' way.
I equally dislike this aspect, but doing it this way also enables us to implement this in this one place and get self pidfd support 'for free' everywhere.
So I think RoI-wise this is a better proposition than the alternative.
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 {Skipping the else here might make the rest of the code more legible (since the sentinel branch returns anyway...).
This is so we can declare types for the other branch without having to figure out how to assign the struct fd sensibly.
Normally I'm a big fan of the if (!...) { return ... } guard pattern, but it's because of the 'types first' requirement of kernel code that I do this here.
-- Pedro
On 10/25/24 5:50 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
...
+static inline int pidfd_is_self_sentinel(pid_t pid) +{
return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;+}
Do we want this in the uapi header? Even if this is useful, it might come with several drawbacks such as breaking scripts that parse kernel headers (and a quick git grep suggests we do have static inlines in headers, but in rather obscure ones) and breaking C89:
Let's please not say "C89" anymore, we've moved on! :)
The notes in [1], which is now nearly 2.5 years old, discuss the move to C11, and specifically how to handle the inline keyword.
I think it's quite clear at this point, that we should not hold up new work, based on concerns about handling the inline keyword, nor about C89.
[1] commit e8c07082a810 ("Kbuild: move to -std=gnu11")
thanks,
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
On 10/25/24 5:50 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
...
+static inline int pidfd_is_self_sentinel(pid_t pid) +{
return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;+}
Do we want this in the uapi header? Even if this is useful, it might come with several drawbacks such as breaking scripts that parse kernel headers (and a quick git grep suggests we do have static inlines in headers, but in rather obscure ones) and breaking C89:
Let's please not say "C89" anymore, we've moved on! :)
The notes in [1], which is now nearly 2.5 years old, discuss the move to C11, and specifically how to handle the inline keyword.
That seems to only apply to the kernel internally, uapi headers are included from userspace too (-std=c89 -pedantic doesn't know what a gnu extension is). And uapi headers _generally_ keep to defining constants and structs, nothing more. I don't know what the guidelines for uapi headers are nowadays, but we generally want to not break userspace.
I think it's quite clear at this point, that we should not hold up new work, based on concerns about handling the inline keyword, nor about C89.
Right, but the correct solution is probably to move pidfd_is_self_sentinel to some other place, since it's not even supposed to be used by userspace (it's semantically useless to userspace, and it's only two users are in the kernel, kernel/pid.c and exit.c).
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
On 10/25/24 5:50 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
...
+static inline int pidfd_is_self_sentinel(pid_t pid) +{
return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;+}
Do we want this in the uapi header? Even if this is useful, it might come with several drawbacks such as breaking scripts that parse kernel headers (and a quick git grep suggests we do have static inlines in headers, but in rather obscure ones) and breaking C89:
Let's please not say "C89" anymore, we've moved on! :)
The notes in [1], which is now nearly 2.5 years old, discuss the move to C11, and specifically how to handle the inline keyword.
That seems to only apply to the kernel internally, uapi headers are
Yes.
included from userspace too (-std=c89 -pedantic doesn't know what a gnu extension is). And uapi headers _generally_ keep to defining constants and structs, nothing more.
OK
I don't know what the guidelines for uapi headers are nowadays, but we generally want to not break userspace.
I think it's quite clear at this point, that we should not hold up new work, based on concerns about handling the inline keyword, nor about C89.
Right, but the correct solution is probably to move pidfd_is_self_sentinel to some other place, since it's not even supposed to be used by userspace (it's semantically useless to userspace, and it's only two users are in the kernel, kernel/pid.c and exit.c).
Yes, if userspace absolutely doesn't need nor want this, then putting it in a non-uapi header does sound like the right move.
thanks,
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
On 10/25/24 5:50 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 10:41 AM Lorenzo Stoakes lorenzo.stoakes@oracle.com wrote:
...
+static inline int pidfd_is_self_sentinel(pid_t pid) +{
return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;+}
Do we want this in the uapi header? Even if this is useful, it might come with several drawbacks such as breaking scripts that parse kernel headers (and a quick git grep suggests we do have static inlines in headers, but in rather obscure ones) and breaking C89:
Let's please not say "C89" anymore, we've moved on! :)
The notes in [1], which is now nearly 2.5 years old, discuss the move to C11, and specifically how to handle the inline keyword.
That seems to only apply to the kernel internally, uapi headers are
Yes.
included from userspace too (-std=c89 -pedantic doesn't know what a gnu extension is). And uapi headers _generally_ keep to defining constants and structs, nothing more.
OK
Because a lot of people using -ANSI- C89 are importing a very new linux feature header.
And let's ignore the hundreds of existing uses... OK.
The rules, unstated anywhere, are that we must support 1972-era C in an optional header for a feature available only in new kernels because somebody somewhere is using a VAX-11 and gosh darn it they can't change their toolchain!
And you had better make sure you don't wear out those tape drums...
I don't know what the guidelines for uapi headers are nowadays, but we generally want to not break userspace.
I think it's quite clear at this point, that we should not hold up new work, based on concerns about handling the inline keyword, nor about C89.
Right, but the correct solution is probably to move pidfd_is_self_sentinel to some other place, since it's not even supposed to be used by userspace (it's semantically useless to userspace, and it's only two users are in the kernel, kernel/pid.c and exit.c).
Yes, if userspace absolutely doesn't need nor want this, then putting it in a non-uapi header does sound like the right move.
The bike shed should be blue! Wait no no, it should be red... Hang on yellow yes! Yellow's great!
No wait - did we _test_ yellow in the way I wanted...
I mean for me this isn't a big deal - we declare the defines here, it makes sense to have a very very simple inline function.
It's not like userspace is overly hurt by this...
Also I did explain there's no obvious header to put this in in the kernel and I'm not introducing one sorry.
ANyway if you guys feel strong enough about this, I'll respin again and just open-code this trivial check where it's used.
thanks,
John Hubbard
On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
...
That seems to only apply to the kernel internally, uapi headers are
Yes.
included from userspace too (-std=c89 -pedantic doesn't know what a gnu extension is). And uapi headers _generally_ keep to defining constants and structs, nothing more.
OK
Because a lot of people using -ANSI- C89 are importing a very new linux feature header.
I'll admit to being easily cowed by "you're breaking userspace" arguments. Even when they start to get rather absurd. Because I can't easily tell where the line is.
Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it to be! :)
And let's ignore the hundreds of existing uses... OK.
The rules, unstated anywhere, are that we must support 1972-era C in an optional header for a feature available only in new kernels because somebody somewhere is using a VAX-11 and gosh darn it they can't change their toolchain!
And you had better make sure you don't wear out those tape drums...
I don't know what the guidelines for uapi headers are nowadays, but we generally want to not break userspace.
I think it's quite clear at this point, that we should not hold up new work, based on concerns about handling the inline keyword, nor about C89.
Right, but the correct solution is probably to move pidfd_is_self_sentinel to some other place, since it's not even supposed to be used by userspace (it's semantically useless to userspace, and it's only two users are in the kernel, kernel/pid.c and exit.c).
Yes, if userspace absolutely doesn't need nor want this, then putting it in a non-uapi header does sound like the right move.
The bike shed should be blue! Wait no no, it should be red... Hang on yellow yes! Yellow's great!
Putting a header in the right location, so as to avoid breakage here or there, is not bikeshedding. Sorry.
No wait - did we _test_ yellow in the way I wanted...
I mean for me this isn't a big deal - we declare the defines here, it makes sense to have a very very simple inline function.
It's not like userspace is overly hurt by this...
Also I did explain there's no obvious header to put this in in the kernel and I'm not introducing one sorry.
ANyway if you guys feel strong enough about this, I'll respin again and just open-code this trivial check where it's used.
No strong feelings, just hoping to help make a choice that gets you closer to getting your patches committed.
thanks,
On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
...
That seems to only apply to the kernel internally, uapi headers are
Yes.
included from userspace too (-std=c89 -pedantic doesn't know what a gnu extension is). And uapi headers _generally_ keep to defining constants and structs, nothing more.
OK
Because a lot of people using -ANSI- C89 are importing a very new linux feature header.
I'll admit to being easily cowed by "you're breaking userspace" arguments. Even when they start to get rather absurd. Because I can't easily tell where the line is.
Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it to be! :)
Well, apparently not...
And let's ignore the hundreds of existing uses... OK.
The rules, unstated anywhere, are that we must support 1972-era C in an optional header for a feature available only in new kernels because somebody somewhere is using a VAX-11 and gosh darn it they can't change their toolchain!
And you had better make sure you don't wear out those tape drums...
I don't know what the guidelines for uapi headers are nowadays, but we generally want to not break userspace.
I think it's quite clear at this point, that we should not hold up new work, based on concerns about handling the inline keyword, nor about C89.
Right, but the correct solution is probably to move pidfd_is_self_sentinel to some other place, since it's not even supposed to be used by userspace (it's semantically useless to userspace, and it's only two users are in the kernel, kernel/pid.c and exit.c).
Yes, if userspace absolutely doesn't need nor want this, then putting it in a non-uapi header does sound like the right move.
The bike shed should be blue! Wait no no, it should be red... Hang on yellow yes! Yellow's great!
Putting a header in the right location, so as to avoid breakage here or there, is not bikeshedding. Sorry.
There are 312 uses of "static inline" already in UAPI headers, not all quite as obscure as claimed.
Specifically requiring me and only me to support ansi C89 for a theorised scenario is in my opinion bikeshedding, but I don't want to get into an argument about something so petty :)
No wait - did we _test_ yellow in the way I wanted...
I mean for me this isn't a big deal - we declare the defines here, it makes sense to have a very very simple inline function.
It's not like userspace is overly hurt by this...
Also I did explain there's no obvious header to put this in in the kernel and I'm not introducing one sorry.
ANyway if you guys feel strong enough about this, I'll respin again and just open-code this trivial check where it's used.
No strong feelings, just hoping to help make a choice that gets you closer to getting your patches committed.
I mean, you are saying I am breaking things and implying the series is blocked on this, that sounds like a strong opinion, but again I'm not going to argue.
As with the requirement that I, only for my part of the change, must fix up test header import, while I disagree I should be doing the fix, I did it anyway as I am accommodating and reasonable.
So fine - I'll respin and just open-code this as it's trivial and there's no (other) sensible place to put it anyway.
A P.S. though - a very NOT theoretical issue with userspace is the import of linux/fcntl.h in pidfd.h which seems to me to have been imported solely for the kernel's sake.
A gentle suggestion (it seems I can't win - gentle suggestions are ignored, tongue-in-cheek parody is taken to be mean... but anyway) is to do something like:
#ifdef __KERNEL__ #include <linux/fcntl.h> #else #include <fcntl.h> #endif
At the top of the pidfd.h header. This must surely sting a _lot_ of people in userland otherwise.
But this is out of scope for this change.
On 10/25/24 2:09 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
... I'll admit to being easily cowed by "you're breaking userspace" arguments. Even when they start to get rather absurd. Because I can't easily tell where the line is.
Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it to be! :)
Well, apparently not...
Why not? Your arguments are clear and reasonable. Why shouldn't they prevail?
Please don't think that I have some sort of firm position here. I'm simply looking for the right answer. And if that's different than something I proposed earlier, no problem. The best answer should win.
...
The bike shed should be blue! Wait no no, it should be red... Hang on yellow yes! Yellow's great!
Putting a header in the right location, so as to avoid breakage here or there, is not bikeshedding. Sorry.
There are 312 uses of "static inline" already in UAPI headers, not all quite as obscure as claimed.
OK, good. Let's lead with that. It seems very clear, then, that a new one won't cause a problem.
Specifically requiring me and only me to support ansi C89 for a theorised scenario is in my opinion bikeshedding, but I don't want to get into an argument about something so petty :)
An argument about the definition of bikeshedding sounds delightfully recursive, but yes, let's not. :)
...
ANyway if you guys feel strong enough about this, I'll respin again and just open-code this trivial check where it's used.
No strong feelings, just hoping to help make a choice that gets you closer to getting your patches committed.
I mean, you are saying I am breaking things and implying the series is blocked on this, that sounds like a strong opinion, but again I'm not going to argue.
Actually, Pedro's request kicked this off, and I was hoping to dismiss it--again, in order to help move things along. My opinion is that we should shun ancient toolchains and ancient systems whenever possible.
Somehow that got turned into "I'm trying to block the patchset". Really, whatever works, follows The Rules (whatever we eventually understand them to be), and doesn't cause someone *else* to come out of the woodwork and claim a problem, is fine with me.
As with the requirement that I, only for my part of the change, must fix up test header import, while I disagree I should be doing the fix, I did it anyway as I am accommodating and reasonable.
I agree that pre-existing problems in selftests should not be your problem.
By the way, I'm occasionally involved in helping fix up various selftest-related problems, especially when they impact mm. Send me a note if you have anything in mind that ought to be fixed up, I might be able to help head off future grief in that area.
So fine - I'll respin and just open-code this as it's trivial and there's no (other) sensible place to put it anyway.
A P.S. though - a very NOT theoretical issue with userspace is the import of linux/fcntl.h in pidfd.h which seems to me to have been imported solely for the kernel's sake.
A gentle suggestion (it seems I can't win - gentle suggestions are ignored, tongue-in-cheek parody is taken to be mean... but anyway) is to do
Actually, these come across as sarcasm, especially in the context of these emails that show you are becoming quite distraught.
I've met you several times at the conferences. We get along well. And your work is top notch. So please consider that I'm very much supportive of you and your work here.
I'm still trying to understand why you are recently sending these very strong emails (Vlastimil also took some heat), but I see that you also mentioned some long hours.
If my feedback is making things worse here, I'll try to adjust. Selftests in general are a frustrating area.
thanks,
On Fri, Oct 25, 2024 at 02:51:29PM -0700, John Hubbard wrote:
On 10/25/24 2:09 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 01:31:49PM -0700, John Hubbard wrote:
On 10/25/24 12:49 PM, Lorenzo Stoakes wrote:
On Fri, Oct 25, 2024 at 11:44:34AM -0700, John Hubbard wrote:
On 10/25/24 11:38 AM, Pedro Falcato wrote:
On Fri, Oct 25, 2024 at 6:41 PM John Hubbard jhubbard@nvidia.com wrote:
... I'll admit to being easily cowed by "you're breaking userspace" arguments. Even when they start to get rather absurd. Because I can't easily tell where the line is.
Maybe "-std=c89 -pedantic" is on the other side of the line. I'd like it to be! :)
Well, apparently not...
Why not? Your arguments are clear and reasonable. Why shouldn't they prevail?
Please don't think that I have some sort of firm position here. I'm simply looking for the right answer. And if that's different than something I proposed earlier, no problem. The best answer should win.
...
The bike shed should be blue! Wait no no, it should be red... Hang on yellow yes! Yellow's great!
Putting a header in the right location, so as to avoid breakage here or there, is not bikeshedding. Sorry.
There are 312 uses of "static inline" already in UAPI headers, not all quite as obscure as claimed.
OK, good. Let's lead with that. It seems very clear, then, that a new one won't cause a problem.
Right, sorry I thought I had made this point earlier, perhaps in a sub-thread of a thread of thread. It felt as if you guys were acting as if that were immaterial, which is why I highlighted it again.
Specifically requiring me and only me to support ansi C89 for a theorised scenario is in my opinion bikeshedding, but I don't want to get into an argument about something so petty :)
An argument about the definition of bikeshedding sounds delightfully recursive, but yes, let's not. :)
:)
...
ANyway if you guys feel strong enough about this, I'll respin again and just open-code this trivial check where it's used.
No strong feelings, just hoping to help make a choice that gets you closer to getting your patches committed.
I mean, you are saying I am breaking things and implying the series is blocked on this, that sounds like a strong opinion, but again I'm not going to argue.
Actually, Pedro's request kicked this off, and I was hoping to dismiss it--again, in order to help move things along. My opinion is that we should shun ancient toolchains and ancient systems whenever possible.
Somehow that got turned into "I'm trying to block the patchset". Really, whatever works, follows The Rules (whatever we eventually understand them to be), and doesn't cause someone *else* to come out of the woodwork and claim a problem, is fine with me.
Text is a poor medium, sorry!
I don't mean to say you're doing that purposefully on any level, I mean to say that, by arguing here over something that feels kind of unimportant, we are inadvertently doing that.
As with the requirement that I, only for my part of the change, must fix up test header import, while I disagree I should be doing the fix, I did it anyway as I am accommodating and reasonable.
I agree that pre-existing problems in selftests should not be your problem.
By the way, I'm occasionally involved in helping fix up various selftest-related problems, especially when they impact mm. Send me a note if you have anything in mind that ought to be fixed up, I might be able to help head off future grief in that area.
Sure, and I'm passionate about tests (I've written _thousands_ of lines of tests recently!) I mention this as a related example of something that feels out of scope.
Equally as the pidfd.h test header already had other instances of exactly what I did and thus really should have been solved as a separate series (one that I'd have been happy to do myself), I feel this issue, if truly a problem should be considered separately.
So fine - I'll respin and just open-code this as it's trivial and there's no (other) sensible place to put it anyway.
A P.S. though - a very NOT theoretical issue with userspace is the import of linux/fcntl.h in pidfd.h which seems to me to have been imported solely for the kernel's sake.
A gentle suggestion (it seems I can't win - gentle suggestions are ignored, tongue-in-cheek parody is taken to be mean... but anyway) is to do
Actually, these come across as sarcasm, especially in the context of these emails that show you are becoming quite distraught.
Yes, I get that, rather a Brit element to this, to be clear - I am not distraught, merely mildly perturbed. Again text is a bloody awful medium!
This genuinely was meant to be tongue in cheek, BUT I realise it's a me issue on this kind of thing - obviously written down like that it comes off as possibly dripping with a kind of venom that was ABSOLUTELY not intended.
Whereas my intent was a sort of wry smile, 'come on guys this doesn't matter' thing.
But since this is the second time now that I've said something intended that way and having been received as quite something different - this is a me thing - and I will refrain from dalliances into the rhetorical like this in future!
Apologies if either you or Pedro took offence and I'm fine! Other than this damn cold that wont' go away...
I've met you several times at the conferences. We get along well. And your work is top notch. So please consider that I'm very much supportive of you and your work here.
And I consider you one of the loveliest people in the kernel and very very sharp, and have enjoyed meeting you and your erstwhile colleague Jason :)
To be clear - I also have high regard for Pedro who I consider very smart, and I don't say that lightly.
I mean this _whole series_ is his idea, for instance. I don't just write series based on an idea on review for just anyone ;)
So this is not at all intended to be a critique of either of you, as I have the utmost regard for you both...!
I'm still trying to understand why you are recently sending these very strong emails (Vlastimil also took some heat), but I see that you also mentioned some long hours.
Well some of higher 'strength' have more basis than others that may be my failing to communicate things quite as intended. We'd have to speak on a case-by-case basis :)
But in Vlastimil's case, that was absolutely not intended. Again text is a bad medium!
If my feedback is making things worse here, I'll try to adjust. Selftests in general are a frustrating area.
No it's fine, I think just a comms thing here.
Please do carry on reviewing it is all much appreciated... I promise!
thanks,
John Hubbard
something like:
#ifdef __KERNEL__ #include <linux/fcntl.h> #else #include <fcntl.h> #endif
At the top of the pidfd.h header. This must surely sting a _lot_ of people in userland otherwise.
But this is out of scope for this change.
Anyway on this issue, as I said, and meant - I will respin with this taken out to alleviate concerns.
The _far_ more pressing issue I think is the one Pedro raised about the actual PIDFD_SELF* values. I may simply choose some arbitrary ones in the range specified by Pedro on respin.
Thanks! And I guess I owe you both beers ;)
It seems tests other than the pidfd tests use the wait_for_pid() function declared in pidfd.h.
Since we will shortly be modifying pidfd.h in a way that might clash with other tests, separate this out and update tests accordingly.
Signed-off-by: Lorenzo Stoakes lorenzo.stoakes@oracle.com --- tools/testing/selftests/cgroup/test_kill.c | 2 +- .../pid_namespace/regression_enomem.c | 2 +- tools/testing/selftests/pidfd/pidfd.h | 26 +------------ tools/testing/selftests/pidfd/pidfd_helpers.h | 39 +++++++++++++++++++ 4 files changed, 42 insertions(+), 27 deletions(-) create mode 100644 tools/testing/selftests/pidfd/pidfd_helpers.h
diff --git a/tools/testing/selftests/cgroup/test_kill.c b/tools/testing/selftests/cgroup/test_kill.c index 0e5bb6c7307a..2367f645fe89 100644 --- a/tools/testing/selftests/cgroup/test_kill.c +++ b/tools/testing/selftests/cgroup/test_kill.c @@ -10,7 +10,7 @@ #include <unistd.h>
#include "../kselftest.h" -#include "../pidfd/pidfd.h" +#include "../pidfd/pidfd_helpers.h" #include "cgroup_util.h"
/* diff --git a/tools/testing/selftests/pid_namespace/regression_enomem.c b/tools/testing/selftests/pid_namespace/regression_enomem.c index 7d84097ad45c..f3e6989c8069 100644 --- a/tools/testing/selftests/pid_namespace/regression_enomem.c +++ b/tools/testing/selftests/pid_namespace/regression_enomem.c @@ -12,7 +12,7 @@ #include <sys/wait.h>
#include "../kselftest_harness.h" -#include "../pidfd/pidfd.h" +#include "../pidfd/pidfd_helpers.h"
/* * Regression test for: diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h index 88d6830ee004..0f3fc51cec73 100644 --- a/tools/testing/selftests/pidfd/pidfd.h +++ b/tools/testing/selftests/pidfd/pidfd.h @@ -17,6 +17,7 @@ #include <sys/wait.h>
#include "../kselftest.h" +#include "pidfd_helpers.h"
#ifndef P_PIDFD #define P_PIDFD 3 @@ -68,31 +69,6 @@ #define PIDFD_SKIP 3 #define PIDFD_XFAIL 4
-static inline int wait_for_pid(pid_t pid) -{ - int status, ret; - -again: - ret = waitpid(pid, &status, 0); - if (ret == -1) { - if (errno == EINTR) - goto again; - - ksft_print_msg("waitpid returned -1, errno=%d\n", errno); - return -1; - } - - if (!WIFEXITED(status)) { - ksft_print_msg( - "waitpid !WIFEXITED, WIFSIGNALED=%d, WTERMSIG=%d\n", - WIFSIGNALED(status), WTERMSIG(status)); - return -1; - } - - ret = WEXITSTATUS(status); - return ret; -} - static inline int sys_pidfd_open(pid_t pid, unsigned int flags) { return syscall(__NR_pidfd_open, pid, flags); diff --git a/tools/testing/selftests/pidfd/pidfd_helpers.h b/tools/testing/selftests/pidfd/pidfd_helpers.h new file mode 100644 index 000000000000..5637bfe888de --- /dev/null +++ b/tools/testing/selftests/pidfd/pidfd_helpers.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __PIDFD_HELPERS_H +#define __PIDFD_HELPERS_H + +#define _GNU_SOURCE +#include <errno.h> +#include <stdlib.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <unistd.h> +#include "../kselftest.h" + +static inline int wait_for_pid(pid_t pid) +{ + int status, ret; + +again: + ret = waitpid(pid, &status, 0); + if (ret == -1) { + if (errno == EINTR) + goto again; + + ksft_print_msg("waitpid returned -1, errno=%d\n", errno); + return -1; + } + + if (!WIFEXITED(status)) { + ksft_print_msg( + "waitpid !WIFEXITED, WIFSIGNALED=%d, WTERMSIG=%d\n", + WIFSIGNALED(status), WTERMSIG(status)); + return -1; + } + + ret = WEXITSTATUS(status); + return ret; +} + +#endif /* __PIDFD_HELPERS_H */
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 -
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 0f3fc51cec73..1dbe48c1cf46 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" #include "pidfd_helpers.h"
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; }
linux-kselftest-mirror@lists.linaro.org
-
John Hubbard -
Lorenzo Stoakes -
Pedro Falcato