[musl] [PATCH] sched/headers: move struct sched_param out of uapi

[musl] [PATCH] sched/headers: move struct sched_param out of uapi

[Kir Kolyshkin]

Both glibc and musl define struct sched_param in sched.h, while kernel
has it in uapi/linux/sched/types.h, making it cumbersome to use
sched_getattr(2) or sched_setattr(2) from userspace.

For example, something like this:

	#include <sched.h>
	#include <linux/sched/types.h>

	struct sched_attr sa;

will result in "error: redefinition of ‘struct sched_param’" (note the
code doesn't need sched_param at all -- it needs struct sched_attr
plus some stuff from sched.h).

The situation is, glibc is not going to provide a wrapper for
sched_{get,set}attr, thus the need to include linux/sched_types.h
directly, which leads to the above problem.

Thus, the userspace is left with a few sub-par choices when it wants to
use e.g. sched_setattr(2), such as maintaining a copy of struct
sched_attr definition, or using some other ugly tricks.

OTOH, struct sched_param is well known, defined in POSIX, and it won't
be ever changed (as that would break backward compatibility).

So, while struct sched_param is indeed part of the kernel uapi,
exposing it the way it's done now creates an issue, and hiding it
(like this patch does) fixes that issue, hopefully without creating
another one: common userspace software rely on libc headers, and as
for "special" software (like libc), it looks like glibc and musl
do not rely on kernel headers for struct sched_param definition
(but let's Cc their mailing lists in case it's otherwise).

The alternative to this patch would be to move struct sched_attr to,
say, linux/sched.h, or linux/sched/attr.h (the new file).

Oh, and here is the previous attempt to fix the issue:
https://lore.kernel.org/all/20200528135552.GA87103@google.com/
While I support Linus arguments, the issue is still here
and needs to be fixed.

Cc: libc-alpha@sourceware.org
Cc: musl@lists.openwall.com
Cc: linux-api@vger.kernel.org
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Christian Brauner <brauner@kernel.org>
Fixes: e2d1e2aec572 ("sched/headers: Move various ABI definitions to <uapi/linux/sched/types.h>")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 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 609bde814cb0..3167e97a6b04 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -63,7 +63,6 @@ struct robust_list_head;
 struct root_domain;
 struct rq;
 struct sched_attr;
-struct sched_param;
 struct seq_file;
 struct sighand_struct;
 struct signal_struct;
@@ -370,6 +369,10 @@ extern struct root_domain def_root_domain;
 extern struct mutex sched_domains_mutex;
 #endif
 
