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

[PATCH v3] 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().

v3:
 - 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 +++++------
 1 file changed, 5 insertions(+), 6 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();
-- 
2.25.1

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

[Yajun Deng]

There is a problem with this version, please ignore it.



May 17, 2022 2:18 PM, "Yajun Deng" <yajun.deng@linux.dev> 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().
> 
> v3:
> - 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 +++++------
> 1 file changed, 5 insertions(+), 6 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();
> -- 
> 2.25.1