[PATCH 0/3] fix backchannel crash due to multipath

[PATCH 0/3] fix backchannel crash due to multipath

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

pynfs can crash the nfs server since the new multipath code.  The
problem is that pynfs uses the same tcp connection for multiple v4
clients (something the spec explicitly allows).  The callback client in
such cases has to share the same rpc_xprt.  The new rpc code, when it
creates two callback clients sharing the same rpc_xprt, ends up trying
to keep that rpc_xprt on the lists of two different rpc_xprt_switch's.
The first symptom I see is a list corruption warning.

So, I'm enforcing the requirement that there be only one backchannel
rpc_xprt_switch per tcp connection by doing as we do in the rpc_xprt
case: keeping a pointer to it in the svc_xprt, and using that when it's
available instead of allocating a new one.

That's a pretty straightforward fix (and I've verified it works), but
doesn't look very elegant.  If there's a better solution, I'm all ears.

--b.

J. Bruce Fields (3):
  SUNRPC: fix xprt leak on xps allocation failure
  nfsd4/rpc: move backchannel create logic into rpc code
  rpc: share one xps between all backchannels

 fs/nfsd/nfs4callback.c          | 18 +-----------------
 include/linux/sunrpc/clnt.h     |  2 --
 include/linux/sunrpc/svc_xprt.h |  1 +
 include/linux/sunrpc/xprt.h     |  1 +
 net/sunrpc/clnt.c               | 31 +++++++++++++++++++++++++------
 net/sunrpc/svc_xprt.c           |  2 ++
 net/sunrpc/xprtsock.c           |  1 +
 7 files changed, 31 insertions(+), 25 deletions(-)

-- 
2.5.5

[PATCH 1/3] SUNRPC: fix xprt leak on xps allocation failure

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Callers of rpc_create_xprt expect it to put the xprt on success and
failure.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/clnt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Alternatively, maybe we should just change that convention and leave it
to callers to put the xprt on failure?

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 7e0c9bf22df8..a87e29ac993e 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -453,9 +453,10 @@ struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
 	struct rpc_xprt_switch *xps;
 
 	xps = xprt_switch_alloc(xprt, GFP_KERNEL);
-	if (xps == NULL)
+	if (xps == NULL) {
+		xprt_put(xprt);
 		return ERR_PTR(-ENOMEM);
-
+	}
 	clnt = rpc_new_client(args, xps, xprt, NULL);
 	if (IS_ERR(clnt))
 		return clnt;
-- 
2.5.5

[PATCH 2/3] nfsd4/rpc: move backchannel create logic into rpc code

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

Also simplify the logic a bit.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4callback.c      | 18 +-----------------
 include/linux/sunrpc/clnt.h |  2 --
 net/sunrpc/clnt.c           | 12 ++++++++++--
 3 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 7389cb1d7409..04c68d900324 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -710,22 +710,6 @@ static struct rpc_cred *get_backchannel_cred(struct nfs4_client *clp, struct rpc
 	}
 }
 
-static struct rpc_clnt *create_backchannel_client(struct rpc_create_args *args)
-{
-	struct rpc_xprt *xprt;
-
-	if (args->protocol != XPRT_TRANSPORT_BC_TCP)
-		return rpc_create(args);
-
-	xprt = args->bc_xprt->xpt_bc_xprt;
-	if (xprt) {
-		xprt_get(xprt);
-		return rpc_create_xprt(args, xprt);
-	}
-
-	return rpc_create(args);
-}
-
 static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *conn, struct nfsd4_session *ses)
 {
 	int maxtime = max_cb_time(clp->net);
@@ -768,7 +752,7 @@ static int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *c
 		args.authflavor = ses->se_cb_sec.flavor;
 	}
 	/* Create RPC client */
