One of the sysctl tests registers a valid sysctl table. This operation
is expected to succeed. However, it does not unregister the table after
executing the test. If the code is built as module and the module is
unloaded after the test, the next operation trying to access the table
(such as 'sysctl -a') will trigger a crash.

Unregister the registered table after test completiion to solve the
problem.

Fixes: b5ffbd139688 ("sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array")
Cc: Wen Yang <wen.yang@linux.dev>
Cc: Joel Granados <joel.granados@kernel.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 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 eb2842bd0557..ac84f64dbdeb 100644
--- a/kernel/sysctl-test.c
+++ b/kernel/sysctl-test.c
@@ -374,6 +374,7 @@ static void sysctl_test_register_sysctl_sz_invalid_extra_value(
 		struct kunit *test)
 {
 	unsigned char data = 0;
+	struct ctl_table_header *header;
 	const struct ctl_table table_foo[] = {
 		{
 			.procname	= "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));
+	header = register_sysctl("foo", table_qux);
+	KUNIT_EXPECT_NOT_NULL(test, header);
+	unregister_sysctl_table(header);
 }
 
 static struct kunit_case sysctl_test_cases[] = {
-- 
2.45.2

On Wed, May 21, 2025 at 06:32:11PM -0700, Guenter Roeck wrote:
> One of the sysctl tests registers a valid sysctl table. This operation
> is expected to succeed. However, it does not unregister the table after
> executing the test. If the code is built as module and the module is
> unloaded after the test, the next operation trying to access the table
> (such as 'sysctl -a') will trigger a crash.
> 
> Unregister the registered table after test completiion to solve the
> problem.
> 

Never mind, I just learned that a very similar patch has been submitted
last December or so but was rejected, and that the acceptable (?) fix seems
to be stalled.

Sorry for the noise.

Guenter

On Thu, May 22, 2025 at 11:53:15AM -0700, Guenter Roeck wrote:
> On Wed, May 21, 2025 at 06:32:11PM -0700, Guenter Roeck wrote:
> > One of the sysctl tests registers a valid sysctl table. This operation
> > is expected to succeed. However, it does not unregister the table after
> > executing the test. If the code is built as module and the module is
> > unloaded after the test, the next operation trying to access the table
> > (such as 'sysctl -a') will trigger a crash.
> > 
> > Unregister the registered table after test completiion to solve the
> > problem.
> > 
> 
> Never mind, I just learned that a very similar patch has been submitted
> last December or so but was rejected, and that the acceptable (?) fix seems
> to be stalled.
> 
> Sorry for the noise.
> 
> Guenter

Hey Guenter

It is part of what is getting sent for 6.16 [1]
That test will move out of kunit into self-test.

Best


[1] https://sysctl-dev-rtd.readthedocs.io/en/latest/release.html#patch-0-4-sysctl-move-the-u8-range-check-test-to-lib-test-sysctl-c

-- 

Joel Granados

On 5/23/25 08:01, Joel Granados wrote:
> On Thu, May 22, 2025 at 11:53:15AM -0700, Guenter Roeck wrote:
>> On Wed, May 21, 2025 at 06:32:11PM -0700, Guenter Roeck wrote:
>>> One of the sysctl tests registers a valid sysctl table. This operation
>>> is expected to succeed. However, it does not unregister the table after
>>> executing the test. If the code is built as module and the module is
>>> unloaded after the test, the next operation trying to access the table
>>> (such as 'sysctl -a') will trigger a crash.
>>>
>>> Unregister the registered table after test completiion to solve the
>>> problem.
>>>
>>
>> Never mind, I just learned that a very similar patch has been submitted
>> last December or so but was rejected, and that the acceptable (?) fix seems
>> to be stalled.
>>
>> Sorry for the noise.
>>
>> Guenter
> 
> Hey Guenter
> 
> It is part of what is getting sent for 6.16 [1]
> That test will move out of kunit into self-test.
> 

Yes, I was pointed to that. The version I have seen seems to assume that
the test is running as module, because the created sysctl entry is removed
in the module exit function. If built into the kernel, it would leave
the debug entry in place after the test is complete. Also, it moves
the affected set of tests out of the kunit infrastructure. Is that accurate
or a misunderstanding on my side ?

No criticism or objection, I am just trying to understand the direction
of unit testing and specifically of the kunit infrastructure going forward.

Thanks,
Guenter

On Fri, May 23, 2025 at 08:54:44AM -0700, Guenter Roeck wrote:
> On 5/23/25 08:01, Joel Granados wrote:
> > On Thu, May 22, 2025 at 11:53:15AM -0700, Guenter Roeck wrote:
> > > On Wed, May 21, 2025 at 06:32:11PM -0700, Guenter Roeck wrote:
> > > > One of the sysctl tests registers a valid sysctl table. This operation
> > > > is expected to succeed. However, it does not unregister the table after
> > > > executing the test. If the code is built as module and the module is
> > > > unloaded after the test, the next operation trying to access the table
> > > > (such as 'sysctl -a') will trigger a crash.
> > > > 
> > > > Unregister the registered table after test completiion to solve the
> > > > problem.
> > > > 
> > > 
> > > Never mind, I just learned that a very similar patch has been submitted
> > > last December or so but was rejected, and that the acceptable (?) fix seems
> > > to be stalled.
> > > 
> > > Sorry for the noise.
> > > 
> > > Guenter
> > 
> > Hey Guenter
> > 
> > It is part of what is getting sent for 6.16 [1]
> > That test will move out of kunit into self-test.
> > 
> 
> Yes, I was pointed to that. The version I have seen seems to assume that
> the test is running as module, because the created sysctl entry is removed
> in the module exit function. If built into the kernel, it would leave
> the debug entry in place after the test is complete. Also, it moves
> the affected set of tests out of the kunit infrastructure. Is that accurate
> or a misunderstanding on my side ?
You have understood correctly. That is what the sysctl selftest does at least.
It all runs together with tools/testing/sefltests/sysctl/*. The idea is
to use CONFIG_TEST_SYSCTL only for testing purposes.

Best

-- 

Joel Granados