On Tue, Jan 21, 2025 at 01:33:53PM -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@xxxxxxxxxx> > --- > > The Change from v3 to v4 is to make sure sysctl_check_table_test_helper_sz() > is defined in the unusual case that the sysctl kunit test is enabled, but > CONFIG_SYSCTL is disabled. > > fs/proc/proc_sysctl.c | 22 +++++++++++++++++----- > include/linux/sysctl.h | 17 +++++++++++++++++ > kernel/sysctl-test.c | 9 ++++++--- > 3 files changed, 40 insertions(+), 8 deletions(-) > > 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++) { This should be avoided as the traversal of the ctl_table should be handled in one place (the list_for_each_table_entry macro) > 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..02acd3670bd2 100644 > --- a/include/linux/sysctl.h > +++ b/include/linux/sysctl.h > @@ -288,4 +288,21 @@ static inline bool sysctl_is_alias(char *param) > int sysctl_max_threads(const struct ctl_table *table, int write, void *buffer, > size_t *lenp, loff_t *ppos); > > +#if IS_ENABLED(CONFIG_KUNIT) > +#define sysctl_check_table_test_helper(path, table) \ > + sysctl_check_table_test_helper_sz(path, table, ARRAY_SIZE(table)) > +#ifdef CONFIG_SYSCTL > +int sysctl_check_table_test_helper_sz(const char *path, > + const struct ctl_table *table, > + size_t table_size); > +#else /* CONFIG_SYSCTL */ > +static inline int sysctl_check_table_test_helper_sz(const char *path, > + const struct ctl_table *table, > + size_t table_size) > +{ > + return 0; > +} > +#endif /* CONFIG_SYSCTL */ > +#endif /* CONFIG_KUNIT */ > + > #endif /* _LINUX_SYSCTL_H */ > 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)); This should all be in lib/tests_sysctl.c. We should remove all this function from the kunit and add an equivalent one to lib/tests_sysctl.c Best > } > > static struct kunit_case sysctl_test_cases[] = { > -- > 2.48.0.rc2.279.g1de40edade-goog > -- Joel Granados