[PATCH v2 0/2] Fix sched-rt sysctl files & update docs

[PATCH v2 0/2] Fix sched-rt sysctl files & update docs

[Cyril Hrubis]

- First patch fixes sched-rt sysctl files to disallow writing invalid
  values

- Second patch is new in v2 and clarifies and fixes the documentation
  for these files and is actually independent of the first patch

Cyril Hrubis (2):
  sched/rt: Disallow writing invalid values to sched_rt_period_us
  docs: scheduler-rt: Clarify & fix sched_rt_* sysctl docs

 Documentation/scheduler/sched-rt-group.rst | 14 ++++++++------
 kernel/sched/rt.c                          |  9 +++++----
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.41.0

[LTP] [PATCH v2 0/2] Fix sched-rt sysctl files & update docs

[Cyril Hrubis]

- First patch fixes sched-rt sysctl files to disallow writing invalid
  values

- Second patch is new in v2 and clarifies and fixes the documentation
  for these files and is actually independent of the first patch

Cyril Hrubis (2):
  sched/rt: Disallow writing invalid values to sched_rt_period_us
  docs: scheduler-rt: Clarify & fix sched_rt_* sysctl docs

 Documentation/scheduler/sched-rt-group.rst | 14 ++++++++------
 kernel/sched/rt.c                          |  9 +++++----
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.41.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[PATCH v2 1/2] sched/rt: Disallow writing invalid values to sched_rt_period_us

[Cyril Hrubis]

The validation of the value written to sched_rt_period_us was broken
because:

- the sysclt_sched_rt_period is declared as unsigned int
- parsed by proc_do_intvec()
- the range is asserted after the value parsed by proc_do_intvec()

Because of this negative values written to the file were written into a
unsigned integer that were later on interpreted as large positive
integers which did passed the check:

if (sysclt_sched_rt_period <= 0)
	return EINVAL;

This commit fixes the parsing by setting explicit range for both
perid_us and runtime_us into the sched_rt_sysctls table and processes
the values with proc_dointvec_minmax() instead.

Alternatively if we wanted to use full range of unsigned int for the
period value we would have to split the proc_handler and use
proc_douintvec() for it however even the
Documentation/scheduller/sched-rt-group.rst describes the range as 1 to
INT_MAX.

As far as I can tell the only problem this causes is that the sysctl
file allows writing negative values which when read back may confuse
userspace.

There is also a LTP test being submitted for these sysctl files at:

