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

[Wen Yang <wen.yang@linux.dev>]

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[] = {