On a modern Linux distro, compiling the following program fails: #include<stdlib.h> #include<stdint.h> #include<pthread.h> #include<linux/sched/types.h>
void main() { struct sched_attr sa;
return; }
with: /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.
Guard the kernel's UAPI definition of sched_param with __KERNEL__ so that userspace can compile.
Fixes: e2d1e2aec572a ("sched/headers: Move various ABI definitions to <uapi/linux/sched/types.h>" Signed-off-by: Joel Fernandes (Google) joel@joelfernandes.org --- include/uapi/linux/sched/types.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/uapi/linux/sched/types.h b/include/uapi/linux/sched/types.h index c852153ddb0d3..1f10d935a63fe 100644 --- a/include/uapi/linux/sched/types.h +++ b/include/uapi/linux/sched/types.h @@ -4,9 +4,11 @@
#include <linux/types.h>
+#if defined(__KERNEL__) struct sched_param { int sched_priority; }; +#endif
#define SCHED_ATTR_SIZE_VER0 48 /* sizeof first published struct */ #define SCHED_ATTR_SIZE_VER1 56 /* add: util_{min,max} */
On Thu, May 28, 2020 at 6:55 AM Joel Fernandes (Google) joel@joelfernandes.org wrote:
On a modern Linux distro, compiling the following program fails: #include<stdlib.h> #include<stdint.h> #include<pthread.h> #include<linux/sched/types.h>
You shouldn't include kernel headers in user space - that's the job of glibc and friends.
--- a/include/uapi/linux/sched/types.h +++ b/include/uapi/linux/sched/types.h @@ -4,9 +4,11 @@
#include <linux/types.h>
+#if defined(__KERNEL__) struct sched_param { int sched_priority; }; +#endif
This makes no sense.
The point of a 'uapi' header is to export things to user space. Yes, they sometimes mix kernel-internal thngs in there (because of how they were created by just moving kernel headers to the uapi directory), but that ' struct sched_param' is very much part of the very interface definition that that file is all about exporting.
So no, this patch is fundamentally wrong. It negates the whole point of having a uapi header at all.
The glibc-provided "<sched.h>" should have been where you got all these declarations and #defines from, and the point of the uapi file was always to help glibc (and other library implementations) get them from the kernel.
So why are you including kernel header files and mixing them with system-provided stuff?
Linus
Hi Linus,
On Thu, May 28, 2020 at 03:21:56PM -0700, Linus Torvalds wrote:
On Thu, May 28, 2020 at 6:55 AM Joel Fernandes (Google) joel@joelfernandes.org wrote:
On a modern Linux distro, compiling the following program fails: #include<stdlib.h> #include<stdint.h> #include<pthread.h> #include<linux/sched/types.h>
You shouldn't include kernel headers in user space - that's the job of glibc and friends.
Ah, my bad. Sorry I read the docs now and looks like I got it all backwards.
--- a/include/uapi/linux/sched/types.h +++ b/include/uapi/linux/sched/types.h @@ -4,9 +4,11 @@
#include <linux/types.h>
+#if defined(__KERNEL__) struct sched_param { int sched_priority; }; +#endif
This makes no sense.
The point of a 'uapi' header is to export things to user space. Yes, they sometimes mix kernel-internal thngs in there (because of how they were created by just moving kernel headers to the uapi directory), but that ' struct sched_param' is very much part of the very interface definition that that file is all about exporting.
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.
The glibc-provided "<sched.h>" should have been where you got all these declarations and #defines from, and the point of the uapi file was always to help glibc (and other library implementations) get them from the kernel.
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.
So why are you including kernel header files and mixing them with system-provided stuff?
The include of <sched.h> does not result in availability of the sched_attr header.
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.
Sorry that this patch is wrong, I'll try to fix it the right way. Thanks for your help.
thanks,
- Joel
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...
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.
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.
More commonly, what bleeding edge people do when the system header files don't play nice, is to just build their very own libraries and copy the bleeding-edge features directly from the kernel. So you'll find things like
#ifndef __NR_clone3 #define __NR_clone3 435 #endif
syscall(__NR_clone3, ...);
in user space that wants to use the clone3 system call, but knows that it hasn't made it into the glibc headers yet, so the program just forced the local knowledge of it.
That's obviously generally easier to do with macro defines like the above, exactly because you can trivially _test_ whether they've been exposed or not.
So yes, this is a horrid mess.
But the *intent* is that the uapi header files should be things that system library implementations can just use directly, and then the "special" users could mix-and-match if everything is wonderful.
Which it seldom is.
But that's why putting those interface declarations inside a #ifdef __KERNEL__ would completely destroy the whole point.
Linus
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} */
On Thu, May 28, 2020 at 6:45 PM Joel Fernandes joel@joelfernandes.org wrote:
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.
Hmm.
Reverting that commit makes some sense as a "it broke things", and yes, if this was some recent change that caused problems with user headers, that would be what we should do (at least to then think about it a bit more).
But that commit was done three years ago and you're the first person to report breakage.
So for all I know, modern glibc source bases have already fixed themselves up, and take advantage of the new UAPI location. Or they just did that kernel header sync many years ago, and will fix it up the next time they do a header sync.
So then reverting things (or adding the __KERNEL__ guard) would only break _those_ cases instead and make for only more problems.
Basically, I think you should treat this as a glibc header bug, not a kernel header bug.
And when you say
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:
instead of starting to include the kernel uapi header files - that interact at a deep level with those system header files - you should just treat it as a glibc bug.
And then you can either work around it locally, or make a glibc bug-report and hope it gets fixed that way.
The "work around it locally" might be something like a "glibc-sched-h-fixup.h" header file that does
#ifndef SCHED_FIXUP_H #define SCHED_FIXUP_H #include <sched.h>
/* This is documented to come from <sched.h>, but doesn't */ struct sched_attr { __u32 size;
__u32 sched_policy; __u64 sched_flags;
/* SCHED_NORMAL, SCHED_BATCH */ __s32 sched_nice;
/* SCHED_FIFO, SCHED_RR */ __u32 sched_priority;
/* SCHED_DEADLINE */ __u64 sched_runtime; __u64 sched_deadline; __u64 sched_period;
/* Utilization hints */ __u32 sched_util_min; __u32 sched_util_max;
}; #end /* SCHED_FIXUP_H */
in your build environment (possibly with configure magic etc to find the need for this fixup, depending on how fancy you want to be).
Because when we have a change that is three+ years old, we can't reasonably change the kernel back again without then likely just breaking some other case that depends on that uapi file that has been there for the last few years.
glibc and the kernel aren't developed in sync, so glibc generally takes a snapshot of the kernel headers and then works with that. That allows glibc developers to work around any issues they have with our uapi headers (we've had lots of namespace issues, for example), but it also means that the system headers aren't using some "generic kernel UAPI headers". They are using a very _particular_ set of kernel uapi headers from (likely) several years ago, and quite possibly then further edited too.
Which is why you can't then mix glibc system headers that are years old with kernel headers that are modern (or vice versa).
Well, with extreme luck and/or care you can. But not in general.
Linus
Hi Linus,
On Thu, May 28, 2020 at 07:17:38PM -0700, Linus Torvalds wrote:
On Thu, May 28, 2020 at 6:45 PM Joel Fernandes joel@joelfernandes.org wrote:
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.
Hmm.
Reverting that commit makes some sense as a "it broke things", and yes, if this was some recent change that caused problems with user headers, that would be what we should do (at least to then think about it a bit more).
But that commit was done three years ago and you're the first person to report breakage.
So for all I know, modern glibc source bases have already fixed themselves up, and take advantage of the new UAPI location. Or they just did that kernel header sync many years ago, and will fix it up the next time they do a header sync.
So then reverting things (or adding the __KERNEL__ guard) would only break _those_ cases instead and make for only more problems.
Basically, I think you should treat this as a glibc header bug, not a kernel header bug.
Got it, thanks.
And when you say
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:
instead of starting to include the kernel uapi header files - that interact at a deep level with those system header files - you should just treat it as a glibc bug.
And then you can either work around it locally, or make a glibc bug-report and hope it gets fixed that way.
The "work around it locally" might be something like a "glibc-sched-h-fixup.h" header file that does
#ifndef SCHED_FIXUP_H #define SCHED_FIXUP_H #include <sched.h>
/* This is documented to come from <sched.h>, but doesn't */ struct sched_attr { __u32 size;
__u32 sched_policy; __u64 sched_flags; /* SCHED_NORMAL, SCHED_BATCH */ __s32 sched_nice; /* SCHED_FIFO, SCHED_RR */ __u32 sched_priority; /* SCHED_DEADLINE */ __u64 sched_runtime; __u64 sched_deadline; __u64 sched_period; /* Utilization hints */ __u32 sched_util_min; __u32 sched_util_max;}; #end /* SCHED_FIXUP_H */
in your build environment (possibly with configure magic etc to find the need for this fixup, depending on how fancy you want to be).
Got it, I will look into these options. Thanks.
Turns out I hit the same/similar issue in 2018 but for a different reason. At the time I was working on Android and needed this struct. The bionic C library folks refused to add it because no other libc exposed it either (that was their reason to not have it, anyway). I suspect everyone was just doing their own fixups to use it and that was what I was asked to do.
I think it would be better to just do the fixup you suggested above for now.
Because when we have a change that is three+ years old, we can't reasonably change the kernel back again without then likely just breaking some other case that depends on that uapi file that has been there for the last few years.
glibc and the kernel aren't developed in sync, so glibc generally takes a snapshot of the kernel headers and then works with that. That allows glibc developers to work around any issues they have with our uapi headers (we've had lots of namespace issues, for example), but it also means that the system headers aren't using some "generic kernel UAPI headers". They are using a very _particular_ set of kernel uapi headers from (likely) several years ago, and quite possibly then further edited too.
Which is why you can't then mix glibc system headers that are years old with kernel headers that are modern (or vice versa).
Well, with extreme luck and/or care you can. But not in general.
Got it, thank you Linus !!!
- Joel
linux-stable-mirror@lists.linaro.org
-
Joel Fernandes -
Joel Fernandes (Google)
-
Linus Torvalds