[PATCH] sysctl: unregister sysctl table after testing

[PATCH] sysctl: unregister sysctl table after testing

[John Sperbeck]

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

If we unregister the test sysctl table, then the failure is gone.

Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array")
Signed-off-by: John Sperbeck <jsperbeck@google.com>
---
 kernel/sysctl-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
index 3ac98bb7fb82..2184c1813b1d 100644
--- a/kernel/sysctl-test.c
+++ b/kernel/sysctl-test.c
@@ -373,6 +373,7 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
 static void sysctl_test_register_sysctl_sz_invalid_extra_value(
 		struct kunit *test)
 {
+	struct ctl_table_header *hdr;
 	unsigned char data = 0;
 	struct ctl_table table_foo[] = {
 		{
@@ -412,7 +413,9 @@ 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));
+	hdr = register_sysctl("foo", table_qux);
+	KUNIT_EXPECT_NOT_NULL(test, hdr);
+	unregister_sysctl_table(hdr);
 }
 
 static struct kunit_case sysctl_test_cases[] = {
-- 
2.47.1.613.gc27f4b7a9f-goog

Re: [PATCH] sysctl: unregister sysctl table after testing

[Wen Yang]

Good catch, thanks.

Reviewed-by: Wen Yang <wen.yang@linux.dev>


--
Best wishes,
Wen


On 2024/12/25 01:11, 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
> 
> If we unregister the test sysctl table, then the failure is gone.
> 
> Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array")
> Signed-off-by: John Sperbeck <jsperbeck@google.com>
> ---
>   kernel/sysctl-test.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> index 3ac98bb7fb82..2184c1813b1d 100644
> --- a/kernel/sysctl-test.c
> +++ b/kernel/sysctl-test.c
> @@ -373,6 +373,7 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
>   static void sysctl_test_register_sysctl_sz_invalid_extra_value(
>   		struct kunit *test)
>   {
> +	struct ctl_table_header *hdr;
>   	unsigned char data = 0;
>   	struct ctl_table table_foo[] = {
>   		{
> @@ -412,7 +413,9 @@ 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));
> +	hdr = register_sysctl("foo", table_qux);
> +	KUNIT_EXPECT_NOT_NULL(test, hdr);
> +	unregister_sysctl_table(hdr);
>   }
>   
>   static struct kunit_case sysctl_test_cases[] = {

Re: [PATCH] sysctl: unregister sysctl table after testing

[Joel Granados]

On Tue, Dec 24, 2024 at 09:11:24AM -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.

Very good catch indeed!!!.
> 
> 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
> 
> If we unregister the test sysctl table, then the failure is gone.
> 
> Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array")
> Signed-off-by: John Sperbeck <jsperbeck@google.com>
> ---
>  kernel/sysctl-test.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> index 3ac98bb7fb82..2184c1813b1d 100644
> --- a/kernel/sysctl-test.c
> +++ b/kernel/sysctl-test.c
> @@ -373,6 +373,7 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
>  static void sysctl_test_register_sysctl_sz_invalid_extra_value(
>  		struct kunit *test)
>  {
> +	struct ctl_table_header *hdr;
>  	unsigned char data = 0;
>  	struct ctl_table table_foo[] = {
>  		{
> @@ -412,7 +413,9 @@ 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));
> +	hdr = register_sysctl("foo", table_qux);
> +	KUNIT_EXPECT_NOT_NULL(test, hdr);
> +	unregister_sysctl_table(hdr);
This indeed fixes the behaviour, but it is not what should be done
and this is why:
1. sysctl-test.c is part of the unit tests for sysctl and actually
   trying to execute a register here does not really make sense.
2. The file that actually does the regression testing is
   lib/test_sysctl.c

If you are up for it this is what needs to be done:
1. change what is in sysctl-test.c to call sysctl_check_table_array
   directly and not worry about keeping track of the registration.
2. Add a similar regression test in lib/test_sysctl.c where we actually
   check for the error.

Please tell me if you are up for it (if not I can add it to my todos)

Best

>  }
>  
>  static struct kunit_case sysctl_test_cases[] = {
> -- 
> 2.47.1.613.gc27f4b7a9f-goog
> 

-- 

Joel Granados

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

[John Sperbeck]

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(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 27a283d85a6e..1de946176d74 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;
 }
 
+#ifdef 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..0f1d3a626f4f 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;
 
+#ifdef 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[] = {
-- 
2.47.1.613.gc27f4b7a9f-goog

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

[kernel test robot]

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 v6.13-rc7 next-20250110]
[cannot apply to kees/for-next/pstore sysctl/sysctl-next]
[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-055310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/kspp
patch link:    https://lore.kernel.org/r/20250112215013.2386009-1-jsperbeck%40google.com
patch subject: [PATCH v2] sysctl: expose sysctl_check_table for unit testing and use it
config: csky-randconfig-002-20250113 (https://download.01.org/0day-ci/archive/20250113/202501131354.JbiHtAEH-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250113/202501131354.JbiHtAEH-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/202501131354.JbiHtAEH-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/sysctl-test.c:6:
   kernel/sysctl-test.c: In function 'sysctl_test_register_sysctl_sz_invalid_extra_value':
>> kernel/sysctl-test.c:414:25: error: implicit declaration of function 'sysctl_check_table_test_helper' [-Wimplicit-function-declaration]
     414 |                         sysctl_check_table_test_helper("foo", table_foo));
         |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/kunit/test.h:774:22: note: in definition of macro 'KUNIT_BASE_BINARY_ASSERTION'
     774 |         const typeof(right) __right = (right);                                 \
         |                      ^~~~~
   include/kunit/test.h:969:9: note: in expansion of macro 'KUNIT_BINARY_INT_ASSERTION'
     969 |         KUNIT_BINARY_INT_ASSERTION(test,                                       \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   include/kunit/test.h:966:9: note: in expansion of macro 'KUNIT_EXPECT_EQ_MSG'
     966 |         KUNIT_EXPECT_EQ_MSG(test, left, right, NULL)
         |         ^~~~~~~~~~~~~~~~~~~
   kernel/sysctl-test.c:413:9: note: in expansion of macro 'KUNIT_EXPECT_EQ'
     413 |         KUNIT_EXPECT_EQ(test, -EINVAL,
         |         ^~~~~~~~~~~~~~~


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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[John Sperbeck]

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(-)

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[] = {
-- 
2.47.1.613.gc27f4b7a9f-goog

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

[Joel Granados]

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

-- 

Joel Granados

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

[Joel Granados]

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
> 

-- 

Joel Granados

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[John Sperbeck]

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

[Joel Granados]

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

Re: [PATCH] sysctl: unregister sysctl table after testing

[Joel Granados]

On Mon, Jan 06, 2025 at 03:15:13PM +0100, Joel Granados wrote:
> 
> On Tue, Dec 24, 2024 at 09:11:24AM -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.
> 
> Very good catch indeed!!!.
> > 
...
> >  	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));
> > +	hdr = register_sysctl("foo", table_qux);
> > +	KUNIT_EXPECT_NOT_NULL(test, hdr);
> > +	unregister_sysctl_table(hdr);
> This indeed fixes the behaviour, but it is not what should be done
> and this is why:
> 1. sysctl-test.c is part of the unit tests for sysctl and actually
>    trying to execute a register here does not really make sense.
> 2. The file that actually does the regression testing is
>    lib/test_sysctl.c
> 
> If you are up for it this is what needs to be done:
> 1. change what is in sysctl-test.c to call sysctl_check_table_array
>    directly and not worry about keeping track of the registration.

With your V4 it is clear to me know that I should *not* have made the
first suggestion of calling sysctl_check_table. Exposing the
sysctl_check_table just for the kunit test is overkill as we can get the
same result with a register call from lib/tests_sysctl.c without all the
hassle of exposing the function call. Your proposal was still valuable
as it clarified what the "right" approach should be.

Best
> 2. Add a similar regression test in lib/test_sysctl.c where we actually
>    check for the error.
> 
> Please tell me if you are up for it (if not I can add it to my todos)
> 
> Best
> 
> >  }
> >  
> >  static struct kunit_case sysctl_test_cases[] = {
> > -- 
> > 2.47.1.613.gc27f4b7a9f-goog
> > 
> 
> -- 
> 
> Joel Granados

-- 

Joel Granados