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

[PATCH v2] 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.

Fixes: 391e43da797a ("sched: Move all scheduler bits into kernel/sched/")
Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 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,
 	},
 	{
 		.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,
 	},
 	{
 		.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

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

[Valentin Schneider]

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

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

[Yajun Deng]

May 16, 2022 11:04 PM, "Valentin Schneider" <vschneid@redhat.com> wrote:

> 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")
> 

I know that, but I didn't find out the source. Thank you for helping me find out it.

>> 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).
> 

Got it, I will add it to the next version.

>> 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?
> 

Yes, we can make sysctl_sched_rt_period int.
>> },
>> {
>> .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?
>

No, the extra2 just limit the maximum value of sysctl_sched_rt_runtime is sysctl_sched_rt_period, but not limit the minimum value of sysctl_sched_rt_period is sysctl_sched_rt_runtime. (sysctl_sched_rt_runtime > sysctl_sched_rt_period) can do both. 
Its purpose is to return error earlier. Perhaps I should remove extra2 to avoid ambiguity.

>> }, 
>> {
>> .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

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

[Valentin Schneider]

On 17/05/22 01:55, Yajun Deng wrote:
> May 16, 2022 11:04 PM, "Valentin Schneider" <vschneid@redhat.com> wrote:
>>> },
>>> {
>>> .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?
>>
>
> No, the extra2 just limit the maximum value of sysctl_sched_rt_runtime is sysctl_sched_rt_period, but not limit the minimum value of sysctl_sched_rt_period is sysctl_sched_rt_runtime. (sysctl_sched_rt_runtime > sysctl_sched_rt_period) can do both.

Gotcha.

> Its purpose is to return error earlier. Perhaps I should remove extra2 to avoid ambiguity.
>

It's probably better to only have the "pure" bounds in there yes.