[PATCH 0/8] Remove child from struct ctl_table

[PATCH 0/8] Remove child from struct ctl_table

[Joel Granados]

This is part of the effort to remove the empty element of the ctl_table
structures (used to calculate size) and replace it with an ARRAY_SIZE call. By
replacing the child element in struct ctl_table with a flags element we make
sure that there are no forward recursions on child nodes and therefore set
ourselves up for just using an ARRAY_SIZE. We also added some self tests to
make sure that we do not break anything.

Patchset is separated in 4: parport fixes, selftests fixes, selftests additions and
replacement of child element. Tested everything with sysctl self tests and everything
seems "ok".

1. parport fixes: @mcgrof: this is related to my previous series and it plugs a
   sysct table leak in the parport driver. Please tell me if you want me to repost
   the parport series with this one stiched in.

2. Selftests fixes: Remove the prefixed zeros when passing a awk field to the
   awk print command because it was causing $0009 to be interpreted as $0.
   Replaced continue with return in sysctl.sh(test_case) so the test actually
   gets skipped. The skip decision is now in sysctl.sh(skip_test).

3. Selftest additions: New test to confirm that unregister actually removes
   targets. New test to confirm that permanently empty targets are indeed
   created and that no other targets can be created "on top".

4. Replaced the child pointer in struct ctl_table with a u8 flag. The flag
   is used to differentiate between permanently empty targets and non-empty ones.

Comments/feedback greatly appreciated

Best
Joel

Joel Granados (8):
  parport: plug a sysctl register leak
  test_sysctl: Fix test metadata getters
  test_sysctl: Group node sysctl test under one func
  test_sysctl: Add an unregister sysctl test
  test_sysctl: Add an option to prevent test skip
  test_sysclt: Test for registering a mount point
  sysctl: Remove debugging dump_stack
  sysctl: replace child with a flags var

 drivers/parport/procfs.c                 |  23 ++---
 fs/proc/proc_sysctl.c                    |  82 ++++------------
 include/linux/sysctl.h                   |   4 +-
 lib/test_sysctl.c                        |  91 ++++++++++++++++--
 tools/testing/selftests/sysctl/sysctl.sh | 115 +++++++++++++++++------
 5 files changed, 204 insertions(+), 111 deletions(-)

-- 
2.30.2

[PATCH 1/8] parport: plug a sysctl register leak

[Joel Granados]

