On Thu, May 28, 2020 at 04:23:26PM -0700, Linus Torvalds wrote:
On Thu, May 28, 2020 at 4:09 PM Joel Fernandes joel@joelfernandes.org wrote:
So no, this patch is fundamentally wrong. It negates the whole point of having a uapi header at all.
Sorry, I naively assumed that headers in 'include/uapi/' are safe to include from userspace. I feel terrible.
Well, they "kind of" are safe to include.
It's just that normally they are meant for system integrators to include as part of the system header files. So they are designed not to be safe for normal programs, but for library writers.
And to make things more confusing, sometimes people _do_ include kernel header files directly, just because they want to get features that either haven't been exposed by the system libraries, or because the system libraries copied the uapi header files for an older version of the kernel, and you want the shiny new feature, so...
And some of those header files are easier to do that with than others...
Got it. Thank you Linus for the detailed reply, I really appreciate that.
The problem is <sched.h> still does not get us 'struct sched_attr' even though the manpage of sched_setattr(2) says including <sched.h> is all that's needed.
Ouch. But clearly you get it from -somewhere- since it then complains about the double declaration.
Strange.
The reason is, since <sched.h> did not provide struct sched_attr as the manpage said, so I did the include of uapi's linux/sched/types.h myself:
#include <sched.h> // inclusion of this header to get struct sched_attr will break due to // another structure struct sched_param redefinition. #include <linux/sched/types.h>
glibc's <sched.h> already defines struct sched_param (which is a POSIX struct), so my inclusion of <linux/sched/types.h> above which is a UAPI header exported by the kernel, breaks because the following commit moved sched_param into the UAPI: e2d1e2aec572a ("sched/headers: Move various ABI definitions to <uapi/linux/sched/types.h>")
Simply reverting that part of the patch also fixes it, like below. Would that be an acceptable fix? Then I can go patch glibc to get struct sched_attr by including the UAPI's <linux/sched/types.h>. Otherwise, I suspect glibc will also break if it tried to include the UAPI header.
If this is the wrong fix still, I'm sorry and I'll continue to research the solution.
Also, even if glibc included 'include/uapi/linux/sched/types.h' to get struct sched_attr's definition, we would run into the same issue I reported right? The 'struct sched_param' is already defined by glibc, and this header redefines it.
That's kind of the point: glibc sets up whatever system headers it wants. The uapi ones are there to _help_ it, but it's not like glibc _has_ to use them.
In fact, traditionally we didn't have any uapi header files at all, and we just expected the system libraries to scrape them and make their own private copies.
The uapi headers are _meant_ to make that easier, and to allow system headers to then co-exists with the inevitable "I need to get newer headers because I'm using a bleeding edge feature that glibc isn't exposing" crowd.
Put another way: a very non-portable programs _could_ include the uapi headers directly, if the system library headers were set up that way.
This might be painful for struct sched_attr because new attributes keep getting added to it, so having the UAPI help glibc and other libcs in this regard might be attractive.
---8<-----------------------
From: "Joel Fernandes (Google)" joel@joelfernandes.org Subject: [PATCH] sched/headers: Fix sched_setattr userspace compilation
This is a partial revert of e2d1e2aec572a to fix the following.
On a modern Linux distro, compiling the following program fails: #include<stdlib.h> #include<stdint.h>
// pthread.h includes sched.h internally. #include<pthread.h>
// inclusion of this header to get struct sched_attr will break due to // struct sched_param redefinition. #include<linux/sched/types.h>
void main() { struct sched_attr sa;
return; }
Compiler errors are: /usr/include/linux/sched/types.h:8:8: \ error: redefinition of ‘struct sched_param’ 8 | struct sched_param { | ^~~~~~~~~~~ In file included from /usr/include/x86_64-linux-gnu/bits/sched.h:74, from /usr/include/sched.h:43, from /usr/include/pthread.h:23, from /tmp/s.c:4: /usr/include/x86_64-linux-gnu/bits/types/struct_sched_param.h:23:8: note: originally defined here 23 | struct sched_param | ^~~~~~~~~~~
This also causes a problem with using sched_attr in Chrome. The issue is sched_param is already provided by glibc.
Fixes: e2d1e2aec572a ("sched/headers: Move various ABI definitions to <uapi/linux/sched/types.h>" Signed-off-by: Joel Fernandes (Google) joel@joelfernandes.org --- include/linux/sched.h | 5 ++++- include/uapi/linux/sched/types.h | 4 ---- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h index fc2d2ede2d9ef..e6917b9d919fd 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -55,13 +55,16 @@ struct robust_list_head; struct root_domain; struct rq; struct sched_attr; -struct sched_param; struct seq_file; struct sighand_struct; struct signal_struct; struct task_delay_info; struct task_group;
+struct sched_param { + int sched_priority; +}; + /* * Task state bitmask. NOTE! These bits are also * encoded in fs/proc/array.c: get_task_state(). diff --git a/include/uapi/linux/sched/types.h b/include/uapi/linux/sched/types.h index c852153ddb0d3..f863e747ff5f8 100644 --- a/include/uapi/linux/sched/types.h +++ b/include/uapi/linux/sched/types.h @@ -4,10 +4,6 @@
#include <linux/types.h>
-struct sched_param { - int sched_priority; -}; - #define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */ #define SCHED_ATTR_SIZE_VER1 56 /* add: util_{min,max} */