-	client = create_backchannel_client(&args);
+	client = rpc_create(&args);
 	if (IS_ERR(client)) {
 		dprintk("NFSD: couldn't create callback client: %ld\n",
 			PTR_ERR(client));
diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index 9a7ddbaf116e..14d70f59f0c2 100644
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -137,8 +137,6 @@ struct rpc_create_args {
 #define RPC_CLNT_CREATE_NO_RETRANS_TIMEOUT	(1UL << 9)
 
 struct rpc_clnt *rpc_create(struct rpc_create_args *args);
-struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
-					struct rpc_xprt *xprt);
 struct rpc_clnt	*rpc_bind_new_program(struct rpc_clnt *,
 				const struct rpc_program *, u32);
 struct rpc_clnt *rpc_clone_client(struct rpc_clnt *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index a87e29ac993e..ad3f5ebec2d1 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -446,7 +446,7 @@ out_no_rpciod:
 	return ERR_PTR(err);
 }
 
-struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
+static struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
 					struct rpc_xprt *xprt)
 {
 	struct rpc_clnt *clnt = NULL;
@@ -484,7 +484,6 @@ struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
 
 	return clnt;
 }
-EXPORT_SYMBOL_GPL(rpc_create_xprt);
 
 /**
  * rpc_create - create an RPC client and transport with one call
@@ -510,6 +509,15 @@ struct rpc_clnt *rpc_create(struct rpc_create_args *args)
 	};
 	char servername[48];
 
+	if (args->bc_xprt) {
+		WARN_ON(args->protocol != XPRT_TRANSPORT_BC_TCP);
+		xprt = args->bc_xprt->xpt_bc_xprt;
+		if (xprt) {
+			xprt_get(xprt);
+			return rpc_create_xprt(args, xprt);
+		}
+	}
+
 	if (args->flags & RPC_CLNT_CREATE_INFINITE_SLOTS)
 		xprtargs.flags |= XPRT_CREATE_INFINITE_SLOTS;
 	if (args->flags & RPC_CLNT_CREATE_NO_IDLE_TIMEOUT)
-- 
2.5.5

[PATCH 3/3] rpc: share one xps between all backchannels

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

The spec allows backchannels for multiple clients to share the same tcp
connection.  When that happens, we need to use the same xprt for all of
them.  Similarly, we need the same xps.

This fixes list corruption introduced by the multipath code.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 include/linux/sunrpc/svc_xprt.h |  1 +
 include/linux/sunrpc/xprt.h     |  1 +
 net/sunrpc/clnt.c               | 18 ++++++++++++++----
 net/sunrpc/svc_xprt.c           |  2 ++
 net/sunrpc/xprtsock.c           |  1 +
 5 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index b7dabc4baafd..79ba50856707 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -84,6 +84,7 @@ struct svc_xprt {
 
 	struct net		*xpt_net;
 	struct rpc_xprt		*xpt_bc_xprt;	/* NFSv4.1 backchannel */
+	struct rpc_xprt_switch	*xpt_bc_xps;	/* NFSv4.1 backchannel */
 };
 
 static inline void unregister_xpt_user(struct svc_xprt *xpt, struct svc_xpt_user *u)
diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.h
index fb0d212e0d3a..9f51e1df3023 100644
--- a/include/linux/sunrpc/xprt.h
+++ b/include/linux/sunrpc/xprt.h
@@ -296,6 +296,7 @@ struct xprt_create {
 	size_t			addrlen;
 	const char		*servername;
 	struct svc_xprt		*bc_xprt;	/* NFSv4.1 backchannel */
+	struct rpc_xprt_switch	*bc_xps;
 	unsigned int		flags;
 };
 
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index ad3f5ebec2d1..837dd910a252 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -452,10 +452,20 @@ static struct rpc_clnt *rpc_create_xprt(struct rpc_create_args *args,
 	struct rpc_clnt *clnt = NULL;
 	struct rpc_xprt_switch *xps;
 
-	xps = xprt_switch_alloc(xprt, GFP_KERNEL);
-	if (xps == NULL) {
-		xprt_put(xprt);
-		return ERR_PTR(-ENOMEM);
+	if (args->bc_xprt && args->bc_xprt->xpt_bc_xps) {
+		WARN_ON(args->protocol != XPRT_TRANSPORT_BC_TCP);
+		xps = args->bc_xprt->xpt_bc_xps;
+		xprt_switch_get(xps);
+	} else {
+		xps = xprt_switch_alloc(xprt, GFP_KERNEL);
+		if (xps == NULL) {
+			xprt_put(xprt);
+			return ERR_PTR(-ENOMEM);
+		}
+		if (xprt->bc_xprt) {
+			xprt_switch_get(xps);
+			xprt->bc_xprt->xpt_bc_xps = xps;
+		}
 	}
 	clnt = rpc_new_client(args, xps, xprt, NULL);
 	if (IS_ERR(clnt))
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index f5572e31d518..4f01f63102ee 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -136,6 +136,8 @@ static void svc_xprt_free(struct kref *kref)
 	/* See comment on corresponding get in xs_setup_bc_tcp(): */
 	if (xprt->xpt_bc_xprt)
 		xprt_put(xprt->xpt_bc_xprt);
+	if (xprt->xpt_bc_xps)
+		xprt_switch_put(xprt->xpt_bc_xps);
 	xprt->xpt_ops->xpo_free(xprt);
 	module_put(owner);
 }
diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 65e759569e48..e9e5dd0dc8f4 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -3050,6 +3050,7 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 		return xprt;
 
 	args->bc_xprt->xpt_bc_xprt = NULL;
+	args->bc_xprt->xpt_bc_xps = NULL;
 	xprt_put(xprt);
 	ret = ERR_PTR(-EINVAL);
 out_err:
-- 
2.5.5

Re: [PATCH 0/3] fix backchannel crash due to multipath

[J. Bruce Fields]

On March 24 J. Bruce Fields said:
> pynfs can crash the nfs server since the new multipath code.  The
> problem is that pynfs uses the same tcp connection for multiple v4
> clients (something the spec explicitly allows).  The callback client in
> such cases has to share the same rpc_xprt.  The new rpc code, when it
> creates two callback clients sharing the same rpc_xprt, ends up trying
> to keep that rpc_xprt on the lists of two different rpc_xprt_switch's.
> The first symptom I see is a list corruption warning.
>
> So, I'm enforcing the requirement that there be only one backchannel
> rpc_xprt_switch per tcp connection by doing as we do in the rpc_xprt
> case: keeping a pointer to it in the svc_xprt, and using that when it's
> available instead of allocating a new one.

> That's a pretty straightforward fix (and I've verified it works), but
> doesn't look very elegant.  If there's a better solution, I'm all ears.

I haven't come up with anything neater, and kind of need the crash
fixed.  ACK or NACK?  I can take it through my tree if it's an ACK.

--b.

Re: [PATCH 0/3] fix backchannel crash due to multipath

[Trond Myklebust]

