| Message ID | 20231031125114.5879-1-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] sched: add sched sysctl sanity test | expand |
Hi! > Currently the test fails due to kernel bug, I will send patch to LKML > later on. Should have removed this line, the patch has been pulled into mainline today. > The problem with kernel is that sysctl_sched_rt_period is unsigned int > but it's processed with proc_dointvec() which means that you are allowed > to write negative values into the variable even though documentation > says it shouldn't be possible and the kernel code asserts that rt_period > is > 0. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > > - Fixed a few whitespaces > - Added second kernel commit into metadata > > runtest/sched | 2 + > testcases/kernel/sched/sysctl/.gitignore | 1 + > testcases/kernel/sched/sysctl/Makefile | 7 + > .../kernel/sched/sysctl/proc_sched_rt01.c | 122 ++++++++++++++++++ > 4 files changed, 132 insertions(+) > create mode 100644 testcases/kernel/sched/sysctl/.gitignore > create mode 100644 testcases/kernel/sched/sysctl/Makefile > create mode 100644 testcases/kernel/sched/sysctl/proc_sched_rt01.c > > diff --git a/runtest/sched b/runtest/sched > index 172fe4174..3457114f4 100644 > --- a/runtest/sched > +++ b/runtest/sched > @@ -16,3 +16,5 @@ sched_cli_serv run_sched_cliserv.sh > sched_stress sched_stress.sh > > autogroup01 autogroup01 > + > +proc_sched_rt01 > diff --git a/testcases/kernel/sched/sysctl/.gitignore b/testcases/kernel/sched/sysctl/.gitignore > new file mode 100644 > index 000000000..29b859b81 > --- /dev/null > +++ b/testcases/kernel/sched/sysctl/.gitignore > @@ -0,0 +1 @@ > +proc_sched_rt01 > diff --git a/testcases/kernel/sched/sysctl/Makefile b/testcases/kernel/sched/sysctl/Makefile > new file mode 100644 > index 000000000..18896b6f2 > --- /dev/null > +++ b/testcases/kernel/sched/sysctl/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0-or-later > + > +top_srcdir ?= ../../../.. > + > +include $(top_srcdir)/include/mk/testcases.mk > + > +include $(top_srcdir)/include/mk/generic_leaf_target.mk > diff --git a/testcases/kernel/sched/sysctl/proc_sched_rt01.c b/testcases/kernel/sched/sysctl/proc_sched_rt01.c > new file mode 100644 > index 000000000..203846ae4 > --- /dev/null > +++ b/testcases/kernel/sched/sysctl/proc_sched_rt01.c > @@ -0,0 +1,122 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) Cyril Hrubis <chrubis@suse.cz> > + */ > + > +/*\ > + * [Description] > + * > + * Sanity tests for the /proc/sys/kernel/sched_r* files. > + * > + * - The sched_rt_period_us range is 1 to INT_MAX > + * try invalid values and check for EINVAL > + * > + * - The sched_rt_runtime_us range is -1 to INT_MAX > + * try invalid values and check for EINVAL > + * > + * - The sched_rt_runtime_us must be less or equal to sched_rt_period_us > + * > + * - Reset sched_rr_timeslice_ms to default value by writing -1 and check that > + * we get the default value on next read. > + * > + * This is a regression test for a commits: > + * > + * commit c1fc6484e1fb7cc2481d169bfef129a1b0676abe > + * Author: Cyril Hrubis <chrubis@suse.cz> > + * Date: Wed Aug 2 17:19:06 2023 +0200 > + * > + * sched/rt: sysctl_sched_rr_timeslice show default timeslice after reset > + * > + * commit 079be8fc630943d9fc70a97807feb73d169ee3fc > + * Author: Cyril Hrubis <chrubis@suse.cz> > + * Date: Mon Oct 2 13:55:51 2023 +0200 > + * > + * sched/rt: Disallow writing invalid values to sched_rt_period_us > + */ > + > +#include <stdio.h> > +#include "tst_test.h" > + > +#define RT_PERIOD_US "/proc/sys/kernel/sched_rt_period_us" > +#define RT_RUNTIME_US "/proc/sys/kernel/sched_rt_runtime_us" > +#define RR_TIMESLICE_MS "/proc/sys/kernel/sched_rr_timeslice_ms" > + > +static int period_fd; > +static int runtime_fd; > + > +static void rr_timeslice_ms_reset(void) > +{ > + long timeslice_ms; > + > + SAFE_FILE_PRINTF(RR_TIMESLICE_MS, "-1"); > + SAFE_FILE_SCANF(RR_TIMESLICE_MS, "%li", ×lice_ms); > + > + TST_EXP_EXPR(timeslice_ms > 0, > + "timeslice_ms > 0 after reset to default"); > +} > + > +static void rt_period_us_einval(void) > +{ > + TST_EXP_FAIL(write(period_fd, "0", 2), EINVAL, > + "echo 0 > "RT_PERIOD_US); > + TST_EXP_FAIL(write(period_fd, "-1", 2), EINVAL, > + "echo -1 > "RT_PERIOD_US); > +} > + > +static void rt_runtime_us_einval(void) > +{ > + TST_EXP_FAIL(write(runtime_fd, "-2", 2), EINVAL, > + "echo -2 > "RT_RUNTIME_US); > +} > + > +static void rt_runtime_us_le_period_us(void) > +{ > + int period_us; > + char buf[32]; > + > + SAFE_FILE_SCANF(RT_PERIOD_US, "%i", &period_us); > + > + sprintf(buf, "%i", period_us+1); > + > + TST_EXP_FAIL(write(runtime_fd, buf, strlen(buf)), EINVAL, > + "echo rt_period_us+1 > "RT_RUNTIME_US); > +} > + > +static void verify_sched_proc(void) > +{ > + rr_timeslice_ms_reset(); > + rt_period_us_einval(); > + rt_runtime_us_einval(); > + rt_runtime_us_le_period_us(); > +} > + > +static void setup(void) > +{ > + period_fd = open(RT_PERIOD_US, O_RDWR); > + runtime_fd = open(RT_RUNTIME_US, O_RDWR); > +} > + > +static void cleanup(void) > +{ > + if (period_fd > 0) > + SAFE_CLOSE(period_fd); > + > + if (runtime_fd > 0) > + SAFE_CLOSE(runtime_fd); > +} > + > +static struct tst_test test = { > + .needs_root = 1, > + .setup = setup, > + .cleanup = cleanup, > + .test_all = verify_sched_proc, > + .tags = (struct tst_tag []) { > + {"linux-git", "c1fc6484e1fb"}, > + {"linux-git", "079be8fc6309"}, > + {} > + }, > + .needs_kconfigs = (const char *[]) { > + "CONFIG_SYSCTL", > + NULL > + }, > +}; > -- > 2.41.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
Hi! Ping?
Hi Cyril,
It looks like the second fix 079be8fc6309 ("sched/rt: Disallow writing invalid
values to sched_rt_period_us") is not in stable, shouldn't be there?
> Currently the test fails due to kernel bug, I will send patch to LKML
> later on.
> The problem with kernel is that sysctl_sched_rt_period is unsigned int
> but it's processed with proc_dointvec() which means that you are allowed
> to write negative values into the variable even though documentation
> says it shouldn't be possible and the kernel code asserts that rt_period
> is > 0.
> Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
> ---
> - Fixed a few whitespaces
> - Added second kernel commit into metadata
nit: metadata of the git commit looks ugly, but we would need to agree on better
solution.
> runtest/sched | 2 +
> testcases/kernel/sched/sysctl/.gitignore | 1 +
> testcases/kernel/sched/sysctl/Makefile | 7 +
> .../kernel/sched/sysctl/proc_sched_rt01.c | 122 ++++++++++++++++++
> 4 files changed, 132 insertions(+)
> create mode 100644 testcases/kernel/sched/sysctl/.gitignore
> create mode 100644 testcases/kernel/sched/sysctl/Makefile
> create mode 100644 testcases/kernel/sched/sysctl/proc_sched_rt01.c
> diff --git a/runtest/sched b/runtest/sched
> index 172fe4174..3457114f4 100644
> --- a/runtest/sched
> +++ b/runtest/sched
> @@ -16,3 +16,5 @@ sched_cli_serv run_sched_cliserv.sh
> sched_stress sched_stress.sh
> autogroup01 autogroup01
> +
> +proc_sched_rt01
This should obviously be:
proc_sched_rt01 proc_sched_rt01
And I would move it above sched_stress.sh.
The rest LGTM, thanks!
Reviewed-by: Petr Vorel <pvorel@suse.cz>
Kind regards,
Petr
Hi! > It looks like the second fix 079be8fc6309 ("sched/rt: Disallow writing invalid > values to sched_rt_period_us") is not in stable, shouldn't be there? No idea to be honest, the second fix about rejecting invalid values is a minor fix. > > - Fixed a few whitespaces > > - Added second kernel commit into metadata > nit: metadata of the git commit looks ugly, but we would need to agree on better > solution. Which metadata exactly? This part is after the scisors and will be cut when the patch is applied. > > runtest/sched | 2 + > > testcases/kernel/sched/sysctl/.gitignore | 1 + > > testcases/kernel/sched/sysctl/Makefile | 7 + > > .../kernel/sched/sysctl/proc_sched_rt01.c | 122 ++++++++++++++++++ > > 4 files changed, 132 insertions(+) > > create mode 100644 testcases/kernel/sched/sysctl/.gitignore > > create mode 100644 testcases/kernel/sched/sysctl/Makefile > > create mode 100644 testcases/kernel/sched/sysctl/proc_sched_rt01.c > > > diff --git a/runtest/sched b/runtest/sched > > index 172fe4174..3457114f4 100644 > > --- a/runtest/sched > > +++ b/runtest/sched > > @@ -16,3 +16,5 @@ sched_cli_serv run_sched_cliserv.sh > > sched_stress sched_stress.sh > > > autogroup01 autogroup01 > > + > > +proc_sched_rt01 > > This should obviously be: > proc_sched_rt01 proc_sched_rt01 > > And I would move it above sched_stress.sh. Goot catch, will fix. > The rest LGTM, thanks! > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Hi! > > It looks like the second fix 079be8fc6309 ("sched/rt: Disallow writing invalid > > values to sched_rt_period_us") is not in stable, shouldn't be there? > No idea to be honest, the second fix about rejecting invalid values is a > minor fix. IMHO it's a fix of broken functionality. And Greg stated several times that stable is not only for security fixes but for other fixes. Therefore I would vote for adding it. > > > - Fixed a few whitespaces > > > - Added second kernel commit into metadata > > nit: metadata of the git commit looks ugly, but we would need to agree on better > > solution. > Which metadata exactly? This part is after the scisors and will be cut > when the patch is applied. I don't like formatting after "This is a regression test for a commits:". It formates like code block (gray background) via using <pre>...</pre> HTML tags: commit c1fc6484e1fb7cc2481d169bfef129a1b0676abe Author: Cyril Hrubis <chrubis@suse.cz> Date: Wed Aug 2 17:19:06 2023 +0200 sched/rt: sysctl_sched_rr_timeslice show default timeslice after reset commit 079be8fc630943d9fc70a97807feb73d169ee3fc Author: Cyril Hrubis <chrubis@suse.cz> Date: Mon Oct 2 13:55:51 2023 +0200 sched/rt: Disallow writing invalid values to sched_rt_period_us (see a real output in metadata.html) Given we have this in the Tag table below, I found it useless. I mean, we could have it, but not in docparse comment /*\ ... */, but in normal C comment /* ... */ (for reader of the C source, but not in docparse format). Or, if we want to have it in docparse output, we could format it as: /*\ ... * This is a regression test for a commits: * * c1fc6484e1fb ("sched/rt: sysctl_sched_rr_timeslice show default timeslice after reset") * * 079be8fc6309 ("sched/rt: Disallow writing invalid values to sched_rt_period_us") This is not unique to this commit, we don't have to solve it now (and it's definitely a blocker for this patch). But it would be good to find a consensus how the output should look like. Kind regards, Petr
Hi! > The rest LGTM, thanks! > Reviewed-by: Petr Vorel <pvorel@suse.cz> Fixed all the problems and pushed, thanks.
diff --git a/runtest/sched b/runtest/sched index 172fe4174..3457114f4 100644 --- a/runtest/sched +++ b/runtest/sched @@ -16,3 +16,5 @@ sched_cli_serv run_sched_cliserv.sh sched_stress sched_stress.sh autogroup01 autogroup01 + +proc_sched_rt01 diff --git a/testcases/kernel/sched/sysctl/.gitignore b/testcases/kernel/sched/sysctl/.gitignore new file mode 100644 index 000000000..29b859b81 --- /dev/null +++ b/testcases/kernel/sched/sysctl/.gitignore @@ -0,0 +1 @@ +proc_sched_rt01 diff --git a/testcases/kernel/sched/sysctl/Makefile b/testcases/kernel/sched/sysctl/Makefile new file mode 100644 index 000000000..18896b6f2 --- /dev/null +++ b/testcases/kernel/sched/sysctl/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0-or-later + +top_srcdir ?= ../../../.. + +include $(top_srcdir)/include/mk/testcases.mk + +include $(top_srcdir)/include/mk/generic_leaf_target.mk diff --git a/testcases/kernel/sched/sysctl/proc_sched_rt01.c b/testcases/kernel/sched/sysctl/proc_sched_rt01.c new file mode 100644 index 000000000..203846ae4 --- /dev/null +++ b/testcases/kernel/sched/sysctl/proc_sched_rt01.c @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) Cyril Hrubis <chrubis@suse.cz> + */ + +/*\ + * [Description] + * + * Sanity tests for the /proc/sys/kernel/sched_r* files. + * + * - The sched_rt_period_us range is 1 to INT_MAX + * try invalid values and check for EINVAL + * + * - The sched_rt_runtime_us range is -1 to INT_MAX + * try invalid values and check for EINVAL + * + * - The sched_rt_runtime_us must be less or equal to sched_rt_period_us + * + * - Reset sched_rr_timeslice_ms to default value by writing -1 and check that + * we get the default value on next read. + * + * This is a regression test for a commits: + * + * commit c1fc6484e1fb7cc2481d169bfef129a1b0676abe + * Author: Cyril Hrubis <chrubis@suse.cz> + * Date: Wed Aug 2 17:19:06 2023 +0200 + * + * sched/rt: sysctl_sched_rr_timeslice show default timeslice after reset + * + * commit 079be8fc630943d9fc70a97807feb73d169ee3fc + * Author: Cyril Hrubis <chrubis@suse.cz> + * Date: Mon Oct 2 13:55:51 2023 +0200 + * + * sched/rt: Disallow writing invalid values to sched_rt_period_us + */ + +#include <stdio.h> +#include "tst_test.h" + +#define RT_PERIOD_US "/proc/sys/kernel/sched_rt_period_us" +#define RT_RUNTIME_US "/proc/sys/kernel/sched_rt_runtime_us" +#define RR_TIMESLICE_MS "/proc/sys/kernel/sched_rr_timeslice_ms" + +static int period_fd; +static int runtime_fd; + +static void rr_timeslice_ms_reset(void) +{ + long timeslice_ms; + + SAFE_FILE_PRINTF(RR_TIMESLICE_MS, "-1"); + SAFE_FILE_SCANF(RR_TIMESLICE_MS, "%li", ×lice_ms); + + TST_EXP_EXPR(timeslice_ms > 0, + "timeslice_ms > 0 after reset to default"); +} + +static void rt_period_us_einval(void) +{ + TST_EXP_FAIL(write(period_fd, "0", 2), EINVAL, + "echo 0 > "RT_PERIOD_US); + TST_EXP_FAIL(write(period_fd, "-1", 2), EINVAL, + "echo -1 > "RT_PERIOD_US); +} + +static void rt_runtime_us_einval(void) +{ + TST_EXP_FAIL(write(runtime_fd, "-2", 2), EINVAL, + "echo -2 > "RT_RUNTIME_US); +} + +static void rt_runtime_us_le_period_us(void) +{ + int period_us; + char buf[32]; + + SAFE_FILE_SCANF(RT_PERIOD_US, "%i", &period_us); + + sprintf(buf, "%i", period_us+1); + + TST_EXP_FAIL(write(runtime_fd, buf, strlen(buf)), EINVAL, + "echo rt_period_us+1 > "RT_RUNTIME_US); +} + +static void verify_sched_proc(void) +{ + rr_timeslice_ms_reset(); + rt_period_us_einval(); + rt_runtime_us_einval(); + rt_runtime_us_le_period_us(); +} + +static void setup(void) +{ + period_fd = open(RT_PERIOD_US, O_RDWR); + runtime_fd = open(RT_RUNTIME_US, O_RDWR); +} + +static void cleanup(void) +{ + if (period_fd > 0) + SAFE_CLOSE(period_fd); + + if (runtime_fd > 0) + SAFE_CLOSE(runtime_fd); +} + +static struct tst_test test = { + .needs_root = 1, + .setup = setup, + .cleanup = cleanup, + .test_all = verify_sched_proc, + .tags = (struct tst_tag []) { + {"linux-git", "c1fc6484e1fb"}, + {"linux-git", "079be8fc6309"}, + {} + }, + .needs_kconfigs = (const char *[]) { + "CONFIG_SYSCTL", + NULL + }, +};
Currently the test fails due to kernel bug, I will send patch to LKML later on. The problem with kernel is that sysctl_sched_rt_period is unsigned int but it's processed with proc_dointvec() which means that you are allowed to write negative values into the variable even though documentation says it shouldn't be possible and the kernel code asserts that rt_period is > 0. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- - Fixed a few whitespaces - Added second kernel commit into metadata runtest/sched | 2 + testcases/kernel/sched/sysctl/.gitignore | 1 + testcases/kernel/sched/sysctl/Makefile | 7 + .../kernel/sched/sysctl/proc_sched_rt01.c | 122 ++++++++++++++++++ 4 files changed, 132 insertions(+) create mode 100644 testcases/kernel/sched/sysctl/.gitignore create mode 100644 testcases/kernel/sched/sysctl/Makefile create mode 100644 testcases/kernel/sched/sysctl/proc_sched_rt01.c