* [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems. @ 2020-05-22 2:01 NeilBrown 2020-05-22 2:01 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: NeilBrown @ 2020-05-22 2:01 UTC (permalink / raw) To: J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger; +Cc: linux-nfs In this second version: - #include added in first patch so new function is declare both where it is defined and where it is used - doxy comment added for auth_domain_cleanup() - pr_warn() used instead of printk - 'svc:' used as message prefix - EADDRINUSE returned instead of EALREADY - I think it is slightly more accurate. - 'cc: stable' dropped for first patch. Bug goes back before 'git' so no 'Fixes:' - minor code revision Thanks, NeilBrown --- NeilBrown (3): sunrpc: check that domain table is empty at module unload. sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations. sunrpc: clean up properly in gss_mech_unregister() include/linux/sunrpc/gss_api.h | 1 + include/linux/sunrpc/svcauth_gss.h | 3 ++- net/sunrpc/auth_gss/gss_mech_switch.c | 12 +++++++++--- net/sunrpc/auth_gss/svcauth_gss.c | 18 ++++++++++-------- net/sunrpc/sunrpc.h | 1 + net/sunrpc/sunrpc_syms.c | 2 ++ net/sunrpc/svcauth.c | 25 +++++++++++++++++++++++++ 7 files changed, 50 insertions(+), 12 deletions(-) -- Signature ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-22 2:01 [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems NeilBrown @ 2020-05-22 2:01 ` NeilBrown 2020-05-22 2:01 ` [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() NeilBrown ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: NeilBrown @ 2020-05-22 2:01 UTC (permalink / raw) To: J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger; +Cc: linux-nfs 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 Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/sunrpc.h | 1 + net/sunrpc/sunrpc_syms.c | 2 ++ net/sunrpc/svcauth.c | 25 +++++++++++++++++++++++++ 3 files changed, 28 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..998b196b6176 100644 --- a/net/sunrpc/svcauth.c +++ b/net/sunrpc/svcauth.c @@ -21,6 +21,8 @@ #include <trace/events/sunrpc.h> +#include "sunrpc.h" + #define RPCDBG_FACILITY RPCDBG_AUTH @@ -205,3 +207,26 @@ struct auth_domain *auth_domain_find(char *name) return NULL; } EXPORT_SYMBOL_GPL(auth_domain_find); + +/** + * auth_domain_cleanup - check that the auth_domain table is empty + * + * On module unload the auth_domain_table must be empty. To make it + * easier to catch bugs which don't clean up domains properly, we + * warn if anything remains in the table at cleanup time. + * + * Note that we cannot proactively remove the domains at this stage. + * The ->release() function might be in a module that has already been + * unloaded. + */ + +void auth_domain_cleanup(void) +{ + int h; + struct auth_domain *hp; + + for (h = 0; h < DN_HASHMAX; h++) + hlist_for_each_entry(hp, &auth_domain_table[h], hash) + pr_warn("svc: domain %s still present at module unload.\n", + hp->name); +} ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() 2020-05-22 2:01 [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems NeilBrown 2020-05-22 2:01 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown @ 2020-05-22 2:01 ` NeilBrown 2020-05-26 0:23 ` Sasha Levin 2020-05-22 2:01 ` [PATCH 2/3] sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations NeilBrown 2020-05-28 22:26 ` [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems J. Bruce Fields 3 siblings, 1 reply; 13+ messages in thread From: NeilBrown @ 2020-05-22 2:01 UTC (permalink / raw) To: J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger; +Cc: linux-nfs gss_mech_register() calls svcauth_gss_register_pseudoflavor() for each flavour, but gss_mech_unregister() does not call auth_domain_put(). This is unbalanced and makes it impossible to reload the module. Change svcauth_gss_register_pseudoflavor() to return the registered auth_domain, and save it for later release. Cc: stable@vger.kernel.org (v2.6.12+) Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651 Signed-off-by: NeilBrown <neilb@suse.de> --- include/linux/sunrpc/gss_api.h | 1 + include/linux/sunrpc/svcauth_gss.h | 3 ++- net/sunrpc/auth_gss/gss_mech_switch.c | 12 +++++++++--- net/sunrpc/auth_gss/svcauth_gss.c | 12 ++++++------ 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/include/linux/sunrpc/gss_api.h b/include/linux/sunrpc/gss_api.h index bc07e51f20d1..bf4ac8a0268c 100644 --- a/include/linux/sunrpc/gss_api.h +++ b/include/linux/sunrpc/gss_api.h @@ -84,6 +84,7 @@ struct pf_desc { u32 service; char *name; char *auth_domain_name; + struct auth_domain *domain; bool datatouch; }; diff --git a/include/linux/sunrpc/svcauth_gss.h b/include/linux/sunrpc/svcauth_gss.h index ca39a388dc22..8983628b10ff 100644 --- a/include/linux/sunrpc/svcauth_gss.h +++ b/include/linux/sunrpc/svcauth_gss.h @@ -20,7 +20,8 @@ int gss_svc_init(void); void gss_svc_shutdown(void); int gss_svc_init_net(struct net *net); void gss_svc_shutdown_net(struct net *net); -int svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name); +struct auth_domain *svcauth_gss_register_pseudoflavor(u32 pseudoflavor, + char *name); u32 svcauth_gss_flavor(struct auth_domain *dom); #endif /* _LINUX_SUNRPC_SVCAUTH_GSS_H */ diff --git a/net/sunrpc/auth_gss/gss_mech_switch.c b/net/sunrpc/auth_gss/gss_mech_switch.c index 69316ab1b9fa..fae632da1058 100644 --- a/net/sunrpc/auth_gss/gss_mech_switch.c +++ b/net/sunrpc/auth_gss/gss_mech_switch.c @@ -37,6 +37,8 @@ gss_mech_free(struct gss_api_mech *gm) for (i = 0; i < gm->gm_pf_num; i++) { pf = &gm->gm_pfs[i]; + if (pf->domain) + auth_domain_put(pf->domain); kfree(pf->auth_domain_name); pf->auth_domain_name = NULL; } @@ -59,6 +61,7 @@ make_auth_domain_name(char *name) static int gss_mech_svc_setup(struct gss_api_mech *gm) { + struct auth_domain *dom; struct pf_desc *pf; int i, status; @@ -68,10 +71,13 @@ gss_mech_svc_setup(struct gss_api_mech *gm) status = -ENOMEM; if (pf->auth_domain_name == NULL) goto out; - status = svcauth_gss_register_pseudoflavor(pf->pseudoflavor, - pf->auth_domain_name); - if (status) + dom = svcauth_gss_register_pseudoflavor( + pf->pseudoflavor, pf->auth_domain_name); + if (IS_ERR(dom)) { + status = PTR_ERR(dom); goto out; + } + pf->domain = dom; } return 0; out: diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c index 49bb346a6215..46027d0c903f 100644 --- a/net/sunrpc/auth_gss/svcauth_gss.c +++ b/net/sunrpc/auth_gss/svcauth_gss.c @@ -809,7 +809,7 @@ u32 svcauth_gss_flavor(struct auth_domain *dom) EXPORT_SYMBOL_GPL(svcauth_gss_flavor); -int +struct auth_domain * svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name) { struct gss_domain *new; @@ -832,17 +832,17 @@ svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name) name); stat = -EADDRINUSE; auth_domain_put(test); - kfree(new->h.name); - goto out_free_dom; + goto out_free_name; } - return 0; + return test; +out_free_name: + kfree(new->h.name); out_free_dom: kfree(new); out: - return stat; + return ERR_PTR(stat); } - EXPORT_SYMBOL_GPL(svcauth_gss_register_pseudoflavor); static inline int ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() 2020-05-22 2:01 ` [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() NeilBrown @ 2020-05-26 0:23 ` Sasha Levin 2020-05-26 0:50 ` NeilBrown 0 siblings, 1 reply; 13+ messages in thread From: Sasha Levin @ 2020-05-26 0:23 UTC (permalink / raw) To: Sasha Levin, NeilBrown, J. Bruce Fields; +Cc: linux-nfs, stable Hi [This is an automated email] This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: 2.6.12+ The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224. v5.6.14: Build OK! v5.4.42: Build OK! v4.19.124: Build OK! v4.14.181: Build OK! v4.9.224: Build OK! v4.4.224: Failed to apply! Possible dependencies: 302d3deb2068 ("xprtrdma: Prevent inline overflow") 65b80179f9b8 ("xprtrdma: No direct data placement with krb5i and krb5p") 94f58c58c0b4 ("xprtrdma: Allow Read list and Reply chunk simultaneously") cce6deeb56aa ("xprtrdma: Avoid using Write list for small NFS READ requests") NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks Sasha ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() 2020-05-26 0:23 ` Sasha Levin @ 2020-05-26 0:50 ` NeilBrown 0 siblings, 0 replies; 13+ messages in thread From: NeilBrown @ 2020-05-26 0:50 UTC (permalink / raw) To: Sasha Levin, Sasha Levin, J. Bruce Fields; +Cc: linux-nfs, stable [-- Attachment #1: Type: text/plain, Size: 1301 bytes --] On Tue, May 26 2020, Sasha Levin wrote: > Hi > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: 2.6.12+ > > The bot has tested the following trees: v5.6.14, v5.4.42, v4.19.124, v4.14.181, v4.9.224, v4.4.224. > > v5.6.14: Build OK! > v5.4.42: Build OK! > v4.19.124: Build OK! > v4.14.181: Build OK! > v4.9.224: Build OK! > v4.4.224: Failed to apply! Possible dependencies: > 302d3deb2068 ("xprtrdma: Prevent inline overflow") > 65b80179f9b8 ("xprtrdma: No direct data placement with krb5i and krb5p") > 94f58c58c0b4 ("xprtrdma: Allow Read list and Reply chunk simultaneously") > cce6deeb56aa ("xprtrdma: Avoid using Write list for small NFS READ requests") > > > NOTE: The patch will not be queued to stable trees until it is upstream. > > How should we proceed with this patch? The conflict is trivial - this patch adds a field (domain) to 'struct pf_desc" which at the time didn't have an expected field (datatouch). You can merge it manually or ask me when the time comes and I'll provide a backport. (65b80179f9b8 is the textual dependency, but it is no a semantic dependency). NeilBrown > > -- > Thanks > Sasha [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations. 2020-05-22 2:01 [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems NeilBrown 2020-05-22 2:01 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown 2020-05-22 2:01 ` [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() NeilBrown @ 2020-05-22 2:01 ` NeilBrown 2020-05-28 22:26 ` [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems J. Bruce Fields 3 siblings, 0 replies; 13+ messages in thread From: NeilBrown @ 2020-05-22 2:01 UTC (permalink / raw) To: J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger; +Cc: linux-nfs There is no valid case for supporting duplicate pseudoflavor registrations. Currently the silent acceptance of such registrations is hiding a bug. The rpcsec_gss_krb5 module registers 2 flavours but does not unregister them, so if you load, unload, reload the module, it will happily continue to use the old registration which now has pointers to the memory were the module was originally loaded. This could lead to unexpected results. So disallow duplicate registrations. Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651 Cc: stable@vger.kernel.org (v2.6.12+) Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/auth_gss/svcauth_gss.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c index 50d93c49ef1a..49bb346a6215 100644 --- a/net/sunrpc/auth_gss/svcauth_gss.c +++ b/net/sunrpc/auth_gss/svcauth_gss.c @@ -826,9 +826,11 @@ svcauth_gss_register_pseudoflavor(u32 pseudoflavor, char * name) new->h.flavour = &svcauthops_gss; new->pseudoflavor = pseudoflavor; - stat = 0; test = auth_domain_lookup(name, &new->h); - if (test != &new->h) { /* Duplicate registration */ + if (test != &new->h) { + pr_warn("svc: duplicate registration of gss pseudo flavour %s.\n", + name); + stat = -EADDRINUSE; auth_domain_put(test); kfree(new->h.name); goto out_free_dom; ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems. 2020-05-22 2:01 [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems NeilBrown ` (2 preceding siblings ...) 2020-05-22 2:01 ` [PATCH 2/3] sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations NeilBrown @ 2020-05-28 22:26 ` J. Bruce Fields 3 siblings, 0 replies; 13+ messages in thread From: J. Bruce Fields @ 2020-05-28 22:26 UTC (permalink / raw) To: NeilBrown; +Cc: Chuck Lever, kircherlike, Stephen Hemminger, linux-nfs Applying for 5.8, thanks! --b. On Fri, May 22, 2020 at 12:01:32PM +1000, NeilBrown wrote: > In this second version: > - #include added in first patch so new function is declare both where > it is defined and where it is used > - doxy comment added for auth_domain_cleanup() > - pr_warn() used instead of printk > - 'svc:' used as message prefix > - EADDRINUSE returned instead of EALREADY - I think it is slightly more > accurate. > - 'cc: stable' dropped for first patch. Bug goes back before 'git' so > no 'Fixes:' > - minor code revision > > Thanks, > NeilBrown > --- > > NeilBrown (3): > sunrpc: check that domain table is empty at module unload. > sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations. > sunrpc: clean up properly in gss_mech_unregister() > > > include/linux/sunrpc/gss_api.h | 1 + > include/linux/sunrpc/svcauth_gss.h | 3 ++- > net/sunrpc/auth_gss/gss_mech_switch.c | 12 +++++++++--- > net/sunrpc/auth_gss/svcauth_gss.c | 18 ++++++++++-------- > net/sunrpc/sunrpc.h | 1 + > net/sunrpc/sunrpc_syms.c | 2 ++ > net/sunrpc/svcauth.c | 25 +++++++++++++++++++++++++ > 7 files changed, 50 insertions(+), 12 deletions(-) > > -- > Signature ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/3] SUNRPC/svc: fix gss flavour registration problems. @ 2020-05-21 3:21 NeilBrown 2020-05-21 3:21 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown 0 siblings, 1 reply; 13+ messages in thread From: NeilBrown @ 2020-05-21 3:21 UTC (permalink / raw) To: J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger; +Cc: linux-nfs As reported in https://bugzilla.kernel.org/show_bug.cgi?id=206651 there are problems with sunrpc/svc flavour registration. This can be demonstrated as a memory-leak if you load the rpcsec_gss_krb5 module, then unload the sunrpc module and all dependents. This action leaks 3 kmalloc-64 slab entires, and some strings. The possible consequences are worse. If only unload rpcsec_gss_krb5 and reload just that, it will allow the old registered flavour handlers to be used, and they will include pointers into memory which has since been freed and possibly reused. This can result in undesired behaviour. The first patch makes the leak apparent with a WARNing, the second prevents it but also prevents module reload, the third removes the incorrect behaviour so the module can be safely unloaded and reloaded. I think all are suitable for -stable, but I haven't determined appropriate 'Fixes:' tags. NeilBrown --- NeilBrown (3): sunrpc: check that domain table is empty at module unload. sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations. sunrpc: clean up properly in gss_mech_unregister() include/linux/sunrpc/gss_api.h | 1 + include/linux/sunrpc/svcauth_gss.h | 3 ++- net/sunrpc/auth_gss/gss_mech_switch.c | 12 +++++++++--- net/sunrpc/auth_gss/svcauth_gss.c | 17 ++++++++++------- net/sunrpc/sunrpc.h | 1 + net/sunrpc/sunrpc_syms.c | 2 ++ net/sunrpc/svcauth.c | 18 ++++++++++++++++++ 7 files changed, 43 insertions(+), 11 deletions(-) -- Signature ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-21 3:21 [PATCH 0/3] " NeilBrown @ 2020-05-21 3:21 ` NeilBrown 2020-05-21 7:09 ` kbuild test robot ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: NeilBrown @ 2020-05-21 3:21 UTC (permalink / raw) To: J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger; +Cc: linux-nfs 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@vger.kernel.org Signed-off-by: NeilBrown <neilb@suse.de> --- 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 */ + 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); + } + } + WARN(found, "sunrpc: auth_domain_table not clean -> memory leak\n"); +} ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-21 3:21 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown @ 2020-05-21 7:09 ` kbuild test robot 2020-05-21 12:39 ` kbuild test robot 2020-05-21 14:06 ` Chuck Lever 2 siblings, 0 replies; 13+ messages in thread From: kbuild test robot @ 2020-05-21 7:09 UTC (permalink / raw) To: NeilBrown, J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger Cc: kbuild-all, linux-nfs [-- Attachment #1: Type: text/plain, Size: 3223 bytes --] Hi NeilBrown, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on nfsd/nfsd-next] [also build test WARNING on nfs/linux-next cel/for-next v5.7-rc6 next-20200519] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/NeilBrown/SUNRPC-svc-fix-gss-flavour-registration-problems/20200521-112443 base: git://linux-nfs.org/~bfields/linux.git nfsd-next config: um-allmodconfig (attached as .config) compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make ARCH=um If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>, old ones prefixed by <<): cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs] In file included from include/linux/uaccess.h:11:0, from include/linux/sched/task.h:11, from include/linux/sched/signal.h:9, from include/linux/sunrpc/types.h:14, from net/sunrpc/svcauth.c:15: arch/um/include/asm/uaccess.h: In function '__access_ok': arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] (((unsigned long) (addr) >= FIXADDR_USER_START) && ^ arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall' __access_ok_vsyscall(addr, size) || ^~~~~~~~~~~~~~~~~~~~ In file included from include/linux/kernel.h:11:0, from include/linux/list.h:9, from include/linux/module.h:12, from net/sunrpc/svcauth.c:14: include/asm-generic/fixmap.h: In function 'fix_to_virt': include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] BUILD_BUG_ON(idx >= __end_of_fixed_addresses); ^ include/linux/compiler.h:330:9: note: in definition of macro '__compiletime_assert' if (!(condition)) ^~~~~~~~~ include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert' _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) ^~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) ^~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG' BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) ^~~~~~~~~~~~~~~~ include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON' BUILD_BUG_ON(idx >= __end_of_fixed_addresses); ^~~~~~~~~~~~ net/sunrpc/svcauth.c: At top level: >> net/sunrpc/svcauth.c:209:6: warning: no previous prototype for 'auth_domain_cleanup' [-Wmissing-prototypes] void auth_domain_cleanup(void) ^~~~~~~~~~~~~~~~~~~ vim +/auth_domain_cleanup +209 net/sunrpc/svcauth.c 208 > 209 void auth_domain_cleanup(void) --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 22561 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-21 3:21 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown 2020-05-21 7:09 ` kbuild test robot @ 2020-05-21 12:39 ` kbuild test robot 2020-05-21 14:06 ` Chuck Lever 2 siblings, 0 replies; 13+ messages in thread From: kbuild test robot @ 2020-05-21 12:39 UTC (permalink / raw) To: NeilBrown, J. Bruce Fields, Chuck Lever, kircherlike, Stephen Hemminger Cc: kbuild-all, clang-built-linux, linux-nfs [-- Attachment #1: Type: text/plain, Size: 1900 bytes --] Hi NeilBrown, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on nfsd/nfsd-next] [also build test WARNING on nfs/linux-next v5.7-rc6 next-20200519] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/NeilBrown/SUNRPC-svc-fix-gss-flavour-registration-problems/20200521-112443 base: git://linux-nfs.org/~bfields/linux.git nfsd-next config: arm-randconfig-r035-20200521 (attached as .config) compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b) reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm If you fix the issue, kindly add following tag as appropriate Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>, old ones prefixed by <<): >> net/sunrpc/svcauth.c:209:6: warning: no previous prototype for function 'auth_domain_cleanup' [-Wmissing-prototypes] void auth_domain_cleanup(void) ^ net/sunrpc/svcauth.c:209:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void auth_domain_cleanup(void) ^ static 1 warning generated. vim +/auth_domain_cleanup +209 net/sunrpc/svcauth.c 208 > 209 void auth_domain_cleanup(void) --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 39799 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-21 3:21 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown 2020-05-21 7:09 ` kbuild test robot 2020-05-21 12:39 ` kbuild test robot @ 2020-05-21 14:06 ` Chuck Lever 2020-05-21 23:44 ` NeilBrown 2 siblings, 1 reply; 13+ messages in thread From: Chuck Lever @ 2020-05-21 14:06 UTC (permalink / raw) To: Neil Brown Cc: Bruce Fields, kircherlike, Stephen Hemminger, Linux NFS Mailing List 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@suse.de> 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@vger.kernel.org > Signed-off-by: NeilBrown <neilb@suse.de> > --- > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-21 14:06 ` Chuck Lever @ 2020-05-21 23:44 ` NeilBrown 2020-05-22 0:33 ` Chuck Lever 0 siblings, 1 reply; 13+ messages in thread From: NeilBrown @ 2020-05-21 23:44 UTC (permalink / raw) To: Chuck Lever Cc: Bruce Fields, kircherlike, Stephen Hemminger, Linux NFS Mailing List [-- Attachment #1: Type: text/plain, Size: 4109 bytes --] 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@suse.de> 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@vger.kernel.org >> Signed-off-by: NeilBrown <neilb@suse.de> >> --- >> 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload. 2020-05-21 23:44 ` NeilBrown @ 2020-05-22 0:33 ` Chuck Lever 0 siblings, 0 replies; 13+ messages in thread From: Chuck Lever @ 2020-05-22 0:33 UTC (permalink / raw) To: NeilBrown Cc: Bruce Fields, kircherlike, Stephen Hemminger, Linux NFS Mailing List > On May 21, 2020, at 7:45 PM, NeilBrown <neilb@suse.de> 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@suse.de> 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@vger.kernel.org >>> Signed-off-by: NeilBrown <neilb@suse.de> >>> --- >>> 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-05-28 22:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-22 2:01 [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems NeilBrown 2020-05-22 2:01 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown 2020-05-22 2:01 ` [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() NeilBrown 2020-05-26 0:23 ` Sasha Levin 2020-05-26 0:50 ` NeilBrown 2020-05-22 2:01 ` [PATCH 2/3] sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations NeilBrown 2020-05-28 22:26 ` [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems J. Bruce Fields -- strict thread matches above, loose matches on Subject: below -- 2020-05-21 3:21 [PATCH 0/3] " NeilBrown 2020-05-21 3:21 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown 2020-05-21 7:09 ` kbuild test robot 2020-05-21 12:39 ` kbuild test robot 2020-05-21 14:06 ` Chuck Lever 2020-05-21 23:44 ` NeilBrown 2020-05-22 0:33 ` Chuck Lever
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).