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. > > >> + } >> + } >> + 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
Attachment:
signature.asc
Description: PGP signature