parport registers two sysctl directories in the parport_proc_register
function but only one of them was getting unregistered in
parport_proc_unregister. Keep track of both sysctl table headers and
handle them together when (un)registering.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/parport/procfs.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index cbb1fb5127ce..0f2d2e1ee28e 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -257,14 +257,16 @@ PARPORT_MAX_SPINTIME_VALUE;
 
 
 struct parport_sysctl_table {
-	struct ctl_table_header *sysctl_header;
+	struct ctl_table_header *port_header;
+	struct ctl_table_header *devices_header;
 	struct ctl_table vars[12];
 	struct ctl_table device_dir[2];
 };
 
 static const struct parport_sysctl_table parport_sysctl_template = {
-	.sysctl_header = NULL,
-        {
+	.port_header = NULL,
+	.devices_header = NULL,
+	{
 		{
 			.procname	= "spintime",
 			.data		= NULL,
@@ -429,7 +431,6 @@ parport_default_sysctl_table = {
 int parport_proc_register(struct parport *port)
 {
 	struct parport_sysctl_table *t;
-	struct ctl_table_header *devices_h;
 	char *tmp_dir_path;
 	size_t tmp_path_len, port_name_len;
 	int bytes_written, i, err = 0;
@@ -464,8 +465,8 @@ int parport_proc_register(struct parport *port)
 		err = -ENOENT;
 		goto exit_free_tmp_dir_path;
 	}
-	devices_h = register_sysctl(tmp_dir_path, t->device_dir);
-	if (devices_h == NULL) {
+	t->devices_header = register_sysctl(tmp_dir_path, t->device_dir);
+	if (t->devices_header == NULL) {
 		err = -ENOENT;
 		goto  exit_free_tmp_dir_path;
 	}
@@ -478,8 +479,8 @@ int parport_proc_register(struct parport *port)
 		goto unregister_devices_h;
 	}
 
-	t->sysctl_header = register_sysctl(tmp_dir_path, t->vars);
-	if (t->sysctl_header == NULL) {
+	t->port_header = register_sysctl(tmp_dir_path, t->vars);
+	if (t->port_header == NULL) {
 		err = -ENOENT;
 		goto unregister_devices_h;
 	}
@@ -490,7 +491,7 @@ int parport_proc_register(struct parport *port)
 	return 0;
 
 unregister_devices_h:
-	unregister_sysctl_table(devices_h);
+	unregister_sysctl_table(t->devices_header);
 
 exit_free_tmp_dir_path:
 	kfree(tmp_dir_path);
@@ -505,7 +506,8 @@ int parport_proc_unregister(struct parport *port)
 	if (port->sysctl_table) {
 		struct parport_sysctl_table *t = port->sysctl_table;
 		port->sysctl_table = NULL;
-		unregister_sysctl_table(t->sysctl_header);
+		unregister_sysctl_table(t->devices_header);
+		unregister_sysctl_table(t->port_header);
 		kfree(t);
 	}
 	return 0;
-- 
2.30.2

[PATCH 2/8] test_sysctl: Fix test metadata getters

[Joel Granados]

The functions get_test_{count,enabled,target} use awk to get the N'th
field in the ALL_TESTS variable. A variable with leading zeros (e.g.
0009) is misinterpreted as an entire line instead of the N'th field.
Remove the leading zeros so this does not happen. We can now use the
helper in tests 6, 7 and 8.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 tools/testing/selftests/sysctl/sysctl.sh | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index bfc54b422f25..cb8f83dfe16b 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -730,7 +730,7 @@ sysctl_test_0005()
 
 sysctl_test_0006()
 {
-	TARGET="${SYSCTL}/bitmap_0001"
+	TARGET="${SYSCTL}/$(get_test_target 0006)"
 	reset_vals
 	ORIG=""
 	run_bitmaptest
@@ -738,7 +738,7 @@ sysctl_test_0006()
 
 sysctl_test_0007()
 {
-	TARGET="${SYSCTL}/boot_int"
+	TARGET="${SYSCTL}/$(get_test_target 0007)"
 	if [ ! -f $TARGET ]; then
 		echo "Skipping test for $TARGET as it is not present ..."
 		return $ksft_skip
@@ -778,7 +778,7 @@ sysctl_test_0007()
 
 sysctl_test_0008()
 {
-	TARGET="${SYSCTL}/match_int"
+	TARGET="${SYSCTL}/$(get_test_target 0008)"
 	if [ ! -f $TARGET ]; then
 		echo "Skipping test for $TARGET as it is not present ..."
 		return $ksft_skip
@@ -857,25 +857,32 @@ function test_num()
 		usage
 	fi
 }
+function remove_leading_zeros()
+{
+	echo $1 | sed 's/^0*//'
+}
 
 function get_test_count()
 {
 	test_num $1
-	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+	awk_field=$(remove_leading_zeros $1)
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$awk_field'}')
 	echo ${TEST_DATA} | awk -F":" '{print $2}'
 }
 
 function get_test_enabled()
 {
 	test_num $1
-	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+	awk_field=$(remove_leading_zeros $1)
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$awk_field'}')
 	echo ${TEST_DATA} | awk -F":" '{print $3}'
 }
 
 function get_test_target()
 {
 	test_num $1
-	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$1'}')
+	awk_field=$(remove_leading_zeros $1)
+	TEST_DATA=$(echo $ALL_TESTS | awk '{print $'$awk_field'}')
 	echo ${TEST_DATA} | awk -F":" '{print $4}'
 }
 
-- 
2.30.2

[PATCH 3/8] test_sysctl: Group node sysctl test under one func

[Joel Granados]

Preparation commit to add a new type of test to test_sysctl.c. We
want to differentiate between node and (sub)directory tests.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 lib/test_sysctl.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index e2a816d85ea2..0cf7c547d61a 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -126,9 +126,7 @@ static struct ctl_table test_table[] = {
 	{ }
 };
 
-static struct ctl_table_header *test_sysctl_header;
-
-static int __init test_sysctl_init(void)
+static void test_sysctl_calc_match_int_ok(void)
 {
 	int i;
 
@@ -153,7 +151,13 @@ static int __init test_sysctl_init(void)
 	for (i = 0; i < ARRAY_SIZE(match_int); i++)
 		if (match_int[i].defined != match_int[i].wanted)
 			match_int_ok = 0;
+}
 
+static struct ctl_table_header *test_sysctl_header;
+
+static int test_sysctl_setup_node_tests(void)
+{
+	test_sysctl_calc_match_int_ok();
 	test_data.bitmap_0001 = kzalloc(SYSCTL_TEST_BITMAP_SIZE/8, GFP_KERNEL);
 	if (!test_data.bitmap_0001)
 		return -ENOMEM;
@@ -162,8 +166,18 @@ static int __init test_sysctl_init(void)
 		kfree(test_data.bitmap_0001);
 		return -ENOMEM;
 	}
+
 	return 0;
 }
+
+static int __init test_sysctl_init(void)
+{
+	int err;
+
+	err = test_sysctl_setup_node_tests();
+
+	return err;
+}
 module_init(test_sysctl_init);
 
 static void __exit test_sysctl_exit(void)
-- 
2.30.2

[PATCH 4/8] test_sysctl: Add an unregister sysctl test

[Joel Granados]

Add a test that checks that the unregistered directory is removed from
/proc/sys/debug

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 lib/test_sysctl.c                        | 30 ++++++++++++++++++++++++
 tools/testing/selftests/sysctl/sysctl.sh | 16 +++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 0cf7c547d61a..555244687443 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -170,12 +170,42 @@ static int test_sysctl_setup_node_tests(void)
 	return 0;
 }
 
+/* Used to test that unregister actually removes the directory */
+static struct ctl_table test_table_unregister[] = {
+	{
+		.procname	= "unregister_error",
+		.data		= &test_data.int_0001,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+	},
+	{}
+};
+
+static int test_sysctl_run_unregister_nested(void)
+{
+	struct ctl_table_header *unregister;
+
+	unregister = register_sysctl("debug/test_sysctl/unregister_error",
+				   test_table_unregister);
+	if (!unregister)
+		return -ENOMEM;
+
+	unregister_sysctl_table(unregister);
+	return 0;
+}
+
 static int __init test_sysctl_init(void)
 {
 	int err;
 
 	err = test_sysctl_setup_node_tests();
+	if (err)
+		goto out;
 
+	err = test_sysctl_run_unregister_nested();
+
+out:
 	return err;
 }
 module_init(test_sysctl_init);
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index cb8f83dfe16b..a6d79d7a36e4 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -31,6 +31,7 @@ ALL_TESTS="$ALL_TESTS 0005:3:1:int_0003"
 ALL_TESTS="$ALL_TESTS 0006:50:1:bitmap_0001"
 ALL_TESTS="$ALL_TESTS 0007:1:1:boot_int"
 ALL_TESTS="$ALL_TESTS 0008:1:1:match_int"
+ALL_TESTS="$ALL_TESTS 0009:1:1:unregister_error"
 
 function allow_user_defaults()
 {
@@ -797,6 +798,20 @@ sysctl_test_0008()
 	return 0
 }
 
+sysctl_test_0009()
+{
+	TARGET="${SYSCTL}/$(get_test_target 0009)"
+	echo -n "Testing if $TARGET unregistered correctly ..."
+	if [ -d $TARGET ]; then
+		echo "TEST FAILED"
+		rc=1
+		test_rc
+	fi
+
+	echo "ok"
+	return 0
+}
+
 list_tests()
 {
 	echo "Test ID list:"
@@ -813,6 +828,7 @@ list_tests()
 	echo "0006 x $(get_test_count 0006) - tests proc_do_large_bitmap()"
 	echo "0007 x $(get_test_count 0007) - tests setting sysctl from kernel boot param"
 	echo "0008 x $(get_test_count 0008) - tests sysctl macro values match"
+	echo "0009 x $(get_test_count 0009) - tests sysct unregister"
 }
 
 usage()
-- 
2.30.2

Re: [PATCH 0/8] Remove child from struct ctl_table

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

On Tue, Jun 06, 2023 at 11:56:26AM -0700, Luis Chamberlain wrote:
> On Fri, Jun 02, 2023 at 01:06:30PM +0200, Joel Granados wrote:
> > Resending as the first set got mangled with smtp error.
> > 
> > This is part of the effort to remove the empty element of the ctl_table
> > structures (used to calculate size) and replace it with an ARRAY_SIZE call. By
> > replacing the child element in struct ctl_table with a flags element we make
> > sure that there are no forward recursions on child nodes and therefore set
> > ourselves up for just using an ARRAY_SIZE. We also added some self tests to
> > make sure that we do not break anything.
> > 
> > Patchset is separated in 4: parport fixes, selftests fixes, selftests additions and
> > replacement of child element. Tested everything with sysctl self tests and everything
> > seems "ok".
> > 
> > 1. parport fixes: @mcgrof: this is related to my previous series and it plugs a
> >    sysct table leak in the parport driver. Please tell me if you want me to repost
> >    the parport series with this one stiched in.
> > 
> > 2. Selftests fixes: Remove the prefixed zeros when passing a awk field to the
> >    awk print command because it was causing $0009 to be interpreted as $0.
> >    Replaced continue with return in sysctl.sh(test_case) so the test actually
> >    gets skipped. The skip decision is now in sysctl.sh(skip_test).
> > 
> > 3. Selftest additions: New test to confirm that unregister actually removes
> >    targets. New test to confirm that permanently empty targets are indeed
> >    created and that no other targets can be created "on top".
> > 
> > 4. Replaced the child pointer in struct ctl_table with a u8 flag. The flag
> >    is used to differentiate between permanently empty targets and non-empty ones.
> > 
> > Comments/feedback greatly appreciated
> 
> This all looks great, thanks so much for doing all this work! I pushed
> to sysctl-next.
I have a version where I replace the u8 flag with an enumeration with
two memebers. Tell me if it make sense to post the V2 and I'll send it
out after fixing it up a bit.

Best

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/8] Remove child from struct ctl_table

[Luis Chamberlain]

On Fri, Jun 02, 2023 at 01:06:30PM +0200, Joel Granados wrote:
> Resending as the first set got mangled with smtp error.
> 
> This is part of the effort to remove the empty element of the ctl_table
> structures (used to calculate size) and replace it with an ARRAY_SIZE call. By
> replacing the child element in struct ctl_table with a flags element we make
> sure that there are no forward recursions on child nodes and therefore set
> ourselves up for just using an ARRAY_SIZE. We also added some self tests to
> make sure that we do not break anything.
> 
> Patchset is separated in 4: parport fixes, selftests fixes, selftests additions and
> replacement of child element. Tested everything with sysctl self tests and everything
> seems "ok".
> 
> 1. parport fixes: @mcgrof: this is related to my previous series and it plugs a
>    sysct table leak in the parport driver. Please tell me if you want me to repost
>    the parport series with this one stiched in.
> 
> 2. Selftests fixes: Remove the prefixed zeros when passing a awk field to the
>    awk print command because it was causing $0009 to be interpreted as $0.
>    Replaced continue with return in sysctl.sh(test_case) so the test actually
>    gets skipped. The skip decision is now in sysctl.sh(skip_test).
> 
> 3. Selftest additions: New test to confirm that unregister actually removes
>    targets. New test to confirm that permanently empty targets are indeed
>    created and that no other targets can be created "on top".
> 
> 4. Replaced the child pointer in struct ctl_table with a u8 flag. The flag
>    is used to differentiate between permanently empty targets and non-empty ones.
> 
> Comments/feedback greatly appreciated

This all looks great, thanks so much for doing all this work! I pushed
to sysctl-next.

  Luis

[PATCH 0/8] Remove child from struct ctl_table

[Joel Granados]

Resending as the first set got mangled with smtp error.

This is part of the effort to remove the empty element of the ctl_table
structures (used to calculate size) and replace it with an ARRAY_SIZE call. By
replacing the child element in struct ctl_table with a flags element we make
sure that there are no forward recursions on child nodes and therefore set
ourselves up for just using an ARRAY_SIZE. We also added some self tests to
make sure that we do not break anything.

Patchset is separated in 4: parport fixes, selftests fixes, selftests additions and
replacement of child element. Tested everything with sysctl self tests and everything
seems "ok".

1. parport fixes: @mcgrof: this is related to my previous series and it plugs a
   sysct table leak in the parport driver. Please tell me if you want me to repost
   the parport series with this one stiched in.

2. Selftests fixes: Remove the prefixed zeros when passing a awk field to the
   awk print command because it was causing $0009 to be interpreted as $0.
   Replaced continue with return in sysctl.sh(test_case) so the test actually
   gets skipped. The skip decision is now in sysctl.sh(skip_test).

3. Selftest additions: New test to confirm that unregister actually removes
   targets. New test to confirm that permanently empty targets are indeed
   created and that no other targets can be created "on top".

4. Replaced the child pointer in struct ctl_table with a u8 flag. The flag
   is used to differentiate between permanently empty targets and non-empty ones.

Comments/feedback greatly appreciated

Best
Joel

Joel Granados (8):
  parport: plug a sysctl register leak
  test_sysctl: Fix test metadata getters
  test_sysctl: Group node sysctl test under one func
  test_sysctl: Add an unregister sysctl test
  test_sysctl: Add an option to prevent test skip
  test_sysclt: Test for registering a mount point
  sysctl: Remove debugging dump_stack
  sysctl: replace child with a flags var

 drivers/parport/procfs.c                 |  23 ++---
 fs/proc/proc_sysctl.c                    |  82 ++++------------
 include/linux/sysctl.h                   |   4 +-
 lib/test_sysctl.c                        |  91 ++++++++++++++++--
 tools/testing/selftests/sysctl/sysctl.sh | 115 +++++++++++++++++------
 5 files changed, 204 insertions(+), 111 deletions(-)

-- 
2.30.2