2022-02-08 11:56:23

by Tong Zhang

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

On Mon, Feb 7, 2022 at 1:46 PM Luis Chamberlain <[email protected]> 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)

- Tong

2022-02-09 05:21:59

by Luis Chamberlain

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

On Mon, Feb 07, 2022 at 02:53:21PM -0800, Tong Zhang wrote:
> On Mon, Feb 7, 2022 at 1:46 PM Luis Chamberlain <[email protected]> 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);