| Message ID | 20210203164213.GA26588@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: register pernet ops last, unregister first | expand |
Hi Bruce- > On Feb 3, 2021, at 11:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > 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 <yieli@redhat.com> > Fixes: 3ba75830ce17 "nfsd4: drc containerization" > Signed-off-by: J. Bruce Fields <bfields@redhat.com> 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 okir@monad.swb.de).\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 <okir@monad.swb.de>"); > -- > 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 <bfields@fieldses.org> wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > 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 <yieli@redhat.com> > > Fixes: 3ba75830ce17 "nfsd4: drc containerization" > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > 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 okir@monad.swb.de).\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 <okir@monad.swb.de>"); > > -- > > 2.29.2 > > > > -- > Chuck Lever > >
> On Feb 3, 2021, at 12:52 PM, Bruce Fields <bfields@fieldses.org> 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 <bfields@fieldses.org> wrote: >>> >>> From: "J. Bruce Fields" <bfields@redhat.com> >>> >>> 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 <yieli@redhat.com> >>> Fixes: 3ba75830ce17 "nfsd4: drc containerization" >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >> >> 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
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 okir@monad.swb.de).\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 <okir@monad.swb.de>");