> On May 21, 2020, at 7:45 PM, NeilBrown <neilb@xxxxxxx> wrote: > > On Thu, May 21 2020, Chuck Lever wrote: > >> 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. > > I wouldn't call it "globally-visible" as it isn't exported, and isn't > even declared in linux/include/... > But a Doxy comment is probably justified. > >> >> >>> + 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'll fix that, thanks. > >> >> 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. > > I don't think it is safe to release the domain. The ->release() > function could be in a module that has already been unloaded. A comment to that effect would be good. Up to you! >>> + } >>> + } >>> + 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? > > I guess so. I wanted a nice loud warning - and people tend to notice > stack traces more than they notice printks - it was an attempt at human > engineering :-) > > Maybe I'll just leave it as pr_warn... > > Thanks for the review. > > NeilBrown > > >> >> Thanks! >> >> >>> +} >>> >>> >> >> -- >> Chuck Lever
