sched/headers: Fix sched_setattr userspace compilation breakage

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.

---
 include/uapi/linux/sched/types.h | 2 ++
 1 file changed, 2 insertions(+)

index c852153ddb0d3..1f10d935a63fe 100644
@@ -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

You shouldn't include kernel headers in user space - that's the job of glibc and friends.

Hi Linus,

Ah, my bad. Sorry I read the docs now and looks like I got it all backwards.

Sorry, I naively assumed that headers in 'include/uapi/' are safe to include from userspace. I feel terrible.

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.

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

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...

Ouch. But clearly you get it from -somewhere- since it then complains about the double declaration.

Strange.

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

Got it. Thank you Linus for the detailed reply, I really appreciate that.

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.

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)" <[email protected]>
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.

---
 include/linux/sched.h            | 5 ++++-
 include/uapi/linux/sched/types.h | 4 ----
 2 files changed, 4 insertions(+), 5 deletions(-)

index fc2d2ede2d9ef..e6917b9d919fd 100644
@@ -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().

index c852153ddb0d3..f863e747ff5f8 100644
@@ -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

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

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,

Got it, thanks.

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.

Got it, thank you Linus !!!

- Joel