Re: [PATCH] uapi: expose enum pid_type
flat view
From: Christian Brauner <hidden>
Date: 2021-08-09 08:04:42
Also in:
linux-doc, lkml
On Sat, Aug 07, 2021 at 03:01:23AM +0200, Eugene Syromiatnikov wrote:
Commit 7ac592aa35a684ff ("sched: prctl() core-scheduling interface")
made use of enum pid_type in prctl's arg4; this type and the associated(Hm, I thought we were trying to stop exposing the thread-group leader concept directly to userspace. But that ship probably has sailed now.)
quoted hunk ↗ jump to hunk
+ __PIDTYPE_PID, + __PIDTYPE_TGID, + __PIDTYPE_PGID,
What's rather odd is that the interface seems to not be using PIDTYPE_PID, PIDTYPE_TGID, or PIDTYPE_PGID to indicate the type of the pid but rather the scope of the operation which conflicts with the names in the enum. So I think exposing these values as PIDTYPE_* is misleading as it doesn't really apply to the passed in pid. For example, if I pass in a non-thread group leader and specify PIDTYPE_TGID then this interface goes and digs out the thread-group leader from signal->pids when arguably it should've failed because I passed in a non-thread-group leader pid. So I think we shouldn't expose the enum with __PIDTYPE_* prefix. I think there's two ways. First, the prctl() would need to change a bit and then we could use the P_PID, P_PGID definitions from wait.h that we already expose to userspace (adding a P_TGID). But that would mean a uapi change for the prctl() and I've never really liked those defines anyway as they are equally strangely named. So I would suggest we expose a custom enum or a set of defines in prctl.h. Like (sorry, probably terrible names): #define PR_SCHED_CORE_SCOPE_THREAD #define PR_SCHED_CORE_SCOPE_THREADGROUP #define PR_SCHED_CORE_SCOPE_PROCESSGROUP that have the same values as PIDTYPE_* but express clearly that this indicates the scope of the operation independent of whether or not the passed in pid is a thread-group leader, process-group leader or not. Thoughts? Christian
quoted hunk ↗ jump to hunk
enumeration definitions are not exposed to userspace. Try to fix that by providing enum _kernel_pid_type and tying in-kernel enum pid_type definitions to it. Note that enum pid_type cannot be exposed as is, since "enum pid_type" is already exists in various projects [1] (notably gcc and strace), and "enum __pid_type" is defined by glibc and uclibc for fcntl(F_SETOWN_EX) owner ID type. [1] https://codesearch.debian.net/search?q=enum+pid_type Complements: 7ac592aa35a684ff ("sched: prctl() core-scheduling interface") Signed-off-by: Eugene Syromiatnikov <redacted> --- .../admin-guide/hw-vuln/core-scheduling.rst | 7 ++++--- include/linux/pid.h | 12 +++++++----- include/uapi/linux/pid.h | 20 ++++++++++++++++++++ include/uapi/linux/prctl.h | 1 + 4 files changed, 32 insertions(+), 8 deletions(-) create mode 100644 include/uapi/linux/pid.hdiff --git a/Documentation/admin-guide/hw-vuln/core-scheduling.rst b/Documentation/admin-guide/hw-vuln/core-scheduling.rst index 7b410ae..3eb2b7c 100644 --- a/Documentation/admin-guide/hw-vuln/core-scheduling.rst +++ b/Documentation/admin-guide/hw-vuln/core-scheduling.rst@@ -61,9 +61,10 @@ arg3: ``pid`` of the task for which the operation applies. arg4: - ``pid_type`` for which the operation applies. It is of type ``enum pid_type``. - For example, if arg4 is ``PIDTYPE_TGID``, then the operation of this command - will be performed for all tasks in the task group of ``pid``. + ``pid_type`` for which the operation applies. It is of type + ``enum __kernel_pid_type``. For example, if arg4 is ``__PIDTYPE_TGID``, + then the operation of this command will be performed for all tasks + in the task group of ``pid``. arg5: userspace pointer to an unsigned long for storing the cookie returned bydiff --git a/include/linux/pid.h b/include/linux/pid.h index fa10acb..f8ca4c9 100644 --- a/include/linux/pid.h +++ b/include/linux/pid.h@@ -5,14 +5,16 @@ #include <linux/rculist.h> #include <linux/wait.h> #include <linux/refcount.h> +#include <uapi/linux/pid.h> enum pid_type { - PIDTYPE_PID, - PIDTYPE_TGID, - PIDTYPE_PGID, - PIDTYPE_SID, - PIDTYPE_MAX, + PIDTYPE_PID = __PIDTYPE_PID, + PIDTYPE_TGID = __PIDTYPE_TGID, + PIDTYPE_PGID = __PIDTYPE_PGID, + PIDTYPE_SID = __PIDTYPE_SID, + + PIDTYPE_MAX = __PIDTYPE_MAX }; /*diff --git a/include/uapi/linux/pid.h b/include/uapi/linux/pid.h new file mode 100644 index 0000000..91d08e4 --- /dev/null +++ b/include/uapi/linux/pid.h@@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI_LINUX_PID_H +#define _UAPI_LINUX_PID_H + +/* + * Type of process-related ID. So far, it is used only for prctl(PR_SCHED_CORE); + * not to be confused with type field of f_owner_ex structure argument + * of fcntl(F_SETOWN_EX). + */ +enum __kernel_pid_type +{ + __PIDTYPE_PID, + __PIDTYPE_TGID, + __PIDTYPE_PGID, + __PIDTYPE_SID, + + __PIDTYPE_MAX /* Non-UAPI */ +}; + +#endif /* _UAPI_LINUX_PID_H */diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index 967d9c5..4e794aa 100644 --- a/include/uapi/linux/prctl.h +++ b/include/uapi/linux/prctl.h@@ -3,6 +3,7 @@ #define _LINUX_PRCTL_H #include <linux/types.h> +#include <linux/pid.h> /* enum __kernel_pid_type */ /* Values to pass as first argument to prctl() */-- 2.1.4