lkml.org 
[lkml]   [2020]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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@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
\
 
 \ /
  Last update: 2020-05-29 03:46    [W:0.066 / U:2.920 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site