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: This is related to my previous series and it plugs a sysct
table leak in the parport driver. @mcgrof: I'm just leaving this here so we
don't have to retest the parport stuff
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 an enum which is used to
differentiate between permanently empty targets and non-empty ones.
V2: Replaced the u8 flag with an enumeration.
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 an enumeration
drivers/parport/procfs.c | 23 ++---
fs/proc/proc_sysctl.c | 82 ++++------------
include/linux/sysctl.h | 14 ++-
lib/test_sysctl.c | 91 ++++++++++++++++--
tools/testing/selftests/sysctl/sysctl.sh | 115 +++++++++++++++++------
5 files changed, 214 insertions(+), 111 deletions(-)
--
2.30.2
Add a test that checks that the unregistered directory is removed from
/proc/sys/debug
Signed-off-by: Joel Granados <[email protected]>
---
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
Remove unneeded dump_stack in __register_sysctl_table
Signed-off-by: Joel Granados <[email protected]>
---
fs/proc/proc_sysctl.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8873812d22f3..07804097f997 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1406,7 +1406,6 @@ struct ctl_table_header *__register_sysctl_table(
spin_unlock(&sysctl_lock);
fail:
kfree(header);
- dump_stack();
return NULL;
}
--
2.30.2
Test that target gets created by register_sysctl_mount_point and that no
additional target can be created "on top" of a permanently empty sysctl
table.
Create a mount point target (mnt) in the sysctl test driver; try to
create another on top of that (mnt_error). Output an error if
"mnt_error" is present when we run the sysctl selftests.
Signed-off-by: Joel Granados <[email protected]>
---
lib/test_sysctl.c | 45 ++++++++++++++++++++----
tools/testing/selftests/sysctl/sysctl.sh | 16 +++++++++
2 files changed, 55 insertions(+), 6 deletions(-)
diff --git a/lib/test_sysctl.c b/lib/test_sysctl.c
index 555244687443..8036aa91a1cb 100644
--- a/lib/test_sysctl.c
+++ b/lib/test_sysctl.c
@@ -30,6 +30,13 @@ static int i_zero;
static int i_one_hundred = 100;
static int match_int_ok = 1;
+
+static struct {
+ struct ctl_table_header *test_h_setup_node;
+ struct ctl_table_header *test_h_mnt;
+ struct ctl_table_header *test_h_mnterror;
+} sysctl_test_headers;
+
struct test_sysctl_data {
int int_0001;
int int_0002;
@@ -153,16 +160,14 @@ static void test_sysctl_calc_match_int_ok(void)
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;
- test_sysctl_header = register_sysctl("debug/test_sysctl", test_table);
- if (!test_sysctl_header) {
+ sysctl_test_headers.test_h_setup_node = register_sysctl("debug/test_sysctl", test_table);
+ if (!sysctl_test_headers.test_h_setup_node) {
kfree(test_data.bitmap_0001);
return -ENOMEM;
}
@@ -195,6 +200,26 @@ static int test_sysctl_run_unregister_nested(void)
return 0;
}
+static int test_sysctl_run_register_mount_point(void)
+{
+ sysctl_test_headers.test_h_mnt
+ = register_sysctl_mount_point("debug/test_sysctl/mnt");
+ if (!sysctl_test_headers.test_h_mnt)
+ return -ENOMEM;
+
+ sysctl_test_headers.test_h_mnterror
+ = register_sysctl("debug/test_sysctl/mnt/mnt_error",
+ test_table_unregister);
+ /*
+ * Don't check the result.:
+ * If it fails (expected behavior), return 0.
+ * If successful (missbehavior of register mount point), we want to see
+ * mnt_error when we run the sysctl test script
+ */
+
+ return 0;
+}
+
static int __init test_sysctl_init(void)
{
int err;
@@ -204,6 +229,10 @@ static int __init test_sysctl_init(void)
goto out;
err = test_sysctl_run_unregister_nested();
+ if (err)
+ goto out;
+
+ err = test_sysctl_run_register_mount_point();
out:
return err;
@@ -213,8 +242,12 @@ module_init(test_sysctl_init);
static void __exit test_sysctl_exit(void)
{
kfree(test_data.bitmap_0001);
- if (test_sysctl_header)
- unregister_sysctl_table(test_sysctl_header);
+ if (sysctl_test_headers.test_h_setup_node)
+ unregister_sysctl_table(sysctl_test_headers.test_h_setup_node);
+ if (sysctl_test_headers.test_h_mnt)
+ unregister_sysctl_table(sysctl_test_headers.test_h_mnt);
+ if (sysctl_test_headers.test_h_mnterror)
+ unregister_sysctl_table(sysctl_test_headers.test_h_mnterror);
}
module_exit(test_sysctl_exit);
diff --git a/tools/testing/selftests/sysctl/sysctl.sh b/tools/testing/selftests/sysctl/sysctl.sh
index 9c0e9711138b..444b2befda82 100755
--- a/tools/testing/selftests/sysctl/sysctl.sh
+++ b/tools/testing/selftests/sysctl/sysctl.sh
@@ -34,6 +34,7 @@ ALL_TESTS="$ALL_TESTS 0006:50:1:bitmap_0001:1"
ALL_TESTS="$ALL_TESTS 0007:1:1:boot_int:1"
ALL_TESTS="$ALL_TESTS 0008:1:1:match_int:1"
ALL_TESTS="$ALL_TESTS 0009:1:1:unregister_error:0"
+ALL_TESTS="$ALL_TESTS 0010:1:1:mnt/mnt_error:0"
function allow_user_defaults()
{
@@ -813,6 +814,20 @@ sysctl_test_0009()
return 0
}
+sysctl_test_0010()
+{
+ TARGET="${SYSCTL}/$(get_test_target 0010)"
+ echo -n "Testing that $TARGET was not created ..."
+ if [ -d $TARGET ]; then
+ echo "TEST FAILED"
+ rc=1
+ test_rc
+ fi
+
+ echo "ok"
+ return 0
+}
+
list_tests()
{
echo "Test ID list:"
@@ -830,6 +845,7 @@ list_tests()
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"
+ echo "0010 x $(get_test_count 0010) - tests sysct mount point"
}
usage()
--
2.30.2
This is part of the effort to remove the empty element at the end of
ctl_table structs. "child" was a deprecated elem in this struct and was
being used to differentiate between two types of ctl_tables: "normal"
and "permanently emtpy".
What changed?:
* Replace "child" with an enumeration that will have two values: the
default (0) and the permanently empty (1). The latter is left at zero
so when struct ctl_table is created with kzalloc or in a local
context, it will have the zero value by default. We document the
new enum with kdoc.
* Remove the "empty child" check from sysctl_check_table
* Remove count_subheaders function as there is no longer a need to
calculate how many headers there are for every child
* Remove the recursive call to unregister_sysctl_table as there is no
need to traverse down the child tree any longer
* Add a new SYSCTL_PERM_EMPTY_DIR binary flag
* Remove the last remanence of child from partport/procfs.c
Signed-off-by: Joel Granados <[email protected]>
---
drivers/parport/procfs.c | 1 -
fs/proc/proc_sysctl.c | 81 +++++++++-------------------------------
include/linux/sysctl.h | 14 ++++++-
3 files changed, 30 insertions(+), 66 deletions(-)
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 0f2d2e1ee28e..4e5b972c3e26 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -387,7 +387,6 @@ parport_device_sysctl_template = {
.data = NULL,
.maxlen = 0,
.mode = 0555,
- .child = NULL
},
{}
}
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 07804097f997..c4ea804d862b 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -29,9 +29,8 @@ static const struct file_operations proc_sys_dir_file_operations;
static const struct inode_operations proc_sys_dir_operations;
/* Support for permanently empty directories */
-
struct ctl_table sysctl_mount_point[] = {
- { }
+ {.type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY }
};
/**
@@ -48,21 +47,14 @@ struct ctl_table_header *register_sysctl_mount_point(const char *path)
}
EXPORT_SYMBOL(register_sysctl_mount_point);
-static bool is_empty_dir(struct ctl_table_header *head)
-{
- return head->ctl_table[0].child == sysctl_mount_point;
-}
-
-static void set_empty_dir(struct ctl_dir *dir)
-{
- dir->header.ctl_table[0].child = sysctl_mount_point;
-}
-
-static void clear_empty_dir(struct ctl_dir *dir)
-
-{
- dir->header.ctl_table[0].child = NULL;
-}
+#define sysctl_is_perm_empty_ctl_table(tptr) \
+ (tptr[0].type == SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
+#define sysctl_is_perm_empty_ctl_header(hptr) \
+ (sysctl_is_perm_empty_ctl_table(hptr->ctl_table))
+#define sysctl_set_perm_empty_ctl_header(hptr) \
+ (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY)
+#define sysctl_clear_perm_empty_ctl_header(hptr) \
+ (hptr->ctl_table[0].type = SYSCTL_TABLE_TYPE_DEFAULT)
void proc_sys_poll_notify(struct ctl_table_poll *poll)
{
@@ -230,20 +222,22 @@ static void erase_header(struct ctl_table_header *head)
static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
{
struct ctl_table *entry;
+ struct ctl_table_header *dir_h = &dir->header;
int err;
+
/* Is this a permanently empty directory? */
- if (is_empty_dir(&dir->header))
+ if (sysctl_is_perm_empty_ctl_header(dir_h))
return -EROFS;
/* Am I creating a permanently empty directory? */
- if (header->ctl_table == sysctl_mount_point) {
+ if (sysctl_is_perm_empty_ctl_table(header->ctl_table)) {
if (!RB_EMPTY_ROOT(&dir->root))
return -EINVAL;
- set_empty_dir(dir);
+ sysctl_set_perm_empty_ctl_header(dir_h);
}
- dir->header.nreg++;
+ dir_h->nreg++;
header->parent = dir;
err = insert_links(header);
if (err)
@@ -259,9 +253,9 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
put_links(header);
fail_links:
if (header->ctl_table == sysctl_mount_point)
- clear_empty_dir(dir);
+ sysctl_clear_perm_empty_ctl_header(dir_h);
header->parent = NULL;
- drop_sysctl_table(&dir->header);
+ drop_sysctl_table(dir_h);
return err;
}
@@ -479,7 +473,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb,
inode->i_mode |= S_IFDIR;
inode->i_op = &proc_sys_dir_operations;
inode->i_fop = &proc_sys_dir_file_operations;
- if (is_empty_dir(head))
+ if (sysctl_is_perm_empty_ctl_header(head))
make_empty_dir_inode(inode);
}
@@ -1136,9 +1130,6 @@ static int sysctl_check_table(const char *path, struct ctl_table *table)
struct ctl_table *entry;
int err = 0;
list_for_each_table_entry(entry, table) {
- if (entry->child)
- err |= sysctl_err(path, entry, "Not a file");
-
if ((entry->proc_handler == proc_dostring) ||
(entry->proc_handler == proc_dobool) ||
(entry->proc_handler == proc_dointvec) ||
@@ -1465,25 +1456,6 @@ void __init __register_sysctl_init(const char *path, struct ctl_table *table,
kmemleak_not_leak(hdr);
}
-static int count_subheaders(struct ctl_table *table)
-{
- int has_files = 0;
- int nr_subheaders = 0;
- struct ctl_table *entry;
-
- /* special case: no directory and empty directory */
- if (!table || !table->procname)
- return 1;
-
- list_for_each_table_entry(entry, table) {
- if (entry->child)
- nr_subheaders += count_subheaders(entry->child);
- else
- has_files = 1;
- }
- return nr_subheaders + has_files;
-}
-
static void put_links(struct ctl_table_header *header)
{
struct ctl_table_set *root_set = &sysctl_table_root.default_set;
@@ -1546,28 +1518,11 @@ static void drop_sysctl_table(struct ctl_table_header *header)
*/
void unregister_sysctl_table(struct ctl_table_header * header)
{
- int nr_subheaders;
might_sleep();
if (header == NULL)
return;
- nr_subheaders = count_subheaders(header->ctl_table_arg);
- if (unlikely(nr_subheaders > 1)) {
- struct ctl_table_header **subheaders;
- int i;
-
- subheaders = (struct ctl_table_header **)(header + 1);
- for (i = nr_subheaders -1; i >= 0; i--) {
- struct ctl_table_header *subh = subheaders[i];
- struct ctl_table *table = subh->ctl_table_arg;
- unregister_sysctl_table(subh);
- kfree(table);
- }
- kfree(header);
- return;
- }
-
spin_lock(&sysctl_lock);
drop_sysctl_table(header);
spin_unlock(&sysctl_lock);
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 653b66c762b1..59d451f455bf 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -137,7 +137,17 @@ struct ctl_table {
void *data;
int maxlen;
umode_t mode;
- struct ctl_table *child; /* Deprecated */
+ /**
+ * enum type - Enumeration to differentiate between ctl target types
+ * @SYSCTL_TABLE_TYPE_DEFAULT: ctl target with no special considerations
+ * @SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY: Used to identify a permanently
+ * empty directory target to serve
+ * as mount point.
+ */
+ enum {
+ SYSCTL_TABLE_TYPE_DEFAULT,
+ SYSCTL_TABLE_TYPE_PERMANENTLY_EMPTY
+ } type;
proc_handler *proc_handler; /* Callback for text formatting */
struct ctl_table_poll *poll;
void *extra1;
@@ -229,7 +239,7 @@ extern int unaligned_enabled;
extern int unaligned_dump_stack;
extern int no_unaligned_warning;
-extern struct ctl_table sysctl_mount_point[];
+#define SYSCTL_PERM_EMPTY_DIR (1 << 0)
#else /* CONFIG_SYSCTL */
--
2.30.2
On Fri, Jun 16, 2023 at 10:59:14AM +0200, Joel Granados wrote:
> 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: This is related to my previous series and it plugs a sysct
> table leak in the parport driver. @mcgrof: I'm just leaving this here so we
> don't have to retest the parport stuff
>
> 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 an enum which is used to
> differentiate between permanently empty targets and non-empty ones.
>
> V2: Replaced the u8 flag with an enumeration.
Thanks, I dropped the old patches and applied this new set. Pushed out
to sysctl-next.
Luis
On Sun, Jun 18, 2023 at 02:33:35AM -0700, Luis Chamberlain wrote:
> On Fri, Jun 16, 2023 at 10:59:14AM +0200, Joel Granados wrote:
> > 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: This is related to my previous series and it plugs a sysct
> > table leak in the parport driver. @mcgrof: I'm just leaving this here so we
> > don't have to retest the parport stuff
> >
> > 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 an enum which is used to
> > differentiate between permanently empty targets and non-empty ones.
> >
> > V2: Replaced the u8 flag with an enumeration.
>
> Thanks, I dropped the old patches and applied this new set. Pushed out
> to sysctl-next.
Awesome! thx.
>
> Luis
--
Joel Granados