[PATCH v4] sched/rt: fix the case where sched_rt_period_us is negative

[PATCH v4] sched/rt: fix the case where sched_rt_period_us is negative

[Yajun Deng]

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.

Make sysctl_sched_rt_period integer to match proc_dointvec_minmax().

v4:
 - Make sysctl_sched_rt_period integer (Valentin Schneider)
v2:
 - Remove sched_rr_timeslice_ms related changes (Valentin Schneider)

Fixes: d0b27fa77854 ("sched: rt-group: synchonised bandwidth period")
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 kernel/sched/rt.c    | 11 +++++------
 kernel/sched/sched.h |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 8c9ed9664840..cafc580edbe4 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -16,7 +16,7 @@ struct rt_bandwidth def_rt_bandwidth;
  * period over which we measure -rt task CPU usage in us.
  * default: 1s
  */
-unsigned int sysctl_sched_rt_period = 1000000;
+int sysctl_sched_rt_period = 1000000;
 
 /*
  * part of the period that we allow rt tasks to run in us.
@@ -34,9 +34,10 @@ static struct ctl_table sched_rt_sysctls[] = {
 	{
 		.procname       = "sched_rt_period_us",
 		.data           = &sysctl_sched_rt_period,
-		.maxlen         = sizeof(unsigned int),
+		.maxlen         = sizeof(int),
 		.mode           = 0644,
 		.proc_handler   = sched_rt_handler,
+		.extra1		= SYSCTL_ONE,
 	},
 	{
 		.procname       = "sched_rt_runtime_us",
@@ -44,6 +45,7 @@ static struct ctl_table sched_rt_sysctls[] = {
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
 		.proc_handler   = sched_rt_handler,
+		.extra1		= SYSCTL_NEG_ONE,
 	},
 	{
 		.procname       = "sched_rr_timeslice_ms",
@@ -2960,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 *
@@ -2993,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();
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1f97f357aacd..1cb4d47b3e47 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -115,7 +115,7 @@ extern long calc_load_fold_active(struct rq *this_rq, long adjust);
 
 extern void call_trace_sched_update_nr_running(struct rq *rq, int count);
 
-extern unsigned int sysctl_sched_rt_period;
+extern int sysctl_sched_rt_period;
 extern int sysctl_sched_rt_runtime;
 extern int sched_rr_timeslice;
 
-- 
2.25.1

Re: [PATCH v4] sched/rt: fix the case where sched_rt_period_us is negative

[Valentin Schneider]

On 17/05/22 14:29, 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.
>
> Make sysctl_sched_rt_period integer to match proc_dointvec_minmax().
>

How about:


While sysctl_sched_rt_runtime is a signed integer and
sysctl_sched_rt_period is unsigned, both are handled in sched_rt_handler()
via proc_dointvec(), so negative inputs can be fed into
sysctl_sched_rt_period. However, per sched-rt-group.rst:

  * sched_rt_period_us takes values from 1 to INT_MAX.
  * sched_rt_runtime_us takes values from -1 to (INT_MAX - 1).

Use proc_dointvec_minmax() instead of proc_dointvec() and use the .extra1
parameter to enforce a minimum value.

Make sysctl_sched_rt_period a signed integer as this matches the expected
upper boundary and the expected type of proc_dointvec_minmax().

> v4:
>  - Make sysctl_sched_rt_period integer (Valentin Schneider)

Even if v3 was bogus, it's good not to skip it in the version log.
Also, the version logs should be after the "---" marker line:

Documentation/process/submitting-patches.rt
"""
Please put this information **after** the ``---`` line which separates
the changelog from the rest of the patch. The version information is
not part of the changelog which gets committed to the git tree. It is
additional information for the reviewers. If it's placed above the
commit tags, it needs manual interaction to remove it. If it is below
the separator line, it gets automatically stripped off when applying the
patch
"""

> v2:
>  - Remove sched_rr_timeslice_ms related changes (Valentin Schneider)
>
> Fixes: d0b27fa77854 ("sched: rt-group: synchonised bandwidth period")
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>

Reviewed-by: Valentin Schneider <vschneid@redhat.com>

Re: [PATCH v4] sched/rt: fix the case where sched_rt_period_us is negative

[Yajun Deng]

Ping.



May 18, 2022 12:08 AM, "Valentin Schneider" <vschneid@redhat.com> wrote:

> On 17/05/22 14:29, 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.
>> 
>> Make sysctl_sched_rt_period integer to match proc_dointvec_minmax().
> 
> How about:
> 
> While sysctl_sched_rt_runtime is a signed integer and
> sysctl_sched_rt_period is unsigned, both are handled in sched_rt_handler()
> via proc_dointvec(), so negative inputs can be fed into
> sysctl_sched_rt_period. However, per sched-rt-group.rst:
> 
> * sched_rt_period_us takes values from 1 to INT_MAX.
> * sched_rt_runtime_us takes values from -1 to (INT_MAX - 1).
> 
> Use proc_dointvec_minmax() instead of proc_dointvec() and use the .extra1
> parameter to enforce a minimum value.
> 
> Make sysctl_sched_rt_period a signed integer as this matches the expected
> upper boundary and the expected type of proc_dointvec_minmax().
> 
>> v4:
>> - Make sysctl_sched_rt_period integer (Valentin Schneider)
> 
> Even if v3 was bogus, it's good not to skip it in the version log.
> Also, the version logs should be after the "---" marker line:
> 
> Documentation/process/submitting-patches.rt
> """
> Please put this information **after** the ``---`` line which separates
> the changelog from the rest of the patch. The version information is
> not part of the changelog which gets committed to the git tree. It is
> additional information for the reviewers. If it's placed above the
> commit tags, it needs manual interaction to remove it. If it is below
> the separator line, it gets automatically stripped off when applying the
> patch
> """
> 
>> v2:
>> - Remove sched_rr_timeslice_ms related changes (Valentin Schneider)
>> 
>> Fixes: d0b27fa77854 ("sched: rt-group: synchonised bandwidth period")
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> 
> Reviewed-by: Valentin Schneider <vschneid@redhat.com>