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@xxxxxxxxxx>
> ---
> 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
