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

[Domenico Andreoli]

On Tue, Feb 08, 2022 at 09:20:42AM -0800, Luis Chamberlain wrote:
> 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@xxxxxxxxxx> 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);

Apologies for the late reply, I was notified the patch is added to the
mm patchset and thought to be late for any update but now I'm not seeing
it anywhere. Maybe it's been dropped?

Forgot of IS_ENABLED(), I would surely use it in the v2.

Not clear what's the benefit in registering the mount point in
fs/file_table.c vs. kernel/sysctl.c.  I'd keep it close to where people
was used to find it, unless there is a good reason otherwise.

Could you please elaborate?

Thanks for your review!

Dom

-- 
rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05