[PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems.

[PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems.

[NeilBrown]

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

[PATCH 1/3] sunrpc: check that domain table is empty at module unload.

[NeilBrown]

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);
+}

[PATCH 2/3] sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations.

[NeilBrown]

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;

[PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister()

[NeilBrown]

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

Re: [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister()

[Sasha Levin]

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

Re: [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister()

[NeilBrown]

[-- 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 --]

Re: [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems.

[J. Bruce Fields]

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