Hi Neil! Thanks for the patches. Seems to me like a good fix overall. Judging by the syzbot e-mail, you might be posting a refresh of this patch series, so I proffer a few minor review comments below. > On May 20, 2020, at 11:21 PM, NeilBrown <neilb@xxxxxxx> wrote: > > The domain table should be empty at module unload. If it isn't there is > a bug somewhere. So check and report. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651 > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: NeilBrown <neilb@xxxxxxx> > --- > net/sunrpc/sunrpc.h | 1 + > net/sunrpc/sunrpc_syms.c | 2 ++ > net/sunrpc/svcauth.c | 18 ++++++++++++++++++ > 3 files changed, 21 insertions(+) > > diff --git a/net/sunrpc/sunrpc.h b/net/sunrpc/sunrpc.h > index 47a756503d11..f6fe2e6cd65a 100644 > --- a/net/sunrpc/sunrpc.h > +++ b/net/sunrpc/sunrpc.h > @@ -52,4 +52,5 @@ static inline int sock_is_loopback(struct sock *sk) > > int rpc_clients_notifier_register(void); > void rpc_clients_notifier_unregister(void); > +void auth_domain_cleanup(void); > #endif /* _NET_SUNRPC_SUNRPC_H */ > diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c > index f9edaa9174a4..236fadc4a439 100644 > --- a/net/sunrpc/sunrpc_syms.c > +++ b/net/sunrpc/sunrpc_syms.c > @@ -23,6 +23,7 @@ > #include <linux/sunrpc/rpc_pipe_fs.h> > #include <linux/sunrpc/xprtsock.h> > > +#include "sunrpc.h" > #include "netns.h" > > unsigned int sunrpc_net_id; > @@ -131,6 +132,7 @@ cleanup_sunrpc(void) > unregister_rpc_pipefs(); > rpc_destroy_mempool(); > unregister_pernet_subsys(&sunrpc_net_ops); > + auth_domain_cleanup(); > #if IS_ENABLED(CONFIG_SUNRPC_DEBUG) > rpc_unregister_sysctl(); > #endif > diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c > index 552617e3467b..477890e8b9d8 100644 > --- a/net/sunrpc/svcauth.c > +++ b/net/sunrpc/svcauth.c > @@ -205,3 +205,21 @@ struct auth_domain *auth_domain_find(char *name) > return NULL; > } > EXPORT_SYMBOL_GPL(auth_domain_find); > + > +void auth_domain_cleanup(void) > +{ > + /* There should be no auth_domains left at module unload */ Since this is a globally-visible function, could you move this comment into a Doxy documenting comment before the function? It should make clear that the purpose of this function is only for debugging. > + int h; > + bool found = false; > + > + for (h = 0; h < DN_HASHMAX; h++) { > + struct auth_domain *hp; > + > + hlist_for_each_entry(hp, auth_domain_table+h, hash) { > + found = true; > + printk(KERN_WARNING "sunrpc: domain %s still present at module unload.\n", > + hp->name); Nit: Documentation/process/coding-style.rst recommends using the pr_warn() macro here (and equivalents in other patches)... And note that "svc:" is the conventional prefix for server-side warnings. I'm wondering... is it safe to release an auth_domain here if one is found, so that it is not actually orphaned? The warning is information for developers; there's nothing, say, an administrator can do about this situation. > + } > + } > + WARN(found, "sunrpc: auth_domain_table not clean -> memory leak\n"); Not sure a stack trace in addition to the above warning messages adds relevant information. Can you provide a little justification for that? Thanks! > +} > > -- Chuck Lever
