Re: [PATCH] sched/rt: fix the case where sched_rt_period_us is non-positive

[Valentin Schneider]

On 11/05/22 19:21, Yajun Deng wrote:
> The proc_dointvec() in sched_rt_handler() is for integer, but
> sysctl_sched_rt_period is a unsigned integer. sched_rt_handler() would
> cover a non-positive number to positive number, so both proc_dointvec()
> and sched_rt_global_validate() aren't return error when we set a
> non-positive number.
>
> Use proc_dointvec_minmax() instead of proc_dointvec(), and add extra
> for sched_rt_period_us/sched_rt_runtime_us/sched_rr_timeslice_ms.
>
> Fixes: 391e43da797a ("sched: Move all scheduler bits into kernel/sched/")
> Signed-off-by: Yajun Deng <yajun.deng@xxxxxxxxx>
> ---
>  kernel/sched/rt.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index b491a0f8c25d..f20389aa6731 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,
>       },

Err, what kernel is this based on? AFAICT those have always lived in
kernel/sysctl.c, not kernel/sched/rt.c

>       {
>               .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",
> @@ -51,6 +54,7 @@ static struct ctl_table sched_rt_sysctls[] = {
>               .maxlen         = sizeof(int),
>               .mode           = 0644,
>               .proc_handler   = sched_rr_handler,
> +		.extra1		= SYSCTL_ONE,

Per upstream state of sched_rr_handler:

         * Also, writing zero resets the timeslice to default:

which doesn't happen with that new constraint.

>       },
>       {}
>  };
> @@ -2959,9 +2963,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 +2993,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();
> @@ -3027,14 +3028,13 @@ static int sched_rr_handler(struct ctl_table *table, int write, void *buffer,
>       static DEFINE_MUTEX(mutex);
>
>       mutex_lock(&mutex);
> -	ret = proc_dointvec(table, write, buffer, lenp, ppos);
> +	ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>       /*
>        * Make sure that internally we keep jiffies.
>        * Also, writing zero resets the timeslice to default:
>        */
>       if (!ret && write) {
>               sched_rr_timeslice =
> -			sysctl_sched_rr_timeslice <= 0 ? RR_TIMESLICE :
>                       msecs_to_jiffies(sysctl_sched_rr_timeslice);
>       }
>       mutex_unlock(&mutex);
> --
> 2.25.1