This is part of the general push to deprecate register_sysctl_paths and
register_sysctl_table. This patchset completely removes register_sysctl_table
and replaces it with register_sysctl effectively transitioning 5 base paths
("kernel", "vm", "fs", "dev" and "debug") to the new call. Besides removing the
actuall function, I also removed it from the checks done in check-sysctl-docs.

Testing for this change was done in the same way as with previous sysctl
replacement patches: I made sure that the result of `find /proc/sys/ | sha1sum`
was the same before and after the patchset.

Have pushed this through 0-day. Waiting on results..

Feedback greatly appreciated.

Best
Joel

Joel Granados (2):
  sysctl: Refactor base paths registrations
  sysctl: Remove register_sysctl_table

 fs/proc/proc_sysctl.c     | 70 ---------------------------------------
 fs/sysctls.c              |  9 +++--
 include/linux/sysctl.h    | 23 -------------
 kernel/sysctl.c           | 13 +++-----
 scripts/check-sysctl-docs | 10 ------
 5 files changed, 10 insertions(+), 115 deletions(-)

-- 
2.30.2

On Thu, May 18, 2023 at 06:07:03PM +0200, Joel Granados wrote:
> This is part of the general push to deprecate register_sysctl_paths and
> register_sysctl_table. This patchset completely removes register_sysctl_table
> and replaces it with register_sysctl effectively transitioning 5 base paths
> ("kernel", "vm", "fs", "dev" and "debug") to the new call. Besides removing the
> actuall function, I also removed it from the checks done in check-sysctl-docs.
> 
> Testing for this change was done in the same way as with previous sysctl
> replacement patches: I made sure that the result of `find /proc/sys/ | sha1sum`
> was the same before and after the patchset.
> 
> Have pushed this through 0-day. Waiting on results..
> 
> Feedback greatly appreciated.

Thanks so much! I merged this to sysctl-testing as build tests are ongoing. But
I incorporated these minor changes to your first patch as register_sysctl_init()
is more obvious about when we cannot care about the return value.

If the build tests come through I'll push to sysctl-next.