DQoNCk9uIDYvMTMvMTYsIDE0OjIxLCAiSi4gQnJ1Y2UgRmllbGRzIiA8YmZpZWxkc0BmaWVsZHNl
cy5vcmc+IHdyb3RlOg0KDQo+T24gTWFyY2ggMjQgSi4gQnJ1Y2UgRmllbGRzIHNhaWQ6DQo+PiBw
eW5mcyBjYW4gY3Jhc2ggdGhlIG5mcyBzZXJ2ZXIgc2luY2UgdGhlIG5ldyBtdWx0aXBhdGggY29k
ZS4gIFRoZQ0KPj4gcHJvYmxlbSBpcyB0aGF0IHB5bmZzIHVzZXMgdGhlIHNhbWUgdGNwIGNvbm5l
Y3Rpb24gZm9yIG11bHRpcGxlIHY0DQo+PiBjbGllbnRzIChzb21ldGhpbmcgdGhlIHNwZWMgZXhw
bGljaXRseSBhbGxvd3MpLiAgVGhlIGNhbGxiYWNrIGNsaWVudCBpbg0KPj4gc3VjaCBjYXNlcyBo
YXMgdG8gc2hhcmUgdGhlIHNhbWUgcnBjX3hwcnQuICBUaGUgbmV3IHJwYyBjb2RlLCB3aGVuIGl0
DQo+PiBjcmVhdGVzIHR3byBjYWxsYmFjayBjbGllbnRzIHNoYXJpbmcgdGhlIHNhbWUgcnBjX3hw
cnQsIGVuZHMgdXAgdHJ5aW5nDQo+PiB0byBrZWVwIHRoYXQgcnBjX3hwcnQgb24gdGhlIGxpc3Rz
IG9mIHR3byBkaWZmZXJlbnQgcnBjX3hwcnRfc3dpdGNoJ3MuDQo+PiBUaGUgZmlyc3Qgc3ltcHRv
bSBJIHNlZSBpcyBhIGxpc3QgY29ycnVwdGlvbiB3YXJuaW5nLg0KPj4NCj4+IFNvLCBJJ20gZW5m
b3JjaW5nIHRoZSByZXF1aXJlbWVudCB0aGF0IHRoZXJlIGJlIG9ubHkgb25lIGJhY2tjaGFubmVs
DQo+PiBycGNfeHBydF9zd2l0Y2ggcGVyIHRjcCBjb25uZWN0aW9uIGJ5IGRvaW5nIGFzIHdlIGRv
IGluIHRoZSBycGNfeHBydA0KPj4gY2FzZToga2VlcGluZyBhIHBvaW50ZXIgdG8gaXQgaW4gdGhl
IHN2Y194cHJ0LCBhbmQgdXNpbmcgdGhhdCB3aGVuIGl0J3MNCj4+IGF2YWlsYWJsZSBpbnN0ZWFk
IG9mIGFsbG9jYXRpbmcgYSBuZXcgb25lLg0KPg0KPj4gVGhhdCdzIGEgcHJldHR5IHN0cmFpZ2h0
Zm9yd2FyZCBmaXggKGFuZCBJJ3ZlIHZlcmlmaWVkIGl0IHdvcmtzKSwgYnV0DQo+PiBkb2Vzbid0
IGxvb2sgdmVyeSBlbGVnYW50LiAgSWYgdGhlcmUncyBhIGJldHRlciBzb2x1dGlvbiwgSSdtIGFs
bCBlYXJzLg0KPg0KPkkgaGF2ZW4ndCBjb21lIHVwIHdpdGggYW55dGhpbmcgbmVhdGVyLCBhbmQg
a2luZCBvZiBuZWVkIHRoZSBjcmFzaA0KPmZpeGVkLiAgQUNLIG9yIE5BQ0s/ICBJIGNhbiB0YWtl
IGl0IHRocm91Z2ggbXkgdHJlZSBpZiBpdCdzIGFuIEFDSy4NCj4NCj4tLWIuDQo+DQoNCkFjaw0K
DQo=

Re: [PATCH 0/3] fix backchannel crash due to multipath

[J. Bruce Fields]

On Mon, Jun 13, 2016 at 07:24:06PM +0000, Trond Myklebust wrote:
> 
> 
> On 6/13/16, 14:21, "J. Bruce Fields" <bfields@fieldses.org> wrote:
> 
> >On March 24 J. Bruce Fields said:
> >> pynfs can crash the nfs server since the new multipath code.  The
> >> problem is that pynfs uses the same tcp connection for multiple v4
> >> clients (something the spec explicitly allows).  The callback client in
> >> such cases has to share the same rpc_xprt.  The new rpc code, when it
> >> creates two callback clients sharing the same rpc_xprt, ends up trying
> >> to keep that rpc_xprt on the lists of two different rpc_xprt_switch's.
> >> The first symptom I see is a list corruption warning.
> >>
> >> So, I'm enforcing the requirement that there be only one backchannel
> >> rpc_xprt_switch per tcp connection by doing as we do in the rpc_xprt
> >> case: keeping a pointer to it in the svc_xprt, and using that when it's
> >> available instead of allocating a new one.
> >
> >> That's a pretty straightforward fix (and I've verified it works), but
> >> doesn't look very elegant.  If there's a better solution, I'm all ears.
> >
> >I haven't come up with anything neater, and kind of need the crash
> >fixed.  ACK or NACK?  I can take it through my tree if it's an ACK.
> >
> >--b.
> >
> 
> Ack

Thanks!--b.