1. Patchew
  2. linux
  3. View series

[PATCH v4] sysctl: expose sysctl_check_table for unit testing and use it

John Sperbeck posted 1 patch 1 year ago 
fs/proc/proc_sysctl.c  | 22 +++++++++++++++++-----
include/linux/sysctl.h | 17 +++++++++++++++++
kernel/sysctl-test.c   |  9 ++++++---
3 files changed, 40 insertions(+), 8 deletions(-)
[PATCH v4] sysctl: expose sysctl_check_table for unit testing and use it
Posted by John Sperbeck 1 year ago 
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>
---

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++) {
 		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));
 }
 
 static struct kunit_case sysctl_test_cases[] = {
-- 
2.48.0.rc2.279.g1de40edade-goog
Re: [PATCH v4] sysctl: expose sysctl_check_table for unit testing and use it
Posted by Joel Granados 11 months ago 
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@google.com>
> ---
> 
> 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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.