| Message ID | 20230901144433.2526-1-chrubis@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sched: add sched sysctl sanity test | expand |
Hi! > Currently the test fails due to kernel bug, I will send patch to LKML > later on. https://lkml.org/lkml/2023/9/1/569
Hi Cyril, > Currently the test fails due to kernel bug, I will send patch to LKML > later on. +1. > 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. Interesting. LTP patch uses sometimes spaces instead of tabs: $ make check-proc_sched_rt01 CHECK testcases/kernel/sched/sysctl/proc_sched_rt01.c proc_sched_rt01.c:49: ERROR: code indent should use tabs where possible proc_sched_rt01.c:55: ERROR: code indent should use tabs where possible proc_sched_rt01.c:57: ERROR: code indent should use tabs where possible proc_sched_rt01.c:63: ERROR: code indent should use tabs where possible proc_sched_rt01.c:76: ERROR: code indent should use tabs where possible ... > 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 nit: We usually put / in the front: /proc_sched_rt01 ... > +++ b/testcases/kernel/sched/sysctl/proc_sched_rt01.c > @@ -0,0 +1,115 @@ > +// 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. very nit: If you use dot at the end of this sentence, please add it also to the previous sentences. > + * > + * This is a regression test for a commit: > + * > + * 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 nit: this makes docparse formatting ugly. This would be nicer: c1fc6484e1fb ("sched/rt: sysctl_sched_rr_timeslice show default timeslice after reset") (Unless we really prefer to have the date in the output, but in that case adding a note which kernel version was fixing it would be IMHO more informative than the date.) > + */ > + > +#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, Why not 1 as 3rd write() parameter? > + "echo 0 > "RT_PERIOD_US); nit: I'd add blank line here (readability). > + 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); 4x use of the same code, but I agree it's not worth of creating a function, as the code is simple enough and probably more readable. > +} > + > +static void verify_sched_proc(void) > +{ Is there any value to print content of /proc/sys/kernel/sched_rt_runtime_us before writing into it? The rest LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
Hi Cyril, ... > +++ b/testcases/kernel/sched/sysctl/proc_sched_rt01.c > @@ -0,0 +1,115 @@ > +// 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 As I wrote to the kernel patch, Documentation/scheduller/sched-rt-group.rst [1] specifies this as values from -1 to (INT_MAX - 1). Kind regards, Petr [1] https://www.kernel.org/doc/html/latest/scheduler/sched-rt-group.html#system-wide-settings
Hello, [This is a resend because I had some network issues; sorry if it comes through twice] Cyril Hrubis <chrubis@suse.cz> writes: > 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> > --- > runtest/sched | 2 + > testcases/kernel/sched/sysctl/.gitignore | 1 + > testcases/kernel/sched/sysctl/Makefile | 7 ++ > .../kernel/sched/sysctl/proc_sched_rt01.c | 115 ++++++++++++++++++ > 4 files changed, 125 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..b30256792 > --- /dev/null > +++ b/testcases/kernel/sched/sysctl/proc_sched_rt01.c > @@ -0,0 +1,115 @@ > +// 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 commit: > + * > + * 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 > + */ > + > +#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); > +} I would happily add my tags to the test except that as a general principal, I don't want to deal with tests that fail if an unexpected error number is returned. Unless something can be done (e.g. with meta-data), so that TST_EXP_FAIL and similar can be reduced to a TCONF when the wrong errno is returned. For instance, this would allow running the tests with a seccomp BPF profile, LSM, CGroup or /proc bind mount that blocks the write. Or you could just seperate the tests I guess. Then the reset can be checked while skipping the errno checks. > + > +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"}, > + {} > + }, > + .needs_kconfigs = (const char *[]) { > + "CONFIG_SYSCTL", > + NULL > + }, > +}; > -- > 2.41.0
Hi! > > +static void rt_runtime_us_einval(void) > > +{ > > + TST_EXP_FAIL(write(runtime_fd, "-2", 2), EINVAL, > > + "echo -2 > "RT_RUNTIME_US); > > +} > > I would happily add my tags to the test except that as a general > principal, I don't want to deal with tests that fail if an unexpected > error number is returned. > > Unless something can be done (e.g. with meta-data), so that TST_EXP_FAIL > and similar can be reduced to a TCONF when the wrong errno is returned. > > For instance, this would allow running the tests with a seccomp BPF > profile, LSM, CGroup or /proc bind mount that blocks the write. > > Or you could just seperate the tests I guess. Then the reset can be > checked while skipping the errno checks. Hmm, so I guess that with LSM we are able to open these files R/W but we can stil get EPERM from the write() right? I'm reluctant to add wildcard TCONF on any errno, but I guess that we can add a TST_EXP_FAIL macro version that would have one errno for PASS and one errno for TCONF.
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> > +static void rt_runtime_us_einval(void) >> > +{ >> > + TST_EXP_FAIL(write(runtime_fd, "-2", 2), EINVAL, >> > + "echo -2 > "RT_RUNTIME_US); >> > +} >> >> I would happily add my tags to the test except that as a general >> principal, I don't want to deal with tests that fail if an unexpected >> error number is returned. >> >> Unless something can be done (e.g. with meta-data), so that TST_EXP_FAIL >> and similar can be reduced to a TCONF when the wrong errno is returned. >> >> For instance, this would allow running the tests with a seccomp BPF >> profile, LSM, CGroup or /proc bind mount that blocks the write. >> >> Or you could just seperate the tests I guess. Then the reset can be >> checked while skipping the errno checks. > > Hmm, so I guess that with LSM we are able to open these files R/W but we > can stil get EPERM from the write() right? Sorry, I thought there was an LSM hook in read, but AFAICT there is not. It's possible that another hook get's called (e.g. security_inode_getattr), but it's not immediately clear to me. This still leaves seccomp bpf, FUSE, kernel module, etc. whatever is mounted at /proc doesn't have to be the original file system. Container runtimes do something with /proc to filter it. This situation seems to get more complicated each year. > I'm reluctant to add wildcard > TCONF on any errno, but I guess that we can add a TST_EXP_FAIL macro > version that would have one errno for PASS and one errno for TCONF. I'm leaning increasingly towards partitioning the test, so that bits can be filtered out somehow. Then people can focus on the bits they really care about.
Hi! > Hmm, so I guess that with LSM we are able to open these files R/W but we > can stil get EPERM from the write() right? I'm reluctant to add wildcard > TCONF on any errno, but I guess that we can add a TST_EXP_FAIL macro > version that would have one errno for PASS and one errno for TCONF. I was thinking about this yesterday and maybe best solution would be to introduce a global switch (env variable) that would switch the TST_EXP_FAIL() macros to a more forgiving mode so that EINVAL and EPERM and possibly a few more errnos would be converted into TCONF automatically. What do you think?
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> Hmm, so I guess that with LSM we are able to open these files R/W but we >> can stil get EPERM from the write() right? I'm reluctant to add wildcard >> TCONF on any errno, but I guess that we can add a TST_EXP_FAIL macro >> version that would have one errno for PASS and one errno for TCONF. > > I was thinking about this yesterday and maybe best solution would be to > introduce a global switch (env variable) that would switch the > TST_EXP_FAIL() macros to a more forgiving mode so that EINVAL and EPERM > and possibly a few more errnos would be converted into TCONF > automatically. What do you think? It sounds reasonable, I think it would also make sense to have an option to allow any value in the full errno range.
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..b30256792 --- /dev/null +++ b/testcases/kernel/sched/sysctl/proc_sched_rt01.c @@ -0,0 +1,115 @@ +// 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 commit: + * + * 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 + */ + +#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"}, + {} + }, + .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> --- runtest/sched | 2 + testcases/kernel/sched/sysctl/.gitignore | 1 + testcases/kernel/sched/sysctl/Makefile | 7 ++ .../kernel/sched/sysctl/proc_sched_rt01.c | 115 ++++++++++++++++++ 4 files changed, 125 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