Re: [PATCH] Fix regression due to "fs: move binfmt_misc sysctl to its own file"

[Luis Chamberlain <mcgrof@kernel.org>]

On Mon, Feb 07, 2022 at 02:53:21PM -0800, Tong Zhang wrote:
> On Mon, Feb 7, 2022 at 1:46 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > OK I think the issue here should have been that declaring this on
> > fs/binfmt_misc.c creates the chicken and the egg issue, and so we
> > need to do this on built-in code. Instead of your patch can you try
> > this instead, it just always creates the sysctl mount always now
> > so long as proc sysctl is enabled. It does not do the unregistration
> > as we always want the path present as it used to be before as well.
> >
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 57edef16dce4..4f4739c9405c 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -119,6 +119,7 @@ static struct ctl_table fs_stat_sysctls[] = {
> >  static int __init init_fs_stat_sysctls(void)
> >  {
> >         register_sysctl_init("fs", fs_stat_sysctls);
> > +       register_sysctl_mount_point("fs/binfmt_misc");
> >         return 0;
> >  }
> >  fs_initcall(init_fs_stat_sysctls);
> 
> I'm looking at the original code, and it seems that if we don't select
> CONFIG_BINFMT_MISC at all,
> this file won't be created. This would suggest an IF MACRO around
> > +       register_sysctl_mount_point("fs/binfmt_misc");
> and it should looks like
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +       register_sysctl_mount_point("fs/binfmt_misc");
> +#endif
> or if you prefer original style:
> #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)

Or better yet using IS_ENABLED() to avoid the ifdef cruft:

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index c07f35719ee3..4b8f1b11a7c8 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -817,20 +817,16 @@ static struct file_system_type bm_fs_type = {
 };
 MODULE_ALIAS_FS("binfmt_misc");
 
-static struct ctl_table_header *binfmt_misc_header;
-
 static int __init init_misc_binfmt(void)
 {
 	int err = register_filesystem(&bm_fs_type);
 	if (!err)
 		insert_binfmt(&misc_format);
-	binfmt_misc_header = register_sysctl_mount_point("fs/binfmt_misc");
 	return 0;
 }
 
 static void __exit exit_misc_binfmt(void)
 {
-	unregister_sysctl_table(binfmt_misc_header);
 	unregister_binfmt(&misc_format);
 	unregister_filesystem(&bm_fs_type);
 }
diff --git a/fs/file_table.c b/fs/file_table.c
index 57edef16dce4..4969021fa676 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -119,6 +119,8 @@ static struct ctl_table fs_stat_sysctls[] = {
 static int __init init_fs_stat_sysctls(void)
 {
 	register_sysctl_init("fs", fs_stat_sysctls);
+	if (IS_ENABLED(CONFIG_BINFMT_MISC))
+		register_sysctl_mount_point("fs/binfmt_misc");
 	return 0;
 }
 fs_initcall(init_fs_stat_sysctls);