Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage
From: Joel Fernandes
Date: Thu May 28 2020 - 21:45:32 EST
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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx>
---
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} */
--
2.27.0.rc0.183.gde8f92d652-goog