http://patchwork.ozlabs.org/project/ltp/patch/20230901144433.2526-1-chrubis@suse.cz/

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 kernel/sched/rt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 0597ba0f85ff..aed3d55de2dd 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -37,6 +37,8 @@ static struct ctl_table sched_rt_sysctls[] = {
 		.maxlen         = sizeof(unsigned int),
 		.mode           = 0644,
 		.proc_handler   = sched_rt_handler,
+		.extra1         = SYSCTL_ONE,
+		.extra2         = SYSCTL_INT_MAX,
 	},
 	{
 		.procname       = "sched_rt_runtime_us",
@@ -44,6 +46,8 @@ static struct ctl_table sched_rt_sysctls[] = {
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
 		.proc_handler   = sched_rt_handler,
+		.extra1         = SYSCTL_NEG_ONE,
+		.extra2         = SYSCTL_INT_MAX,
 	},
 	{
 		.procname       = "sched_rr_timeslice_ms",
@@ -2985,9 +2989,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 *
@@ -3018,7 +3019,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.41.0

[LTP] [PATCH v2 1/2] sched/rt: Disallow writing invalid values to sched_rt_period_us

[Cyril Hrubis]

The validation of the value written to sched_rt_period_us was broken
because:

- the sysclt_sched_rt_period is declared as unsigned int
- parsed by proc_do_intvec()
- the range is asserted after the value parsed by proc_do_intvec()

Because of this negative values written to the file were written into a
unsigned integer that were later on interpreted as large positive
integers which did passed the check:

if (sysclt_sched_rt_period <= 0)
	return EINVAL;

This commit fixes the parsing by setting explicit range for both
perid_us and runtime_us into the sched_rt_sysctls table and processes
the values with proc_dointvec_minmax() instead.

Alternatively if we wanted to use full range of unsigned int for the
period value we would have to split the proc_handler and use
proc_douintvec() for it however even the
Documentation/scheduller/sched-rt-group.rst describes the range as 1 to
INT_MAX.

As far as I can tell the only problem this causes is that the sysctl
file allows writing negative values which when read back may confuse
userspace.

There is also a LTP test being submitted for these sysctl files at:

http://patchwork.ozlabs.org/project/ltp/patch/20230901144433.2526-1-chrubis@suse.cz/

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 kernel/sched/rt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 0597ba0f85ff..aed3d55de2dd 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -37,6 +37,8 @@ static struct ctl_table sched_rt_sysctls[] = {
 		.maxlen         = sizeof(unsigned int),
 		.mode           = 0644,
 		.proc_handler   = sched_rt_handler,
+		.extra1         = SYSCTL_ONE,
+		.extra2         = SYSCTL_INT_MAX,
 	},
 	{
 		.procname       = "sched_rt_runtime_us",
@@ -44,6 +46,8 @@ static struct ctl_table sched_rt_sysctls[] = {
 		.maxlen         = sizeof(int),
 		.mode           = 0644,
 		.proc_handler   = sched_rt_handler,
+		.extra1         = SYSCTL_NEG_ONE,
+		.extra2         = SYSCTL_INT_MAX,
 	},
 	{
 		.procname       = "sched_rr_timeslice_ms",
@@ -2985,9 +2989,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 *
@@ -3018,7 +3019,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.41.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[PATCH v2 2/2] docs: scheduler-rt: Clarify & fix sched_rt_* sysctl docs

[Cyril Hrubis]

- Describe explicitly that sched_rt_runtime_us is allocated from
  sched_rt_period_us and hence always less or equal to that value.

- The limit for sched_rt_runtime_us is not INT_MAX - 1 but rather it's
  limited by the value of sched_rt_period_us. If sched_rt_period_us is
  INT_MAX then sched_rt_runtime_us can be set to INT_MAX as well.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 Documentation/scheduler/sched-rt-group.rst | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/scheduler/sched-rt-group.rst b/Documentation/scheduler/sched-rt-group.rst
index 655a096ec8fb..033661a5838f 100644
--- a/Documentation/scheduler/sched-rt-group.rst
+++ b/Documentation/scheduler/sched-rt-group.rst
@@ -87,18 +87,20 @@ lack an EDF scheduler to make non-uniform periods usable.
 The system wide settings are configured under the /proc virtual file system:
 
 /proc/sys/kernel/sched_rt_period_us:
-  The scheduling period that is equivalent to 100% CPU bandwidth
+  The scheduling period that is equivalent to 100% CPU bandwidth.
 
 /proc/sys/kernel/sched_rt_runtime_us:
-  A global limit on how much time realtime scheduling may use.  Even without
-  CONFIG_RT_GROUP_SCHED enabled, this will limit time reserved to realtime
-  processes. With CONFIG_RT_GROUP_SCHED it signifies the total bandwidth
-  available to all realtime groups.
+  A global limit on how much time realtime scheduling may use. This is always
+  less or equal than the period_us as it denotes the time allocated from the
+  period_us for the realtime tasks. Even without CONFIG_RT_GROUP_SCHED enabled,
+  this will limit time reserved to realtime processes. With
+  CONFIG_RT_GROUP_SCHED it signifies the total bandwidth available to all
+  realtime groups.
 
   * Time is specified in us because the interface is s32. This gives an
     operating range from 1us to about 35 minutes.
   * sched_rt_period_us takes values from 1 to INT_MAX.
-  * sched_rt_runtime_us takes values from -1 to (INT_MAX - 1).
+  * sched_rt_runtime_us takes values from -1 to sched_rt_period_us.
   * A run time of -1 specifies runtime == period, ie. no limit.
 
 
-- 
2.41.0

[LTP] [PATCH v2 2/2] docs: scheduler-rt: Clarify & fix sched_rt_* sysctl docs

[Cyril Hrubis]

- Describe explicitly that sched_rt_runtime_us is allocated from
  sched_rt_period_us and hence always less or equal to that value.

- The limit for sched_rt_runtime_us is not INT_MAX - 1 but rather it's
  limited by the value of sched_rt_period_us. If sched_rt_period_us is
  INT_MAX then sched_rt_runtime_us can be set to INT_MAX as well.

Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
 Documentation/scheduler/sched-rt-group.rst | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/Documentation/scheduler/sched-rt-group.rst b/Documentation/scheduler/sched-rt-group.rst
index 655a096ec8fb..033661a5838f 100644
--- a/Documentation/scheduler/sched-rt-group.rst
+++ b/Documentation/scheduler/sched-rt-group.rst
@@ -87,18 +87,20 @@ lack an EDF scheduler to make non-uniform periods usable.
 The system wide settings are configured under the /proc virtual file system:
 
 /proc/sys/kernel/sched_rt_period_us:
-  The scheduling period that is equivalent to 100% CPU bandwidth
+  The scheduling period that is equivalent to 100% CPU bandwidth.
 
 /proc/sys/kernel/sched_rt_runtime_us:
-  A global limit on how much time realtime scheduling may use.  Even without
-  CONFIG_RT_GROUP_SCHED enabled, this will limit time reserved to realtime
-  processes. With CONFIG_RT_GROUP_SCHED it signifies the total bandwidth
-  available to all realtime groups.
+  A global limit on how much time realtime scheduling may use. This is always
+  less or equal than the period_us as it denotes the time allocated from the
+  period_us for the realtime tasks. Even without CONFIG_RT_GROUP_SCHED enabled,
+  this will limit time reserved to realtime processes. With
+  CONFIG_RT_GROUP_SCHED it signifies the total bandwidth available to all
+  realtime groups.
 
   * Time is specified in us because the interface is s32. This gives an
     operating range from 1us to about 35 minutes.
   * sched_rt_period_us takes values from 1 to INT_MAX.
-  * sched_rt_runtime_us takes values from -1 to (INT_MAX - 1).
+  * sched_rt_runtime_us takes values from -1 to sched_rt_period_us.
   * A run time of -1 specifies runtime == period, ie. no limit.
 
 
-- 
2.41.0


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [PATCH v2 2/2] docs: scheduler-rt: Clarify & fix sched_rt_* sysctl docs

[Ingo Molnar]

* Cyril Hrubis <chrubis@suse.cz> wrote:

> - Describe explicitly that sched_rt_runtime_us is allocated from
>   sched_rt_period_us and hence always less or equal to that value.

Just some spelling nits:

> - The limit for sched_rt_runtime_us is not INT_MAX - 1 but rather it's
>   limited by the value of sched_rt_period_us. If sched_rt_period_us is
>   INT_MAX then sched_rt_runtime_us can be set to INT_MAX as well.

s/is not INT_MAX - 1 but rather it's
 /is not INT_MAX-1, but rather it's

>  /proc/sys/kernel/sched_rt_runtime_us:
> -  A global limit on how much time realtime scheduling may use.  Even without
> -  CONFIG_RT_GROUP_SCHED enabled, this will limit time reserved to realtime
> -  processes. With CONFIG_RT_GROUP_SCHED it signifies the total bandwidth
> -  available to all realtime groups.
> +  A global limit on how much time realtime scheduling may use. This is always

s/realtime
 /real-time

> +  less or equal than the period_us as it denotes the time allocated from the

s/than the period_us as it denotes
 /than the period_us, as it denotes

> +  period_us for the realtime tasks. Even without CONFIG_RT_GROUP_SCHED enabled,
> +  this will limit time reserved to realtime processes. With

s/realtime
 /real-time

> +  CONFIG_RT_GROUP_SCHED it signifies the total bandwidth available to all

s/CONFIG_RT_GROUP_SCHED
 /CONFIG_RT_GROUP_SCHED=y

> +  realtime groups.

s/realtime
 /real-time

Thanks,

	Ingo

Re: [LTP] [PATCH v2 2/2] docs: scheduler-rt: Clarify & fix sched_rt_* sysctl docs

[Ingo Molnar]

* Cyril Hrubis <chrubis@suse.cz> wrote:

> - Describe explicitly that sched_rt_runtime_us is allocated from
>   sched_rt_period_us and hence always less or equal to that value.

Just some spelling nits:

> - The limit for sched_rt_runtime_us is not INT_MAX - 1 but rather it's
>   limited by the value of sched_rt_period_us. If sched_rt_period_us is
>   INT_MAX then sched_rt_runtime_us can be set to INT_MAX as well.

s/is not INT_MAX - 1 but rather it's
 /is not INT_MAX-1, but rather it's

>  /proc/sys/kernel/sched_rt_runtime_us:
> -  A global limit on how much time realtime scheduling may use.  Even without
> -  CONFIG_RT_GROUP_SCHED enabled, this will limit time reserved to realtime
> -  processes. With CONFIG_RT_GROUP_SCHED it signifies the total bandwidth
> -  available to all realtime groups.
> +  A global limit on how much time realtime scheduling may use. This is always

s/realtime
 /real-time

> +  less or equal than the period_us as it denotes the time allocated from the

s/than the period_us as it denotes
 /than the period_us, as it denotes

> +  period_us for the realtime tasks. Even without CONFIG_RT_GROUP_SCHED enabled,
> +  this will limit time reserved to realtime processes. With

s/realtime
 /real-time

> +  CONFIG_RT_GROUP_SCHED it signifies the total bandwidth available to all

s/CONFIG_RT_GROUP_SCHED
 /CONFIG_RT_GROUP_SCHED=y

> +  realtime groups.

s/realtime
 /real-time

Thanks,

	Ingo

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp