SUNRPC: fix a memory leak for tcp NFSv4.1 backchannel

xs_setup_bc_tcp may return an existing xprt with non-NULL servername. xprt_create_transport should not kstrdup servername for it. Otherwise, those memory for servername will be leaked.

Signed-off-by Kinglong Mee <[email protected]>

---
 net/sunrpc/xprt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

net/sunrpc/xprt.c +5 -1

index ddd198e..6fa966f 100644
@@ -1339,7 +1339,11 @@ found:
 		xprt_destroy(xprt);
 		return ERR_PTR(-EINVAL);
 	}
-	xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
+	/* servername may not be NULL for tcp NFSv4.1 backchannel */
+	if (xprt->servername == NULL)
+		xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
 	if (xprt->servername == NULL) {
 		xprt_destroy(xprt);
 		return ERR_PTR(-ENOMEM);

Signature · 1 line

1.8.4.2

Quoted 3 lines · Kinglong Mee

xs_setup_bc_tcp may return an existing xprt with non-NULL servername. xprt_create_transport should not kstrdup servername for it. Otherwise, those memory for servername will be leaked.

OK. Applying to my tree if Trond has no objection.

--b.

Quoted 25 lines

Signed-off-by Kinglong Mee <[email protected]>

---
net/sunrpc/xprt.c | 6 +++++-

1 file changed, 5 insertions(+), 1 deletion(-)

net/sunrpc/xprt.c +5 -1

index ddd198e..6fa966f 100644
@@ -1339,7 +1339,11 @@ found:
 		xprt_destroy(xprt);
 		return ERR_PTR(-EINVAL);
 	}
-	xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
+	/* servername may not be NULL for tcp NFSv4.1 backchannel */
+	if (xprt->servername == NULL)
+		xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
 	if (xprt->servername == NULL) {
 		xprt_destroy(xprt);
 		return ERR_PTR(-ENOMEM);

Signature · 1 line

1.8.4.2

Quoted 5 lines · J. Bruce Fields

Quoted 3 lines · Kinglong Mee

xs_setup_bc_tcp may return an existing xprt with non-NULL servername. xprt_create_transport should not kstrdup servername for it. Otherwise, those memory for servername will be leaked.

Quoted 1 line

OK. Applying to my tree if Trond has no objection.

Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming that is the only case where xprt->servername != NULL, right?

For instance, won’t calling INIT_WORK() be a source of problems?

Quoted 29 lines

--b.

Quoted 25 lines

Signed-off-by Kinglong Mee <[email protected]>

---
net/sunrpc/xprt.c | 6 +++++-

1 file changed, 5 insertions(+), 1 deletion(-)

net/sunrpc/xprt.c +5 -1

index ddd198e..6fa966f 100644
@@ -1339,7 +1339,11 @@ found:
 		xprt_destroy(xprt);
 		return ERR_PTR(-EINVAL);
 	}
-	xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
+	/* servername may not be NULL for tcp NFSv4.1 backchannel */
+	if (xprt->servername == NULL)
+		xprt->servername = kstrdup(args->servername, GFP_KERNEL);
+
 	if (xprt->servername == NULL) {
 		xprt_destroy(xprt);
 		return ERR_PTR(-ENOMEM);

Signature · 1 line

1.8.4.2

Quoted 8 lines · Trond Myklebust

Quoted 5 lines · J. Bruce Fields

Quoted 3 lines · Kinglong Mee

xs_setup_bc_tcp may return an existing xprt with non-NULL servername. xprt_create_transport should not kstrdup servername for it. Otherwise, those memory for servername will be leaked.

Quoted 5 lines

Quoted 1 line

OK. Applying to my tree if Trond has no objection.

Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming that is the only case where xprt->servername != NULL, right?

For instance, won’t calling INIT_WORK() be a source of problems?

Huh. Looking at the history.... There used to be a

	if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state))
		/* ->setup returned a pre-initialized xprt: */
		return xprt;

here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a "SUNRPC: Clean up the slot table allocation", which looks otherwise unrelated. Was that just some kind of rebasing mistake, or was there a reason for that?

Quoted 9 lines · Dr Fields James Bruce

Quoted 8 lines · Trond Myklebust

Quoted 5 lines · J. Bruce Fields

Quoted 3 lines · Kinglong Mee

xs_setup_bc_tcp may return an existing xprt with non-NULL servername. xprt_create_transport should not kstrdup servername for it. Otherwise, those memory for servername will be leaked.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

OK. Applying to my tree if Trond has no objection.

Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming that is the only case where xprt->servername != NULL, right?

For instance, won’t calling INIT_WORK() be a source of problems?

Quoted 11 lines

Huh. Looking at the history.... There used to be a

	if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state))
		/* ->setup returned a pre-initialized xprt: */
		return xprt;

here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a "SUNRPC: Clean up the slot table allocation", which looks otherwise unrelated. Was that just some kind of rebasing mistake, or was there a reason for that?

I probably misunderstood that bc_xprt sends a fully initialized struct rpc_xprt.

The obvious question if that is the case, is why we are calling xprt_create_transport() at all? If .bc_xprt->xpt_bc_xprt contains a fully initialized struct rpc_xprt, then just have rpc_create() do the honors. Better yet, create a svc_create_backchannel_client() helper that calls rpc_new_client() with the correct parameters. I really don’t like those rpc_create_args hacks that introduce fields that are completely private to nfsd.

Quoted 20 lines · Dr Fields James Bruce

Quoted 9 lines · Trond Myklebust

Quoted 5 lines · J. Bruce Fields

Quoted 3 lines · Kinglong Mee

xs_setup_bc_tcp may return an existing xprt with non-NULL servername. xprt_create_transport should not kstrdup servername for it. Otherwise, those memory for servername will be leaked.

Quoted 7 lines

Quoted 1 line

OK. Applying to my tree if Trond has no objection.

Actually. Why do we go through all this code at all if xs_setup_bc_tcp() returns args->bc_xprt->xpt_bc_xprt? I’m assuming that is the only case where xprt->servername != NULL, right?

For instance, won’t calling INIT_WORK() be a source of problems?

Huh. Looking at the history.... There used to be a

Quoted 7 lines

	if (test_and_set_bit(XPRT_INITIALIZED, &xprt->state)) /* ->setup
	returned a pre-initialized xprt: */ return xprt;

here, but it got removed by 21de0a955f3af29fa1100d96f66e6adade89e77a "SUNRPC: Clean up the slot table allocation", which looks otherwise unrelated. Was that just some kind of rebasing mistake, or was there a reason for that?

I probably misunderstood that bc_xprt sends a fully initialized struct rpc_xprt.

The obvious question if that is the case, is why we are calling xprt_create_transport() at all? If .bc_xprt->xpt_bc_xprt contains a fully initialized struct rpc_xprt, then just have rpc_create() do the honors. Better yet, create a svc_create_backchannel_client() helper that calls rpc_new_client() with the correct parameters.

I really
don’t like those rpc_create_args hacks that introduce fields that are completely private to nfsd.

Besides checking rpc_xprt out of xs_setup_bc_tcp,
increase it's reference (it's important).

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4callback.c      | 19 ++++++++++++++++++-
 include/linux/sunrpc/xprt.h | 13 ++++++++++++-
 net/sunrpc/xprt.c           | 12 ------------
 net/sunrpc/xprtsock.c       |  9 ---------
 4 files changed, 30 insertions(+), 23 deletions(-)

fs/nfsd/nfs4callback.c +18 -1

index 7f05cd1..39c8ef8 100644
@@ -32,6 +32,7 @@
  */
 
 #include <linux/sunrpc/clnt.h>
+#include <linux/sunrpc/xprt.h>
 #include <linux/sunrpc/svc_xprt.h>
 #include <linux/slab.h>
 #include "nfsd.h"

@@ -635,6 +636,22 @@ 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)
 {
 	struct rpc_timeout	timeparms = {

@@ -674,7 +691,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 = rpc_create(&args);
+	client = create_backchannel_client(&args);
 	if (IS_ERR(client)) {
 		dprintk("NFSD: couldn't create callback client: %ld\n",
 			PTR_ERR(client));

include/linux/sunrpc/xprt.h +12 -1

index 8097b9d..3e5efb2 100644
@@ -295,13 +295,24 @@ int			xprt_adjust_timeout(struct rpc_rqst *req);
 void			xprt_release_xprt(struct rpc_xprt *xprt, struct rpc_task *task);
 void			xprt_release_xprt_cong(struct rpc_xprt *xprt, struct rpc_task *task);
 void			xprt_release(struct rpc_task *task);
-struct rpc_xprt *	xprt_get(struct rpc_xprt *xprt);
 void			xprt_put(struct rpc_xprt *xprt);
 struct rpc_xprt *	xprt_alloc(struct net *net, size_t size,
 				unsigned int num_prealloc,
 				unsigned int max_req);
 void			xprt_free(struct rpc_xprt *);
 
+/**
+ * xprt_get - return a reference to an RPC transport.
+ * @xprt: pointer to the transport
+ *
+ */
+static inline struct rpc_xprt *xprt_get(struct rpc_xprt *xprt)
+{
+	if (atomic_inc_not_zero(&xprt->count))
+		return xprt;
+	return NULL;
+}
+
 static inline __be32 *xprt_skip_transport_header(struct rpc_xprt *xprt, __be32 *p)
 {
 	return p + xprt->tsh_size;

net/sunrpc/xprt.c +0 -12

index ddd198e..b0a1bbb 100644
@@ -1383,15 +1383,3 @@ void xprt_put(struct rpc_xprt *xprt)
 	if (atomic_dec_and_test(&xprt->count))
 		xprt_destroy(xprt);
 }
-
-/**
- * xprt_get - return a reference to an RPC transport.
- * @xprt: pointer to the transport
- *
- */
-struct rpc_xprt *xprt_get(struct rpc_xprt *xprt)
-{
-	if (atomic_inc_not_zero(&xprt->count))
-		return xprt;
-	return NULL;
-}

net/sunrpc/xprtsock.c +0 -9

index 7289e3c..37ea15a 100644
@@ -2923,15 +2923,6 @@ static struct rpc_xprt *xs_setup_bc_tcp(struct xprt_create *args)
 	struct svc_sock *bc_sock;
 	struct rpc_xprt *ret;
 
-	if (args->bc_xprt->xpt_bc_xprt) {
-		/*
-		 * This server connection already has a backchannel
-		 * transport; we can't create a new one, as we wouldn't
-		 * be able to match replies based on xid any more.  So,
-		 * reuse the already-existing one:
-		 */
-		 return args->bc_xprt->xpt_bc_xprt;
-	}
 	xprt = xs_setup_xprt(args, xprt_tcp_slot_table_entries,
 			xprt_tcp_slot_table_entries);
 	if (IS_ERR(xprt))

Signature · 1 line

1.8.4.2

Quoted 2 lines · Kinglong Mee

Besides checking rpc_xprt out of xs_setup_bc_tcp,
increase it's reference (it's important).

This sounds wrong to me: the presence of a backchannel can't prevent the client's connection from going away. Instead, when the connection dies, any associated backchannels should be immediately destroyed.

Quoted 4 lines · Dr Fields James Bruce

Quoted 2 lines · Kinglong Mee

Besides checking rpc_xprt out of xs_setup_bc_tcp,
increase it's reference (it's important).

Quoted 3 lines

This sounds wrong to me: the presence of a backchannel can't prevent the client's connection from going away. Instead, when the connection dies, any associated backchannels should be immediately destroyed.

Hi Bruce,

The right way to deal with this is to have knfsd shut down the rpc_client when it detects the TCP disconnection event.

The xprt->count shouldn’t be an issue here:
it has nothing to do with the socket connection state.

Quoted 7 lines · Trond Myklebust

Quoted 4 lines · Dr Fields James Bruce

Quoted 2 lines · Kinglong Mee

Besides checking rpc_xprt out of xs_setup_bc_tcp,
increase it's reference (it's important).

Quoted 8 lines

Quoted 3 lines

This sounds wrong to me: the presence of a backchannel can't prevent the client's connection from going away. Instead, when the connection dies, any associated backchannels should be immediately destroyed.

Hi Bruce,

The right way to deal with this is to have knfsd shut down the rpc_client when it detects the TCP disconnection event.

Yes, that's right.
Knfsd has do it as you said.

When getting xprt's status of XPT_CLOSE in svc_recv/svc_handle_xprt, knfsd will delete the xprt by svc_delete_xprt.

In svc_delete_xprt, knfsd calls call_xpt_users to notify those users of the xprt. So, nfsd4_conn_lost will be called to free the connection.
After freeing connection, nfsd4_probe_callback will update the callback for the client. And, the clp->cl_cb_client will be shutdown in nfsd4_process_cb_update.

At last, all using of the xprt will be released.
I have test it, that's OK.

Quoted 2 lines

The xprt->count shouldn’t be an issue here:
it has nothing to do with the socket connection state.

The xprt of backchannel, there are two references,
1. rpc_clnt
2. svc_xprt

For rpc_clnt, initialize the xprt->count in xs_setup_bc_tcp,
or increase it in create_backchannel_client, and, decrease in rpc_free_client.

For svc_xprt, increase it in xs_setup_bc_tcp, and decrease in svc_xprt_free.

thanks,
Kinglong Mee

Apologies, I completely dropped this and now I've lost the thread of the discussion.

What is your most recent patchset?

--b.

Quoted 46 lines · Kinglong Mee

Quoted 7 lines · Trond Myklebust

Quoted 4 lines · Dr Fields James Bruce

Quoted 2 lines · Kinglong Mee

Besides checking rpc_xprt out of xs_setup_bc_tcp,
increase it's reference (it's important).

Quoted 8 lines

Quoted 3 lines

This sounds wrong to me: the presence of a backchannel can't prevent the client's connection from going away. Instead, when the connection dies, any associated backchannels should be immediately destroyed.

Hi Bruce,

The right way to deal with this is to have knfsd shut down the rpc_client when it detects the TCP disconnection event.

Yes, that's right.
Knfsd has do it as you said.

When getting xprt's status of XPT_CLOSE in svc_recv/svc_handle_xprt, knfsd will delete the xprt by svc_delete_xprt.

In svc_delete_xprt, knfsd calls call_xpt_users to notify those users of the xprt. So, nfsd4_conn_lost will be called to free the connection.
After freeing connection, nfsd4_probe_callback will update the callback for the client. And, the clp->cl_cb_client will be shutdown in nfsd4_process_cb_update.

At last, all using of the xprt will be released.
I have test it, that's OK.

Quoted 2 lines

The xprt->count shouldn't be an issue here:
it has nothing to do with the socket connection state.

The xprt of backchannel, there are two references,
1. rpc_clnt
2. svc_xprt

For rpc_clnt, initialize the xprt->count in xs_setup_bc_tcp,
or increase it in create_backchannel_client, and, decrease in rpc_free_client.

For svc_xprt, increase it in xs_setup_bc_tcp, and decrease in svc_xprt_free.

thanks,
Kinglong Mee