Message-ID: <20200529014524.GA38759@google.com>
Date:   Thu, 28 May 2020 21:45:24 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matthew Blecker <matthewb@...gle.com>,
        Jesse Barnes <jsbarnes@...gle.com>,
        Mike Frysinger <vapier@...gle.com>,
        Christian Brauner <christian@...uner.io>,
        vpillai <vpillai@...italocean.com>, vineethrp@...il.com,
        Peter Zijlstra <peterz@...radead.org>,
        stable <stable@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        mingo@...nel.org
Subject: Re: [PATCH] sched/headers: Fix sched_setattr userspace compilation
 breakage

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

Powered by blists - more mailing lists