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