+struct sched_param {
+	int sched_priority;
+};
+
 struct sched_info {
 #ifdef CONFIG_SCHED_INFO
 	/* Cumulative counters: */
diff --git a/include/uapi/linux/sched/types.h b/include/uapi/linux/sched/types.h
index f2c4589d4dbf..90662385689b 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.41.0

[musl] Re: [PATCH] sched/headers: move struct sched_param out of uapi

[Ingo Molnar]

* Kir Kolyshkin <kolyshkin@gmail.com> wrote:

> Both glibc and musl define struct sched_param in sched.h, while kernel
> has it in uapi/linux/sched/types.h, making it cumbersome to use
> sched_getattr(2) or sched_setattr(2) from userspace.
> 
> For example, something like this:
> 
> 	#include <sched.h>
> 	#include <linux/sched/types.h>
> 
> 	struct sched_attr sa;
> 
> will result in "error: redefinition of ‘struct sched_param’" (note the
> code doesn't need sched_param at all -- it needs struct sched_attr
> plus some stuff from sched.h).
> 
> The situation is, glibc is not going to provide a wrapper for
> sched_{get,set}attr, thus the need to include linux/sched_types.h
> directly, which leads to the above problem.

Meh, I truly detest this patch, because 'struct sched_param' is very much
part of the syscall ABI signature:

  include/linux/syscalls.h:

  asmlinkage long sys_sched_setparam(pid_t pid,
                                        struct sched_param __user *param);
  asmlinkage long sys_sched_setscheduler(pid_t pid, int policy,
                                        struct sched_param __user *param);
  asmlinkage long sys_sched_getparam(pid_t pid,
                                        struct sched_param __user *param);

... but OTOH let's not pretend that UAPI headers have much justification
to exist unless they are useful & can be utilized in some of the most
common user-space library environments as-is. Namespace collisions happen.

So I'm inclined to apply this workaround, but changed the title and added
the caveat below to the changelog:

    sched/headers: Move 'struct sched_param' out of uapi, to work around glibc/musl breakage

    ...

    Oh, and here is the previous attempt to fix the issue:
    
      https://lore.kernel.org/all/20200528135552.GA87103@google.com/
    
    While I support Linus arguments, the issue is still here
    and needs to be fixed.

    [ mingo: Linus is right, this shouldn't be needed - but on the other
             hand I agree that this header is not really helpful to
             user-space as-is. So let's pretend that
             <uapi/linux/sched/types.h> is only about sched_attr, and
             call this commit a workaround for user-space breakage
             that it in reality is ... Also, remove the Fixes tag. ]
    

So unless Linus objects, I'll apply this to tip:sched/core for a v6.7 merge.

Thanks,

	Ingo

[musl] Re: [PATCH] sched/headers: move struct sched_param out of uapi

[enh]

On Mon, Aug 7, 2023 at 8:04 PM Kir Kolyshkin via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Both glibc and musl define struct sched_param in sched.h, while kernel
> has it in uapi/linux/sched/types.h, making it cumbersome to use
> sched_getattr(2) or sched_setattr(2) from userspace.
>
> For example, something like this:
>
>         #include <sched.h>
>         #include <linux/sched/types.h>
>
>         struct sched_attr sa;
>
> will result in "error: redefinition of ‘struct sched_param’" (note the
> code doesn't need sched_param at all -- it needs struct sched_attr
> plus some stuff from sched.h).

note that `struct sched_attr` is still pretty problematic for
userspace because it keeps changing. i (Android's bionic libc
maintainer) get pretty frequent complaints about the lack of a wrapper
for this in libc, but that doesn't seem plausible if it keeps
changing. worse than that, we do find projects copy & pasting `struct
sched_attr` (ltp, for example), which causes problems when bionic --
which uses the latest released uapi headers directly -- and those
projects' duplicates don't match.

it would be helpful (or at least "less problematic") if each new
variant was called sched_attr_v1, sched_attr_v2 etc.

ironically, the end result of the requests for `struct sched_attr` to
be in <sched.h> and to have wrappers for the syscalls is that i'm
seriously considering _removing_ `struct sched_attr` from our uapi
headers [because when i said we use them "directly", they do actually
go through a python script] on the assumption that "everyone just
carries around the specific version they want, and no-one has to worry
about ABI differences" is less problematic than the current situation.

(to be clear: personally i've only seen source incompatibility.
although one _could_ pass `struct sched_attr` across library
boundaries and have ABI incompatibility, i haven't yet seen that.
unless you count "that's the reason why there's no libc wrapper for
this syscall, despite it being by far my most-demanded syscall
wrapper".)

> The situation is, glibc is not going to provide a wrapper for
> sched_{get,set}attr, thus the need to include linux/sched_types.h
> directly, which leads to the above problem.
>
> Thus, the userspace is left with a few sub-par choices when it wants to
> use e.g. sched_setattr(2), such as maintaining a copy of struct
> sched_attr definition, or using some other ugly tricks.
>
> OTOH, struct sched_param is well known, defined in POSIX, and it won't
> be ever changed (as that would break backward compatibility).
>
> So, while struct sched_param is indeed part of the kernel uapi,
> exposing it the way it's done now creates an issue, and hiding it
> (like this patch does) fixes that issue, hopefully without creating
> another one: common userspace software rely on libc headers, and as
> for "special" software (like libc), it looks like glibc and musl
> do not rely on kernel headers for struct sched_param definition
> (but let's Cc their mailing lists in case it's otherwise).

getting back to your actual point about `struct sched_param`, yes,
this sgtm too. bionic has the exact same <sched.h> vs
<linux/sched/types.h> duplication.

> The alternative to this patch would be to move struct sched_attr to,
> say, linux/sched.h, or linux/sched/attr.h (the new file).

as long as everyone promises never to change `struct sched_param`,
that would be my personal preference --- my _ideal_ is that i never
define anything that's uapi and get it "direct" from the [very lightly
modified] upstream uapi headers instead.

> Oh, and here is the previous attempt to fix the issue:
> https://lore.kernel.org/all/20200528135552.GA87103@google.com/
> While I support Linus arguments, the issue is still here
> and needs to be fixed.
>
> Cc: libc-alpha@sourceware.org
> Cc: musl@lists.openwall.com
> Cc: linux-api@vger.kernel.org
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Cc: Christian Brauner <brauner@kernel.org>
> Fixes: e2d1e2aec572 ("sched/headers: Move various ABI definitions to <uapi/linux/sched/types.h>")
> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
> ---
>  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 609bde814cb0..3167e97a6b04 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -63,7 +63,6 @@ struct robust_list_head;
>  struct root_domain;
>  struct rq;
>  struct sched_attr;
> -struct sched_param;
>  struct seq_file;
>  struct sighand_struct;
>  struct signal_struct;
> @@ -370,6 +369,10 @@ extern struct root_domain def_root_domain;
>  extern struct mutex sched_domains_mutex;
>  #endif
>
> +struct sched_param {
> +       int sched_priority;
> +};
> +
>  struct sched_info {
>  #ifdef CONFIG_SCHED_INFO
>         /* Cumulative counters: */
> diff --git a/include/uapi/linux/sched/types.h b/include/uapi/linux/sched/types.h
> index f2c4589d4dbf..90662385689b 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.41.0
>