SCHED_FLAG_SUGOV is supposed to be a kernel-only flag that userspace
cannot interact with. However, sched_getattr() currently reports it
in sched_flags if called on a sugov worker even though it is not
actually defined in a UAPI header. To avoid this, make sure to
clean-up the sched_flags field in sched_getattr() before returning to
userspace.
Signed-off-by: Quentin Perret <[email protected]>
---
kernel/sched/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d9ff40f4661..d8f489dcc383 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7535,6 +7535,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
kattr.sched_priority = p->rt_priority;
else
kattr.sched_nice = task_nice(p);
+ kattr.sched_flags &= SCHED_FLAG_ALL;
#ifdef CONFIG_UCLAMP_TASK
/*
--
2.32.0.432.gabb21c7263-goog
Hi Quentin,
On 27/07/21 11:11, Quentin Perret wrote:
> SCHED_FLAG_SUGOV is supposed to be a kernel-only flag that userspace
> cannot interact with. However, sched_getattr() currently reports it
> in sched_flags if called on a sugov worker even though it is not
> actually defined in a UAPI header. To avoid this, make sure to
> clean-up the sched_flags field in sched_getattr() before returning to
> userspace.
>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> kernel/sched/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 2d9ff40f4661..d8f489dcc383 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7535,6 +7535,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
> kattr.sched_priority = p->rt_priority;
> else
> kattr.sched_nice = task_nice(p);
> + kattr.sched_flags &= SCHED_FLAG_ALL;
Maybe we can do this in the previous patch so that it's kept confined to
deadline bits?
Thanks,
Juri
On Wednesday 28 Jul 2021 at 11:12:03 (+0200), Juri Lelli wrote:
> Hi Quentin,
>
> On 27/07/21 11:11, Quentin Perret wrote:
> > SCHED_FLAG_SUGOV is supposed to be a kernel-only flag that userspace
> > cannot interact with. However, sched_getattr() currently reports it
> > in sched_flags if called on a sugov worker even though it is not
> > actually defined in a UAPI header. To avoid this, make sure to
> > clean-up the sched_flags field in sched_getattr() before returning to
> > userspace.
> >
> > Signed-off-by: Quentin Perret <[email protected]>
> > ---
> > kernel/sched/core.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 2d9ff40f4661..d8f489dcc383 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -7535,6 +7535,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
> > kattr.sched_priority = p->rt_priority;
> > else
> > kattr.sched_nice = task_nice(p);
> > + kattr.sched_flags &= SCHED_FLAG_ALL;
>
> Maybe we can do this in the previous patch so that it's kept confined to
> deadline bits?
That works too, it just felt like this could happen again if we start
using non-standard flags outside of deadline for any reason at some
point in the future. But no strong opinion really.
Cheers,
Quentin
On 28/07/21 10:39, Quentin Perret wrote:
> On Wednesday 28 Jul 2021 at 11:12:03 (+0200), Juri Lelli wrote:
> > Hi Quentin,
> >
> > On 27/07/21 11:11, Quentin Perret wrote:
> > > SCHED_FLAG_SUGOV is supposed to be a kernel-only flag that userspace
> > > cannot interact with. However, sched_getattr() currently reports it
> > > in sched_flags if called on a sugov worker even though it is not
> > > actually defined in a UAPI header. To avoid this, make sure to
> > > clean-up the sched_flags field in sched_getattr() before returning to
> > > userspace.
> > >
> > > Signed-off-by: Quentin Perret <[email protected]>
> > > ---
> > > kernel/sched/core.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 2d9ff40f4661..d8f489dcc383 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -7535,6 +7535,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
> > > kattr.sched_priority = p->rt_priority;
> > > else
> > > kattr.sched_nice = task_nice(p);
> > > + kattr.sched_flags &= SCHED_FLAG_ALL;
> >
> > Maybe we can do this in the previous patch so that it's kept confined to
> > deadline bits?
>
> That works too, it just felt like this could happen again if we start
> using non-standard flags outside of deadline for any reason at some
> point in the future. But no strong opinion really.
Yeah, I also see this point. :)
So no prob with me to keep it in core.c as you do here.
Best,
Juri
On 28/07/2021 14:36, Juri Lelli wrote:
> On 28/07/21 10:39, Quentin Perret wrote:
>> On Wednesday 28 Jul 2021 at 11:12:03 (+0200), Juri Lelli wrote:
[...]
>>> Maybe we can do this in the previous patch so that it's kept confined to
>>> deadline bits?
>>
>> That works too, it just felt like this could happen again if we start
>> using non-standard flags outside of deadline for any reason at some
>> point in the future. But no strong opinion really.
>
> Yeah, I also see this point. :)
>
> So no prob with me to keep it in core.c as you do here.
>
> Best,
> Juri
>
I would vote for not exporting SCHED_FLAG_SUGOV from __getparam_dl() in
patch 1/2 to underpin the idea that this flag is a hack.
@ -2759,7 +2759,7 @@ void __getparam_dl(struct task_struct *p, struct
sched_attr *attr)
attr->sched_deadline = dl_se->dl_deadline;
attr->sched_period = dl_se->dl_period;
attr->sched_flags &= ~SCHED_DL_FLAGS;
- attr->sched_flags |= dl_se->flags;
+ attr->sched_flags |= dl_se->flags & ~SCHED_FLAG_SUGOV;
On Thursday 29 Jul 2021 at 19:21:03 (+0200), Dietmar Eggemann wrote:
> On 28/07/2021 14:36, Juri Lelli wrote:
> > On 28/07/21 10:39, Quentin Perret wrote:
> >> On Wednesday 28 Jul 2021 at 11:12:03 (+0200), Juri Lelli wrote:
>
> [...]
>
> >>> Maybe we can do this in the previous patch so that it's kept confined to
> >>> deadline bits?
> >>
> >> That works too, it just felt like this could happen again if we start
> >> using non-standard flags outside of deadline for any reason at some
> >> point in the future. But no strong opinion really.
> >
> > Yeah, I also see this point. :)
> >
> > So no prob with me to keep it in core.c as you do here.
> >
> > Best,
> > Juri
> >
>
> I would vote for not exporting SCHED_FLAG_SUGOV from __getparam_dl() in
> patch 1/2 to underpin the idea that this flag is a hack.
>
> @ -2759,7 +2759,7 @@ void __getparam_dl(struct task_struct *p, struct
> sched_attr *attr)
> attr->sched_deadline = dl_se->dl_deadline;
> attr->sched_period = dl_se->dl_period;
> attr->sched_flags &= ~SCHED_DL_FLAGS;
> - attr->sched_flags |= dl_se->flags;
> + attr->sched_flags |= dl_se->flags & ~SCHED_FLAG_SUGOV;
Alright, that's 2 votes against 1, you win!
I'll post a v2 shortly.
Cheers,
Quentin
The following commit has been merged into the sched/core branch of tip:
Commit-ID: 7ad721bf10718a4e480a27ded8bb16b8f6feb2d1
Gitweb: https://git.kernel.org/tip/7ad721bf10718a4e480a27ded8bb16b8f6feb2d1
Author: Quentin Perret <[email protected]>
AuthorDate: Tue, 27 Jul 2021 11:11:02 +01:00
Committer: Peter Zijlstra <[email protected]>
CommitterDate: Wed, 04 Aug 2021 15:16:44 +02:00
sched: Don't report SCHED_FLAG_SUGOV in sched_getattr()
SCHED_FLAG_SUGOV is supposed to be a kernel-only flag that userspace
cannot interact with. However, sched_getattr() currently reports it
in sched_flags if called on a sugov worker even though it is not
actually defined in a UAPI header. To avoid this, make sure to
clean-up the sched_flags field in sched_getattr() before returning to
userspace.
Signed-off-by: Quentin Perret <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
kernel/sched/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 6c562ad..314f70d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7530,6 +7530,7 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
kattr.sched_priority = p->rt_priority;
else
kattr.sched_nice = task_nice(p);
+ kattr.sched_flags &= SCHED_FLAG_ALL;
#ifdef CONFIG_UCLAMP_TASK
/*