lkml.org 
[lkml]   [2022]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH v2] sched/rt: fix the case where sched_rt_period_us is negative
Date
On 12/05/22 08:39, Yajun Deng wrote:
> The proc_dointvec() is for integer, but sysctl_sched_rt_period is a
> unsigned integer, proc_dointvec() would convert negative number into
> positive number. So both proc_dointvec() and sched_rt_global_validate()
> aren't return error even if we set a negative number.
>
> Use proc_dointvec_minmax() instead of proc_dointvec() and use extra1
> limit the minimum value for sched_rt_period_us/sched_rt_runtime_us.
>
> Fixes: 391e43da797a ("sched: Move all scheduler bits into kernel/sched/")

That's just the last apparent change of the incriminated variable. AFAICT
the issue stems from:

- sysctl_sched_rt_period being unsigned int
- the ctl entry using proc_dointvec()
- the bounds check on sysctl_sched_rt_period being just <= 0 which doesn't
actually respect the [1, INT_MAX] stated in
Documentation/scheduler/sched-rt-group.rst

The <= thing was added by:

ec5d498991e8 ("sched: fix deadlock in setting scheduler parameter to zero")

but AFAICT the unsigned int vs proc_dointvec() thing dates back to:

d0b27fa77854 ("sched: rt-group: synchonised bandwidth period")

> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---

In the absence of a cover letter (e.g. single-patch submission), this is
where you should write patch version changelogs (see
Documentation/process).

> kernel/sched/rt.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index b491a0f8c25d..3add32679885 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -37,6 +37,7 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(unsigned int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> + .extra1 = SYSCTL_ONE,

Per earlier comment, the Documentation says that this needs to be within
[1, INT_MAX], which you do get by excluding negative values when casting to
int...

How about we make sysctl_sched_rt_period int on top of this, then all variables
modified by the sched_rt_handler() proc_dointvec() are *actually* int, and
the upper bound requires less mental gymnastics to be figured out?

> },
> {
> .procname = "sched_rt_runtime_us",
> @@ -44,6 +45,8 @@ static struct ctl_table sched_rt_sysctls[] = {
> .maxlen = sizeof(int),
> .mode = 0644,
> .proc_handler = sched_rt_handler,
> + .extra1 = SYSCTL_NEG_ONE,
> + .extra2 = (void *)&sysctl_sched_rt_period,

Per this, you could also remove the

((sysctl_sched_rt_runtime > sysctl_sched_rt_period) ||

from sched_rt_global_validate(), no?

> },
> {
> .procname = "sched_rr_timeslice_ms",
> @@ -2959,9 +2962,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 *
> @@ -2992,7 +2992,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.25.1

\
 
 \ /
  Last update: 2022-05-16 17:05    [W:0.941 / U:0.944 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site