* [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us
@ 2023-09-01 15:43 Cyril Hrubis
2023-09-01 17:45 ` Petr Vorel
0 siblings, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2023-09-01 15:43 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
Cc: ltp
The validation of the value written to sched_rt_period_us was broken
because:
- the sysclt_sched_rt_period is declared as unsigned int
- parsed by proc_do_intvec()
- the range is asserted after the value parsed by proc_do_intvec()
Because of this negative values written to the file were written into a
unsigned integer that were later on interpreted as large positive
integers which did passed the check:
if (sysclt_sched_rt_period <= 0)
return EINVAL;
This commit fixes the parsing by setting explicit range for both
perid_us and runtime_us into the sched_rt_sysctls table and processes
the values with proc_dointvec_minmax() instead.
Alternatively if we wanted to use full range of unsigned int for the
period value we would have to split the proc_handler and use
proc_douintvec() for it however even the
Documentation/scheduller/sched-rt-group.rst describes the range as 1 to
INT_MAX.
As far as I can tell the only problem this causes is that the sysctl
file allows writing negative values which when read back may confuse
userspace.
There is also a LTP test being submitted for these sysctl files at:
http://patchwork.ozlabs.org/project/ltp/patch/20230901144433.2526-1-chrubis@suse.cz/
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
kernel/sched/rt.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 0597ba0f85ff..aed3d55de2dd 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -37,6 +37,8 @@ static struct ctl_table sched_rt_sysctls[] = {
.maxlen = sizeof(unsigned int),
.mode = 0644,
.proc_handler = sched_rt_handler,
+ .extra1 = SYSCTL_ONE,
+ .extra2 = SYSCTL_INT_MAX,
},
{
.procname = "sched_rt_runtime_us",
@@ -44,6 +46,8 @@ static struct ctl_table sched_rt_sysctls[] = {
.maxlen = sizeof(int),
.mode = 0644,
.proc_handler = sched_rt_handler,
+ .extra1 = SYSCTL_NEG_ONE,
+ .extra2 = SYSCTL_INT_MAX,
},
{
.procname = "sched_rr_timeslice_ms",
@@ -2985,9 +2989,6 @@ static int sched_rt_global_constraints(void)
#ifdef CONFIG_SYSCTL
static int sched_rt_global_validate(void)
{
- if (sysctl_sched_rt_period <= 0)
- return -EINVAL;
-
if ((sysctl_sched_rt_runtime != RUNTIME_INF) &&
((sysctl_sched_rt_runtime > sysctl_sched_rt_period) ||
((u64)sysctl_sched_rt_runtime *
@@ -3018,7 +3019,7 @@ static int sched_rt_handler(struct ctl_table *table, int write, void *buffer,
old_period = sysctl_sched_rt_period;
old_runtime = sysctl_sched_rt_runtime;
- ret = proc_dointvec(table, write, buffer, lenp, ppos);
+ ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
if (!ret && write) {
ret = sched_rt_global_validate();
--
2.41.0
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us
2023-09-01 15:43 [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us Cyril Hrubis
@ 2023-09-01 17:45 ` Petr Vorel
2023-09-01 18:17 ` Cyril Hrubis
0 siblings, 1 reply; 6+ messages in thread
From: Petr Vorel @ 2023-09-01 17:45 UTC (permalink / raw)
To: Cyril Hrubis
Cc: Juri Lelli, Valentin Schneider, Vincent Guittot, Peter Zijlstra,
linux-kernel, Steven Rostedt, Ben Segall, Ingo Molnar, Mel Gorman,
Daniel Bristot de Oliveira, Dietmar Eggemann, ltp
> The validation of the value written to sched_rt_period_us was broken
> because:
> - the sysclt_sched_rt_period is declared as unsigned int
> - parsed by proc_do_intvec()
> - the range is asserted after the value parsed by proc_do_intvec()
> Because of this negative values written to the file were written into a
> unsigned integer that were later on interpreted as large positive
> integers which did passed the check:
> if (sysclt_sched_rt_period <= 0)
> return EINVAL;
> This commit fixes the parsing by setting explicit range for both
> perid_us and runtime_us into the sched_rt_sysctls table and processes
> the values with proc_dointvec_minmax() instead.
> Alternatively if we wanted to use full range of unsigned int for the
> period value we would have to split the proc_handler and use
> proc_douintvec() for it however even the
> Documentation/scheduller/sched-rt-group.rst describes the range as 1 to
> INT_MAX.
> As far as I can tell the only problem this causes is that the sysctl
> file allows writing negative values which when read back may confuse
> userspace.
> There is also a LTP test being submitted for these sysctl files at:
> http://patchwork.ozlabs.org/project/ltp/patch/20230901144433.2526-1-chrubis@suse.cz/
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
> kernel/sched/rt.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 0597ba0f85ff..aed3d55de2dd 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -37,6 +37,8 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(unsigned int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> + .extra1 = SYSCTL_ONE,
> + .extra2 = SYSCTL_INT_MAX,
> },
> {
> .procname = "sched_rt_runtime_us",
> @@ -44,6 +46,8 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> + .extra1 = SYSCTL_NEG_ONE,
> + .extra2 = SYSCTL_INT_MAX,
Documentation/scheduller/sched-rt-group.rst [1] specifies this as values from -1 to
(INT_MAX - 1), I guess due int range. Looking into proc_dointvec_minmax() [2]
even INT_MAX would pass the check. I suppose we can do nothing about that,
because there is no value in sysctl_vals[] which would represent INT_MAX - 1.
And you specify in LTP test range: from -1 to INT_MAX.
But even much shorter value than INT_MAX fails:
$ echo 1234567 > /proc/sys/kernel/sched_rt_runtime_us
sh: echo: write error: Invalid argument
Ranges in /proc/sys/kernel/sched_rt_period_us works as expected.
[1] https://www.kernel.org/doc/html/latest/scheduler/sched-rt-group.html#system-wide-settings
[2] https://elixir.bootlin.com/linux/latest/source/kernel/sysctl.c#L843
> },
> {
> .procname = "sched_rr_timeslice_ms",
> @@ -2985,9 +2989,6 @@ static int sched_rt_global_constraints(void)
> #ifdef CONFIG_SYSCTL
> static int sched_rt_global_validate(void)
> {
> - if (sysctl_sched_rt_period <= 0)
> - return -EINVAL;
> -
> if ((sysctl_sched_rt_runtime != RUNTIME_INF) &&
> ((sysctl_sched_rt_runtime > sysctl_sched_rt_period) ||
> ((u64)sysctl_sched_rt_runtime *
> @@ -3018,7 +3019,7 @@ static int sched_rt_handler(struct ctl_table *table, int write, void *buffer,
> old_period = sysctl_sched_rt_period;
> old_runtime = sysctl_sched_rt_runtime;
> - ret = proc_dointvec(table, write, buffer, lenp, ppos);
> + ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> if (!ret && write) {
> ret = sched_rt_global_validate();
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us
2023-09-01 17:45 ` Petr Vorel
@ 2023-09-01 18:17 ` Cyril Hrubis
2023-09-02 20:22 ` Petr Vorel
2023-09-22 9:46 ` Ingo Molnar
0 siblings, 2 replies; 6+ messages in thread
From: Cyril Hrubis @ 2023-09-01 18:17 UTC (permalink / raw)
To: Petr Vorel
Cc: Juri Lelli, Valentin Schneider, Vincent Guittot, Peter Zijlstra,
linux-kernel, Steven Rostedt, Ben Segall, Ingo Molnar, Mel Gorman,
Daniel Bristot de Oliveira, Dietmar Eggemann, ltp
Hi!
> Documentation/scheduller/sched-rt-group.rst [1] specifies this as values from -1 to
> (INT_MAX - 1), I guess due int range. Looking into proc_dointvec_minmax() [2]
> even INT_MAX would pass the check. I suppose we can do nothing about that,
> because there is no value in sysctl_vals[] which would represent INT_MAX - 1.
>
> And you specify in LTP test range: from -1 to INT_MAX.
>
> But even much shorter value than INT_MAX fails:
>
> $ echo 1234567 > /proc/sys/kernel/sched_rt_runtime_us
> sh: echo: write error: Invalid argument
That is because runtime_us must be < period_us by definition, since
runtime_us defines how much time is allocated from the period_us. I
guess that this is not described good enough in the kernel docs.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us
2023-09-01 18:17 ` Cyril Hrubis
@ 2023-09-02 20:22 ` Petr Vorel
2023-09-22 9:46 ` Ingo Molnar
1 sibling, 0 replies; 6+ messages in thread
From: Petr Vorel @ 2023-09-02 20:22 UTC (permalink / raw)
To: Cyril Hrubis
Cc: Juri Lelli, Valentin Schneider, Vincent Guittot, Peter Zijlstra,
linux-kernel, Steven Rostedt, Ben Segall, Ingo Molnar, Mel Gorman,
Daniel Bristot de Oliveira, Dietmar Eggemann, ltp
> Hi!
> > Documentation/scheduller/sched-rt-group.rst [1] specifies this as values from -1 to
> > (INT_MAX - 1), I guess due int range. Looking into proc_dointvec_minmax() [2]
> > even INT_MAX would pass the check. I suppose we can do nothing about that,
> > because there is no value in sysctl_vals[] which would represent INT_MAX - 1.
> > And you specify in LTP test range: from -1 to INT_MAX.
> > But even much shorter value than INT_MAX fails:
> > $ echo 1234567 > /proc/sys/kernel/sched_rt_runtime_us
> > sh: echo: write error: Invalid argument
> That is because runtime_us must be < period_us by definition, since
> runtime_us defines how much time is allocated from the period_us. I
> guess that this is not described good enough in the kernel docs.
Thanks for clarification. +1 for docs update.
Kind regards,
Petr
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us
2023-09-01 18:17 ` Cyril Hrubis
2023-09-02 20:22 ` Petr Vorel
@ 2023-09-22 9:46 ` Ingo Molnar
2023-09-22 11:43 ` Cyril Hrubis
1 sibling, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2023-09-22 9:46 UTC (permalink / raw)
To: Cyril Hrubis
Cc: Juri Lelli, Valentin Schneider, Vincent Guittot, Peter Zijlstra,
linux-kernel, Steven Rostedt, Ben Segall, Ingo Molnar, Mel Gorman,
Daniel Bristot de Oliveira, Dietmar Eggemann, ltp
* Cyril Hrubis <chrubis@suse.cz> wrote:
> Hi!
> > Documentation/scheduller/sched-rt-group.rst [1] specifies this as values from -1 to
> > (INT_MAX - 1), I guess due int range. Looking into proc_dointvec_minmax() [2]
> > even INT_MAX would pass the check. I suppose we can do nothing about that,
> > because there is no value in sysctl_vals[] which would represent INT_MAX - 1.
> >
> > And you specify in LTP test range: from -1 to INT_MAX.
> >
> > But even much shorter value than INT_MAX fails:
> >
> > $ echo 1234567 > /proc/sys/kernel/sched_rt_runtime_us
> > sh: echo: write error: Invalid argument
>
> That is because runtime_us must be < period_us by definition, since
> runtime_us defines how much time is allocated from the period_us. I
> guess that this is not described good enough in the kernel docs.
Mind adding a second patch to your series, clarifying
Documentation/scheduler/sched-rt-group.rst?
Thanks,
Ingo
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us
2023-09-22 9:46 ` Ingo Molnar
@ 2023-09-22 11:43 ` Cyril Hrubis
0 siblings, 0 replies; 6+ messages in thread
From: Cyril Hrubis @ 2023-09-22 11:43 UTC (permalink / raw)
To: Ingo Molnar
Cc: Juri Lelli, Valentin Schneider, Vincent Guittot, Peter Zijlstra,
linux-kernel, Steven Rostedt, Ben Segall, Ingo Molnar, Mel Gorman,
Daniel Bristot de Oliveira, Dietmar Eggemann, ltp
Hi!
> > > Documentation/scheduller/sched-rt-group.rst [1] specifies this as values from -1 to
> > > (INT_MAX - 1), I guess due int range. Looking into proc_dointvec_minmax() [2]
> > > even INT_MAX would pass the check. I suppose we can do nothing about that,
> > > because there is no value in sysctl_vals[] which would represent INT_MAX - 1.
> > >
> > > And you specify in LTP test range: from -1 to INT_MAX.
> > >
> > > But even much shorter value than INT_MAX fails:
> > >
> > > $ echo 1234567 > /proc/sys/kernel/sched_rt_runtime_us
> > > sh: echo: write error: Invalid argument
> >
> > That is because runtime_us must be < period_us by definition, since
> > runtime_us defines how much time is allocated from the period_us. I
> > guess that this is not described good enough in the kernel docs.
>
> Mind adding a second patch to your series, clarifying
> Documentation/scheduler/sched-rt-group.rst?
Yes, that is the plan. I've been working on LTP release for past two
weeks so this has been postponed. I will get to it hopefully next week.
--
Cyril Hrubis
chrubis@suse.cz
--
Mailing list info: https://lists.linux.it/listinfo/ltp
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-22 11:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-01 15:43 [LTP] [PATCH] sched/rt: Disallow writing invalid values to sched_rt_period_us Cyril Hrubis
2023-09-01 17:45 ` Petr Vorel
2023-09-01 18:17 ` Cyril Hrubis
2023-09-02 20:22 ` Petr Vorel
2023-09-22 9:46 ` Ingo Molnar
2023-09-22 11:43 ` Cyril Hrubis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox