| Message ID | 20250113070001.143690-1-jsperbeck@google.com |
|---|---|
| State | Superseded |
| Delegated to: | Joel Granados |
| Headers | show |
| Series | [v3] sysctl: expose sysctl_check_table for unit testing and use it | expand |
On Sun, Jan 12, 2025 at 11:00:01PM -0800, John Sperbeck wrote: > In commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check > of u8 to sysctl_check_table_array"), a kunit test was added that > registers a sysctl table. If the test is run as a module, then a > lingering reference to the module is left behind, and a 'sysctl -a' > leads to a panic. > > This can be reproduced with these kernel config settings: > > CONFIG_KUNIT=y > CONFIG_SYSCTL_KUNIT_TEST=m > > Then run these commands: > > modprobe sysctl-test > rmmod sysctl-test > sysctl -a > > The panic varies but generally looks something like this: > > BUG: unable to handle page fault for address: ffffa4571c0c7db4 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 100000067 P4D 100000067 PUD 100351067 PMD 114f5e067 PTE 0 > Oops: Oops: 0000 [#1] SMP NOPTI > ... ... ... > RIP: 0010:proc_sys_readdir+0x166/0x2c0 > ... ... ... > Call Trace: > <TASK> > iterate_dir+0x6e/0x140 > __se_sys_getdents+0x6e/0x100 > do_syscall_64+0x70/0x150 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > Instead of fully registering a sysctl table, expose the underlying > checking function and use it in the unit test. thx for taking the time to change it. I have added it to my backlog and will get to it at some point. It is not pressing as this touches sysctl testing, which should not be used in the wild. I know why you created the V3, but please add a comment on the mail (not the commit) of what changed in every version. Best
On Sun, Jan 12, 2025 at 11:00:01PM -0800, John Sperbeck wrote: > In commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check > of u8 to sysctl_check_table_array"), a kunit test was added that > registers a sysctl table. If the test is run as a module, then a > lingering reference to the module is left behind, and a 'sysctl -a' > leads to a panic. > > This can be reproduced with these kernel config settings: > > CONFIG_KUNIT=y > CONFIG_SYSCTL_KUNIT_TEST=m > > Then run these commands: > > modprobe sysctl-test > rmmod sysctl-test > sysctl -a > > The panic varies but generally looks something like this: > > BUG: unable to handle page fault for address: ffffa4571c0c7db4 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 100000067 P4D 100000067 PUD 100351067 PMD 114f5e067 PTE 0 > Oops: Oops: 0000 [#1] SMP NOPTI > ... ... ... > RIP: 0010:proc_sys_readdir+0x166/0x2c0 > ... ... ... > Call Trace: > <TASK> > iterate_dir+0x6e/0x140 > __se_sys_getdents+0x6e/0x100 > do_syscall_64+0x70/0x150 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > Instead of fully registering a sysctl table, expose the underlying > checking function and use it in the unit test. > > Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array") > Signed-off-by: John Sperbeck <jsperbeck@google.com> You have received a error from the bot. please address it to move forward. Best > --- > fs/proc/proc_sysctl.c | 22 +++++++++++++++++----- > include/linux/sysctl.h | 8 ++++++++ > kernel/sysctl-test.c | 9 ++++++--- > 3 files changed, 31 insertions(+), 8 deletions(-) > ... > > static struct kunit_case sysctl_test_cases[] = { > -- > 2.47.1.613.gc27f4b7a9f-goog >
Hi John, kernel test robot noticed the following build errors: [auto build test ERROR on kees/for-next/kspp] [also build test ERROR on linus/master sysctl/sysctl-next v6.13-rc7] [cannot apply to kees/for-next/pstore] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/John-Sperbeck/sysctl-expose-sysctl_check_table-for-unit-testing-and-use-it/20250113-150214 base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/kspp patch link: https://lore.kernel.org/r/20250113070001.143690-1-jsperbeck%40google.com patch subject: [PATCH v3] sysctl: expose sysctl_check_table for unit testing and use it config: um-randconfig-002-20250118 (https://download.01.org/0day-ci/archive/20250118/202501182003.Gfi63jzH-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250118/202501182003.Gfi63jzH-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202501182003.Gfi63jzH-lkp@intel.com/ All errors (new ones prefixed by >>): >> kernel/sysctl-test.c:414:4: error: call to undeclared function 'sysctl_check_table_test_helper'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration] sysctl_check_table_test_helper("foo", table_foo)); ^ kernel/sysctl-test.c:416:4: error: call to undeclared function 'sysctl_check_table_test_helper'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration] sysctl_check_table_test_helper("foo", table_bar)); ^ kernel/sysctl-test.c:418:4: error: call to undeclared function 'sysctl_check_table_test_helper'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration] sysctl_check_table_test_helper("foo", table_qux)); ^ 3 errors generated. vim +/sysctl_check_table_test_helper +414 kernel/sysctl-test.c 369 370 /* 371 * Test that registering an invalid extra value is not allowed. 372 */ 373 static void sysctl_test_register_sysctl_sz_invalid_extra_value( 374 struct kunit *test) 375 { 376 unsigned char data = 0; 377 struct ctl_table table_foo[] = { 378 { 379 .procname = "foo", 380 .data = &data, 381 .maxlen = sizeof(u8), 382 .mode = 0644, 383 .proc_handler = proc_dou8vec_minmax, 384 .extra1 = SYSCTL_FOUR, 385 .extra2 = SYSCTL_ONE_THOUSAND, 386 }, 387 }; 388 389 struct ctl_table table_bar[] = { 390 { 391 .procname = "bar", 392 .data = &data, 393 .maxlen = sizeof(u8), 394 .mode = 0644, 395 .proc_handler = proc_dou8vec_minmax, 396 .extra1 = SYSCTL_NEG_ONE, 397 .extra2 = SYSCTL_ONE_HUNDRED, 398 }, 399 }; 400 401 struct ctl_table table_qux[] = { 402 { 403 .procname = "qux", 404 .data = &data, 405 .maxlen = sizeof(u8), 406 .mode = 0644, 407 .proc_handler = proc_dou8vec_minmax, 408 .extra1 = SYSCTL_ZERO, 409 .extra2 = SYSCTL_TWO_HUNDRED, 410 }, 411 }; 412 413 KUNIT_EXPECT_EQ(test, -EINVAL, > 414 sysctl_check_table_test_helper("foo", table_foo)); 415 KUNIT_EXPECT_EQ(test, -EINVAL, 416 sysctl_check_table_test_helper("foo", table_bar)); 417 KUNIT_EXPECT_EQ(test, 0, 418 sysctl_check_table_test_helper("foo", table_qux)); 419 } 420
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 27a283d85a6e..2d3272826cc2 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1137,11 +1137,12 @@ static int sysctl_check_table_array(const char *path, const struct ctl_table *ta return err; } -static int sysctl_check_table(const char *path, struct ctl_table_header *header) +static int sysctl_check_table(const char *path, const struct ctl_table *table, + size_t table_size) { - const struct ctl_table *entry; + const struct ctl_table *entry = table; int err = 0; - list_for_each_table_entry(entry, header) { + for (size_t i = 0 ; i < table_size; ++i, entry++) { if (!entry->procname) err |= sysctl_err(path, entry, "procname is null"); if ((entry->proc_handler == proc_dostring) || @@ -1173,6 +1174,16 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header) return err; } +#if IS_ENABLED(CONFIG_KUNIT) +int sysctl_check_table_test_helper_sz(const char *path, + const struct ctl_table *table, + size_t table_size) +{ + return sysctl_check_table(path, table, table_size); +} +EXPORT_SYMBOL(sysctl_check_table_test_helper_sz); +#endif /* CONFIG_KUNIT */ + static struct ctl_table_header *new_links(struct ctl_dir *dir, struct ctl_table_header *head) { struct ctl_table *link_table, *link; @@ -1372,6 +1383,9 @@ struct ctl_table_header *__register_sysctl_table( struct ctl_dir *dir; struct ctl_node *node; + if (sysctl_check_table(path, table, table_size)) + return NULL; + header = kzalloc(sizeof(struct ctl_table_header) + sizeof(struct ctl_node)*table_size, GFP_KERNEL_ACCOUNT); if (!header) @@ -1379,8 +1393,6 @@ struct ctl_table_header *__register_sysctl_table( node = (struct ctl_node *)(header + 1); init_header(header, root, set, node, table, table_size); - if (sysctl_check_table(path, header)) - goto fail; spin_lock(&sysctl_lock); dir = &set->dir; diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index 40a6ac6c9713..09caac302333 100644 --- a/include/linux/sysctl.h +++ b/include/linux/sysctl.h @@ -247,6 +247,14 @@ extern int unaligned_enabled; extern int unaligned_dump_stack; extern int no_unaligned_warning; +#if IS_ENABLED(CONFIG_KUNIT) +int sysctl_check_table_test_helper_sz(const char *path, + const struct ctl_table *table, + size_t table_size); +#define sysctl_check_table_test_helper(path, table) \ + sysctl_check_table_test_helper_sz(path, table, ARRAY_SIZE(table)) +#endif /* CONFIG_KUNIT */ + #else /* CONFIG_SYSCTL */ static inline void register_sysctl_init(const char *path, const struct ctl_table *table) diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c index 3ac98bb7fb82..247dd8536fc7 100644 --- a/kernel/sysctl-test.c +++ b/kernel/sysctl-test.c @@ -410,9 +410,12 @@ static void sysctl_test_register_sysctl_sz_invalid_extra_value( }, }; - KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_foo)); - KUNIT_EXPECT_NULL(test, register_sysctl("foo", table_bar)); - KUNIT_EXPECT_NOT_NULL(test, register_sysctl("foo", table_qux)); + KUNIT_EXPECT_EQ(test, -EINVAL, + sysctl_check_table_test_helper("foo", table_foo)); + KUNIT_EXPECT_EQ(test, -EINVAL, + sysctl_check_table_test_helper("foo", table_bar)); + KUNIT_EXPECT_EQ(test, 0, + sysctl_check_table_test_helper("foo", table_qux)); } static struct kunit_case sysctl_test_cases[] = {
In commit b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array"), a kunit test was added that registers a sysctl table. If the test is run as a module, then a lingering reference to the module is left behind, and a 'sysctl -a' leads to a panic. This can be reproduced with these kernel config settings: CONFIG_KUNIT=y CONFIG_SYSCTL_KUNIT_TEST=m Then run these commands: modprobe sysctl-test rmmod sysctl-test sysctl -a The panic varies but generally looks something like this: BUG: unable to handle page fault for address: ffffa4571c0c7db4 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 100000067 P4D 100000067 PUD 100351067 PMD 114f5e067 PTE 0 Oops: Oops: 0000 [#1] SMP NOPTI ... ... ... RIP: 0010:proc_sys_readdir+0x166/0x2c0 ... ... ... Call Trace: <TASK> iterate_dir+0x6e/0x140 __se_sys_getdents+0x6e/0x100 do_syscall_64+0x70/0x150 entry_SYSCALL_64_after_hwframe+0x76/0x7e Instead of fully registering a sysctl table, expose the underlying checking function and use it in the unit test. Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array") Signed-off-by: John Sperbeck <jsperbeck@google.com> --- fs/proc/proc_sysctl.c | 22 +++++++++++++++++----- include/linux/sysctl.h | 8 ++++++++ kernel/sysctl-test.c | 9 ++++++--- 3 files changed, 31 insertions(+), 8 deletions(-)