From: "J. Bruce Fields" <[email protected]>
These pernet operations may depend on stuff set up or torn down in the
module init/exit functions. And they may be called at any time in
between. So it makes more sense for them to be the last to be
registered in the init function, and the first to be unregistered in the
exit function.
In particular, without this, the drc slab is being destroyed before all
the per-net drcs are shut down, resulting in an "Objects remaining in
nfsd_drc on __kmem_cache_shutdown()" warning in exit_nfsd.
Reported-by: Zhi Li <[email protected]>
Fixes: 3ba75830ce17 "nfsd4: drc containerization"
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfsctl.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index f6d5d783f4a4..0759e589ab52 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1522,12 +1522,9 @@ static int __init init_nfsd(void)
int retval;
printk(KERN_INFO "Installing knfsd (copyright (C) 1996 [email protected]).\n");
- retval = register_pernet_subsys(&nfsd_net_ops);
- if (retval < 0)
- return retval;
retval = register_cld_notifier();
if (retval)
- goto out_unregister_pernet;
+ return retval;
retval = nfsd4_init_slabs();
if (retval)
goto out_unregister_notifier;
@@ -1544,9 +1541,14 @@ static int __init init_nfsd(void)
goto out_free_lockd;
retval = register_filesystem(&nfsd_fs_type);
if (retval)
+ goto out_free_exports;
+ retval = register_pernet_subsys(&nfsd_net_ops);
+ if (retval < 0)
goto out_free_all;
return 0;
out_free_all:
+ unregister_pernet_subsys(&nfsd_net_ops);
+out_free_exports:
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
out_free_lockd:
@@ -1559,13 +1561,12 @@ static int __init init_nfsd(void)
nfsd4_free_slabs();
out_unregister_notifier:
unregister_cld_notifier();
-out_unregister_pernet:
- unregister_pernet_subsys(&nfsd_net_ops);
return retval;
}
static void __exit exit_nfsd(void)
{
+ unregister_pernet_subsys(&nfsd_net_ops);
nfsd_drc_slab_free();
remove_proc_entry("fs/nfs/exports", NULL);
remove_proc_entry("fs/nfs", NULL);
@@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
nfsd4_exit_pnfs();
unregister_filesystem(&nfsd_fs_type);
unregister_cld_notifier();
- unregister_pernet_subsys(&nfsd_net_ops);
}
MODULE_AUTHOR("Olaf Kirch <[email protected]>");
--
2.29.2
Hi Bruce-
> On Feb 3, 2021, at 11:42 AM, J. Bruce Fields <[email protected]> wrote:
>
> From: "J. Bruce Fields" <[email protected]>
>
> These pernet operations may depend on stuff set up or torn down in the
> module init/exit functions. And they may be called at any time in
> between. So it makes more sense for them to be the last to be
> registered in the init function, and the first to be unregistered in the
> exit function.
>
> In particular, without this, the drc slab is being destroyed before all
> the per-net drcs are shut down, resulting in an "Objects remaining in
> nfsd_drc on __kmem_cache_shutdown()" warning in exit_nfsd.
>
> Reported-by: Zhi Li <[email protected]>
> Fixes: 3ba75830ce17 "nfsd4: drc containerization"
> Signed-off-by: J. Bruce Fields <[email protected]>
I can't tell how urgent this is. Does it belong in 5.11-rc?
> ---
> fs/nfsd/nfsctl.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index f6d5d783f4a4..0759e589ab52 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1522,12 +1522,9 @@ static int __init init_nfsd(void)
> int retval;
> printk(KERN_INFO "Installing knfsd (copyright (C) 1996 [email protected]).\n");
>
> - retval = register_pernet_subsys(&nfsd_net_ops);
> - if (retval < 0)
> - return retval;
> retval = register_cld_notifier();
> if (retval)
> - goto out_unregister_pernet;
> + return retval;
> retval = nfsd4_init_slabs();
> if (retval)
> goto out_unregister_notifier;
> @@ -1544,9 +1541,14 @@ static int __init init_nfsd(void)
> goto out_free_lockd;
> retval = register_filesystem(&nfsd_fs_type);
> if (retval)
> + goto out_free_exports;
> + retval = register_pernet_subsys(&nfsd_net_ops);
> + if (retval < 0)
> goto out_free_all;
> return 0;
> out_free_all:
> + unregister_pernet_subsys(&nfsd_net_ops);
> +out_free_exports:
> remove_proc_entry("fs/nfs/exports", NULL);
> remove_proc_entry("fs/nfs", NULL);
> out_free_lockd:
> @@ -1559,13 +1561,12 @@ static int __init init_nfsd(void)
> nfsd4_free_slabs();
> out_unregister_notifier:
> unregister_cld_notifier();
> -out_unregister_pernet:
> - unregister_pernet_subsys(&nfsd_net_ops);
> return retval;
> }
>
> static void __exit exit_nfsd(void)
> {
> + unregister_pernet_subsys(&nfsd_net_ops);
> nfsd_drc_slab_free();
> remove_proc_entry("fs/nfs/exports", NULL);
> remove_proc_entry("fs/nfs", NULL);
> @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
> nfsd4_exit_pnfs();
> unregister_filesystem(&nfsd_fs_type);
> unregister_cld_notifier();
> - unregister_pernet_subsys(&nfsd_net_ops);
> }
>
> MODULE_AUTHOR("Olaf Kirch <[email protected]>");
> --
> 2.29.2
>
--
Chuck Lever
On Wed, Feb 03, 2021 at 04:45:56PM +0000, Chuck Lever wrote:
> Hi Bruce-
>
>
> > On Feb 3, 2021, at 11:42 AM, J. Bruce Fields <[email protected]> wrote:
> >
> > From: "J. Bruce Fields" <[email protected]>
> >
> > These pernet operations may depend on stuff set up or torn down in the
> > module init/exit functions. And they may be called at any time in
> > between. So it makes more sense for them to be the last to be
> > registered in the init function, and the first to be unregistered in the
> > exit function.
> >
> > In particular, without this, the drc slab is being destroyed before all
> > the per-net drcs are shut down, resulting in an "Objects remaining in
> > nfsd_drc on __kmem_cache_shutdown()" warning in exit_nfsd.
> >
> > Reported-by: Zhi Li <[email protected]>
> > Fixes: 3ba75830ce17 "nfsd4: drc containerization"
> > Signed-off-by: J. Bruce Fields <[email protected]>
>
> I can't tell how urgent this is. Does it belong in 5.11-rc?
I dunno, I wonder what happens when you try to write to and then free a
bunch of objects that were allocated from a slab that no longer exists.
But, it's triggered by unloading nfsd, and I find it hard to be super
concerned about module unloading bugs (does anyone actually *need* to
unload the nfsd module?).
--b.
>
>
> > ---
> > fs/nfsd/nfsctl.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index f6d5d783f4a4..0759e589ab52 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1522,12 +1522,9 @@ static int __init init_nfsd(void)
> > int retval;
> > printk(KERN_INFO "Installing knfsd (copyright (C) 1996 [email protected]).\n");
> >
> > - retval = register_pernet_subsys(&nfsd_net_ops);
> > - if (retval < 0)
> > - return retval;
> > retval = register_cld_notifier();
> > if (retval)
> > - goto out_unregister_pernet;
> > + return retval;
> > retval = nfsd4_init_slabs();
> > if (retval)
> > goto out_unregister_notifier;
> > @@ -1544,9 +1541,14 @@ static int __init init_nfsd(void)
> > goto out_free_lockd;
> > retval = register_filesystem(&nfsd_fs_type);
> > if (retval)
> > + goto out_free_exports;
> > + retval = register_pernet_subsys(&nfsd_net_ops);
> > + if (retval < 0)
> > goto out_free_all;
> > return 0;
> > out_free_all:
> > + unregister_pernet_subsys(&nfsd_net_ops);
> > +out_free_exports:
> > remove_proc_entry("fs/nfs/exports", NULL);
> > remove_proc_entry("fs/nfs", NULL);
> > out_free_lockd:
> > @@ -1559,13 +1561,12 @@ static int __init init_nfsd(void)
> > nfsd4_free_slabs();
> > out_unregister_notifier:
> > unregister_cld_notifier();
> > -out_unregister_pernet:
> > - unregister_pernet_subsys(&nfsd_net_ops);
> > return retval;
> > }
> >
> > static void __exit exit_nfsd(void)
> > {
> > + unregister_pernet_subsys(&nfsd_net_ops);
> > nfsd_drc_slab_free();
> > remove_proc_entry("fs/nfs/exports", NULL);
> > remove_proc_entry("fs/nfs", NULL);
> > @@ -1575,7 +1576,6 @@ static void __exit exit_nfsd(void)
> > nfsd4_exit_pnfs();
> > unregister_filesystem(&nfsd_fs_type);
> > unregister_cld_notifier();
> > - unregister_pernet_subsys(&nfsd_net_ops);
> > }
> >
> > MODULE_AUTHOR("Olaf Kirch <[email protected]>");
> > --
> > 2.29.2
> >
>
> --
> Chuck Lever
>
>
> On Feb 3, 2021, at 12:52 PM, Bruce Fields <[email protected]> wrote:
>
> On Wed, Feb 03, 2021 at 04:45:56PM +0000, Chuck Lever wrote:
>> Hi Bruce-
>>
>>
>>> On Feb 3, 2021, at 11:42 AM, J. Bruce Fields <[email protected]> wrote:
>>>
>>> From: "J. Bruce Fields" <[email protected]>
>>>
>>> These pernet operations may depend on stuff set up or torn down in the
>>> module init/exit functions. And they may be called at any time in
>>> between. So it makes more sense for them to be the last to be
>>> registered in the init function, and the first to be unregistered in the
>>> exit function.
>>>
>>> In particular, without this, the drc slab is being destroyed before all
>>> the per-net drcs are shut down, resulting in an "Objects remaining in
>>> nfsd_drc on __kmem_cache_shutdown()" warning in exit_nfsd.
>>>
>>> Reported-by: Zhi Li <[email protected]>
>>> Fixes: 3ba75830ce17 "nfsd4: drc containerization"
>>> Signed-off-by: J. Bruce Fields <[email protected]>
>>
>> I can't tell how urgent this is. Does it belong in 5.11-rc?
>
> I dunno, I wonder what happens when you try to write to and then free a
> bunch of objects that were allocated from a slab that no longer exists.
>
> But, it's triggered by unloading nfsd, and I find it hard to be super
> concerned about module unloading bugs (does anyone actually *need* to
> unload the nfsd module?).
This doesn't sound urgent, then. My initial 5.12 changeset is closed,
so I'll send this in the first 5.12-rc, unless someone grumbles.
--
Chuck Lever