[PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Joel Fernandes (Google)]

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} */
-- 
2.26.2.761.g0e0b3e54be-goog

Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Linus Torvalds]

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

Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Joel Fernandes]

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

Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Linus Torvalds]

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

Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Joel Fernandes]

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} */
 
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Linus Torvalds]

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

Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation breakage

[Joel Fernandes]

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