diff --git a/fs/sysctls.c b/fs/sysctls.c
index 228420f5fe1b..76a0aee8c229 100644
--- a/fs/sysctls.c
+++ b/fs/sysctls.c
@@ -31,11 +31,7 @@ static struct ctl_table fs_shared_sysctls[] = {
 
 static int __init init_fs_sysctls(void)
 {
-	/*
-	 * We do not check the return code for register_sysctl because the
-	 * original call to register_sysctl_base always returned 0.
-	 */
-	register_sysctl("fs", fs_shared_sysctls);
+	register_sysctl_init("fs", fs_shared_sysctls);
 	return 0;
 }
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index f784b0fe5689..fa2aa8bd32b6 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2350,10 +2350,10 @@ static struct ctl_table dev_table[] = {
 
 int __init sysctl_init_bases(void)
 {
-	register_sysctl("kernel", kern_table);
-	register_sysctl("vm", vm_table);
-	register_sysctl("debug", debug_table);
-	register_sysctl("dev", dev_table);
+	register_sysctl_init("kernel", kern_table);
+	register_sysctl_init("vm", vm_table);
+	register_sysctl_init("debug", debug_table);
+	register_sysctl_init("dev", dev_table);
 
 	return 0;
 }

On Thu, May 18, 2023 at 01:46:44PM -0700, Luis Chamberlain wrote:
> On Thu, May 18, 2023 at 06:07:03PM +0200, Joel Granados wrote:
> > This is part of the general push to deprecate register_sysctl_paths and
> > register_sysctl_table. This patchset completely removes register_sysctl_table
> > and replaces it with register_sysctl effectively transitioning 5 base paths
> > ("kernel", "vm", "fs", "dev" and "debug") to the new call. Besides removing the
> > actuall function, I also removed it from the checks done in check-sysctl-docs.
> > 
> > Testing for this change was done in the same way as with previous sysctl
> > replacement patches: I made sure that the result of `find /proc/sys/ | sha1sum`
> > was the same before and after the patchset.
> > 
> > Have pushed this through 0-day. Waiting on results..
> > 
> > Feedback greatly appreciated.
> 
> Thanks so much! I merged this to sysctl-testing as build tests are ongoing. But
> I incorporated these minor changes to your first patch as register_sysctl_init()
> is more obvious about when we cannot care about the return value.
> 
> If the build tests come through I'll push to sysctl-next.
> 

I also had to apply this (yay more nuking):

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 7bc7d3c3a215..8873812d22f3 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1466,19 +1466,6 @@ void __init __register_sysctl_init(const char *path, struct ctl_table *table,
 	kmemleak_not_leak(hdr);
 }
 
-static char *append_path(const char *path, char *pos, const char *name)
-{
-	int namelen;
-	namelen = strlen(name);
-	if (((pos - path) + namelen + 2) >= PATH_MAX)
-		return NULL;
-	memcpy(pos, name, namelen);
-	pos[namelen] = '/';
-	pos[namelen + 1] = '\0';
-	pos += namelen + 1;
-	return pos;
-}
-
 static int count_subheaders(struct ctl_table *table)
 {
 	int has_files = 0;
@@ -1498,82 +1485,6 @@ static int count_subheaders(struct ctl_table *table)
 	return nr_subheaders + has_files;
 }
 
-static int register_leaf_sysctl_tables(const char *path, char *pos,
-	struct ctl_table_header ***subheader, struct ctl_table_set *set,
-	struct ctl_table *table)
-{
-	struct ctl_table *ctl_table_arg = NULL;
-	struct ctl_table *entry, *files;
-	int nr_files = 0;
-	int nr_dirs = 0;
-	int err = -ENOMEM;
-
-	list_for_each_table_entry(entry, table) {
-		if (entry->child)
-			nr_dirs++;
-		else
-			nr_files++;
-	}
-
-	files = table;
-	/* If there are mixed files and directories we need a new table */
-	if (nr_dirs && nr_files) {
-		struct ctl_table *new;
-		files = kcalloc(nr_files + 1, sizeof(struct ctl_table),
-				GFP_KERNEL);
-		if (!files)
-			goto out;
-
-		ctl_table_arg = files;
-		new = files;
-
-		list_for_each_table_entry(entry, table) {
-			if (entry->child)
-				continue;
-			*new = *entry;
-			new++;
-		}
-	}
-
-	/* Register everything except a directory full of subdirectories */
-	if (nr_files || !nr_dirs) {
-		struct ctl_table_header *header;
-		header = __register_sysctl_table(set, path, files);
-		if (!header) {
-			kfree(ctl_table_arg);
-			goto out;
-		}
-
-		/* Remember if we need to free the file table */
-		header->ctl_table_arg = ctl_table_arg;
-		**subheader = header;
-		(*subheader)++;
-	}
-
-	/* Recurse into the subdirectories. */
-	list_for_each_table_entry(entry, table) {
-		char *child_pos;
-
-		if (!entry->child)
-			continue;
-
-		err = -ENAMETOOLONG;
-		child_pos = append_path(path, pos, entry->procname);
-		if (!child_pos)
-			goto out;
-
-		err = register_leaf_sysctl_tables(path, child_pos, subheader,
-						  set, entry->child);
-		pos[0] = '\0';
-		if (err)
-			goto out;
-	}
-	err = 0;
-out:
-	/* On failure our caller will unregister all registered subheaders */
-	return err;
-}
-
 static void put_links(struct ctl_table_header *header)
 {
 	struct ctl_table_set *root_set = &sysctl_table_root.default_set;

On Thu, May 18, 2023 at 05:26:34PM -0700, Luis Chamberlain wrote:
> On Thu, May 18, 2023 at 01:46:44PM -0700, Luis Chamberlain wrote:
> > On Thu, May 18, 2023 at 06:07:03PM +0200, Joel Granados wrote:
> > > This is part of the general push to deprecate register_sysctl_paths and
> > > register_sysctl_table. This patchset completely removes register_sysctl_table
> > > and replaces it with register_sysctl effectively transitioning 5 base paths
> > > ("kernel", "vm", "fs", "dev" and "debug") to the new call. Besides removing the
> > > actuall function, I also removed it from the checks done in check-sysctl-docs.
> > > 
> > > Testing for this change was done in the same way as with previous sysctl
> > > replacement patches: I made sure that the result of `find /proc/sys/ | sha1sum`
> > > was the same before and after the patchset.
> > > 
> > > Have pushed this through 0-day. Waiting on results..
> > > 
> > > Feedback greatly appreciated.
> > 
> > Thanks so much! I merged this to sysctl-testing as build tests are ongoing. But
> > I incorporated these minor changes to your first patch as register_sysctl_init()
> > is more obvious about when we cannot care about the return value.

nice! thx.

> > 
> > If the build tests come through I'll push to sysctl-next.
> > 
> 
> I also had to apply this (yay more nuking):
Indeed. I just saw the results of 0-day and there was a warning
regarding these functions. Thx again.

best
joel
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 7bc7d3c3a215..8873812d22f3 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1466,19 +1466,6 @@ void __init __register_sysctl_init(const char *path, struct ctl_table *table,
>  	kmemleak_not_leak(hdr);
>  }
>  
> -static char *append_path(const char *path, char *pos, const char *name)
> -{
> -	int namelen;
> -	namelen = strlen(name);
> -	if (((pos - path) + namelen + 2) >= PATH_MAX)
> -		return NULL;
> -	memcpy(pos, name, namelen);
> -	pos[namelen] = '/';
> -	pos[namelen + 1] = '\0';
> -	pos += namelen + 1;
> -	return pos;
> -}
> -
>  static int count_subheaders(struct ctl_table *table)
>  {
>  	int has_files = 0;
> @@ -1498,82 +1485,6 @@ static int count_subheaders(struct ctl_table *table)
>  	return nr_subheaders + has_files;
>  }
>  
> -static int register_leaf_sysctl_tables(const char *path, char *pos,
> -	struct ctl_table_header ***subheader, struct ctl_table_set *set,
> -	struct ctl_table *table)
> -{
> -	struct ctl_table *ctl_table_arg = NULL;
> -	struct ctl_table *entry, *files;
> -	int nr_files = 0;
> -	int nr_dirs = 0;
> -	int err = -ENOMEM;
> -
> -	list_for_each_table_entry(entry, table) {
> -		if (entry->child)
> -			nr_dirs++;
> -		else
> -			nr_files++;
> -	}
> -
> -	files = table;
> -	/* If there are mixed files and directories we need a new table */
> -	if (nr_dirs && nr_files) {
> -		struct ctl_table *new;
> -		files = kcalloc(nr_files + 1, sizeof(struct ctl_table),
> -				GFP_KERNEL);
> -		if (!files)
> -			goto out;
> -
> -		ctl_table_arg = files;
> -		new = files;
> -
> -		list_for_each_table_entry(entry, table) {
> -			if (entry->child)
> -				continue;
> -			*new = *entry;
> -			new++;
> -		}
> -	}
> -
> -	/* Register everything except a directory full of subdirectories */
> -	if (nr_files || !nr_dirs) {
> -		struct ctl_table_header *header;
> -		header = __register_sysctl_table(set, path, files);
> -		if (!header) {
> -			kfree(ctl_table_arg);
> -			goto out;
> -		}
> -
> -		/* Remember if we need to free the file table */
> -		header->ctl_table_arg = ctl_table_arg;
> -		**subheader = header;
> -		(*subheader)++;
> -	}
> -
> -	/* Recurse into the subdirectories. */
> -	list_for_each_table_entry(entry, table) {
> -		char *child_pos;
> -
> -		if (!entry->child)
> -			continue;
> -
> -		err = -ENAMETOOLONG;
> -		child_pos = append_path(path, pos, entry->procname);
> -		if (!child_pos)
> -			goto out;
> -
> -		err = register_leaf_sysctl_tables(path, child_pos, subheader,
> -						  set, entry->child);
> -		pos[0] = '\0';
> -		if (err)
> -			goto out;
> -	}
> -	err = 0;
> -out:
> -	/* On failure our caller will unregister all registered subheaders */
> -	return err;
> -}
> -
>  static void put_links(struct ctl_table_header *header)
>  {
>  	struct ctl_table_set *root_set = &sysctl_table_root.default_set;

-- 

Joel Granados