[PATCH 1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding

[PATCH 1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding

[Trond Myklebust]

If there are TCP back channel requests either being processed by the
server threads, then we should hold a reference to the transport
to ensure it doesn't get freed from underneath us.

Reported-by: Neil Brown <neilb@suse.de>
Fixes: 2ea24497a1b3 ("SUNRPC: RPC callbacks may be split across several..")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/backchannel_rqst.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 339e8c077c2d..7eb251372f94 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -307,8 +307,8 @@ void xprt_free_bc_rqst(struct rpc_rqst *req)
 		 */
 		dprintk("RPC:       Last session removed req=%p\n", req);
 		xprt_free_allocation(req);
-		return;
 	}
+	xprt_put(xprt);
 }
 
 /*
@@ -339,7 +339,7 @@ struct rpc_rqst *xprt_lookup_bc_request(struct rpc_xprt *xprt, __be32 xid)
 		spin_unlock(&xprt->bc_pa_lock);
 		if (new) {
 			if (req != new)
-				xprt_free_bc_rqst(new);
+				xprt_free_allocation(new);
 			break;
 		} else if (req)
 			break;
@@ -368,6 +368,7 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
 	set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
 
 	dprintk("RPC:       add callback request to list\n");
+	xprt_get(xprt);
 	spin_lock(&bc_serv->sv_cb_lock);
 	list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
 	wake_up(&bc_serv->sv_cb_waitq);
-- 
2.21.0

[PATCH 2/3] SUNRPC: The RDMA back channel mustn't disappear while requests are outstanding

[Trond Myklebust]

If there are RDMA back channel requests either being processed by the
server threads, then we should hold a reference to the transport
to ensure it doesn't get freed from underneath us.

Reported-by: Neil Brown <neilb@suse.de>
Fixes: 63cae47005af ("xprtrdma: Handle incoming backward direction RPC calls")
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprtrdma/backchannel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/sunrpc/xprtrdma/backchannel.c b/net/sunrpc/xprtrdma/backchannel.c
index 50e075fcdd8f..b458bf53ca69 100644
--- a/net/sunrpc/xprtrdma/backchannel.c
+++ b/net/sunrpc/xprtrdma/backchannel.c
@@ -163,6 +163,7 @@ void xprt_rdma_bc_free_rqst(struct rpc_rqst *rqst)
 	spin_lock(&xprt->bc_pa_lock);
 	list_add_tail(&rqst->rq_bc_pa_list, &xprt->bc_pa_list);
 	spin_unlock(&xprt->bc_pa_lock);
+	xprt_put(xprt);
 }
 
 static struct rpc_rqst *rpcrdma_bc_rqst_get(struct rpcrdma_xprt *r_xprt)
@@ -259,6 +260,7 @@ void rpcrdma_bc_receive_call(struct rpcrdma_xprt *r_xprt,
 
 	/* Queue rqst for ULP's callback service */
 	bc_serv = xprt->bc_serv;
+	xprt_get(xprt);
 	spin_lock(&bc_serv->sv_cb_lock);
 	list_add(&rqst->rq_bc_list, &bc_serv->sv_cb_list);
 	spin_unlock(&bc_serv->sv_cb_lock);
-- 
2.21.0

[PATCH 3/3] SUNRPC: Destroy the back channel when we destroy the host transport

[Trond Myklebust]

When we're destroying the host transport mechanism, we should ensure
that we do not leak memory by failing to release any back channel
slots that might still exist.

Reported-by: Neil Brown <neilb@suse.de>
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 net/sunrpc/xprt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
index 8a45b3ccc313..41df4c507193 100644
--- a/net/sunrpc/xprt.c
+++ b/net/sunrpc/xprt.c
@@ -1942,6 +1942,11 @@ static void xprt_destroy_cb(struct work_struct *work)
 	rpc_destroy_wait_queue(&xprt->sending);
 	rpc_destroy_wait_queue(&xprt->backlog);
 	kfree(xprt->servername);
+	/*
+	 * Destroy any existing back channel
+	 */
+	xprt_destroy_backchannel(xprt, UINT_MAX);
+
 	/*
 	 * Tear down transport state and free the rpc_xprt
 	 */
-- 
2.21.0

Re: [PATCH 1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding

[J. Bruce Fields]

These make sense to me.  (But I'll confess I haven't been following the
back and forth with Neil.)

On Wed, Oct 16, 2019 at 10:15:44AM -0400, Trond Myklebust wrote:
> If there are TCP back channel requests either being processed by the

In this patch and the next, is the "either" unnecessary, or was there
some other condition you meant to mention?

--b.

> server threads, then we should hold a reference to the transport
> to ensure it doesn't get freed from underneath us.
> 
> Reported-by: Neil Brown <neilb@suse.de>
> Fixes: 2ea24497a1b3 ("SUNRPC: RPC callbacks may be split across several..")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/backchannel_rqst.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
> index 339e8c077c2d..7eb251372f94 100644
> --- a/net/sunrpc/backchannel_rqst.c
> +++ b/net/sunrpc/backchannel_rqst.c
> @@ -307,8 +307,8 @@ void xprt_free_bc_rqst(struct rpc_rqst *req)
>  		 */
>  		dprintk("RPC:       Last session removed req=%p\n", req);
>  		xprt_free_allocation(req);
> -		return;
>  	}
> +	xprt_put(xprt);
>  }
>  
>  /*
> @@ -339,7 +339,7 @@ struct rpc_rqst *xprt_lookup_bc_request(struct rpc_xprt *xprt, __be32 xid)
>  		spin_unlock(&xprt->bc_pa_lock);
>  		if (new) {
>  			if (req != new)
> -				xprt_free_bc_rqst(new);
> +				xprt_free_allocation(new);
>  			break;
>  		} else if (req)
>  			break;
> @@ -368,6 +368,7 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
>  	set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
>  
>  	dprintk("RPC:       add callback request to list\n");
> +	xprt_get(xprt);
>  	spin_lock(&bc_serv->sv_cb_lock);
>  	list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
>  	wake_up(&bc_serv->sv_cb_waitq);
> -- 
> 2.21.0
> 

Re: [PATCH 3/3] SUNRPC: Destroy the back channel when we destroy the host transport

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

On Wed, Oct 16 2019, Trond Myklebust wrote:

> When we're destroying the host transport mechanism, we should ensure
> that we do not leak memory by failing to release any back channel
> slots that might still exist.
>
> Reported-by: Neil Brown <neilb@suse.de>
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/xprt.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index 8a45b3ccc313..41df4c507193 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1942,6 +1942,11 @@ static void xprt_destroy_cb(struct work_struct *work)
>  	rpc_destroy_wait_queue(&xprt->sending);
>  	rpc_destroy_wait_queue(&xprt->backlog);
>  	kfree(xprt->servername);
> +	/*
> +	 * Destroy any existing back channel
> +	 */
> +	xprt_destroy_backchannel(xprt, UINT_MAX);
> +

This will cause xprt->bc_alloc_max to become meaningless.
That isn't really a problem as the xprt is about to be freed, but it
still seems a little untidy - fragile maybe.
How about another line in the comment:

   * Note: this corrupts ->bc_alloc_max, but it is too late for that to
   * matter.
??

Also, possibly add
 WARN_ON(atomic_read(&xprt->bc_slot_count);
either before or after the xprt_destroy_backchannel - because there
really shouldn't be any requests by this stage.

Thanks,
NeilBrown


>  	/*
>  	 * Tear down transport state and free the rpc_xprt
>  	 */
> -- 
> 2.21.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On Wed, Oct 16 2019, Trond Myklebust wrote:

> If there are TCP back channel requests either being processed by the
> server threads, then we should hold a reference to the transport
> to ensure it doesn't get freed from underneath us.
>
> Reported-by: Neil Brown <neilb@suse.de>
> Fixes: 2ea24497a1b3 ("SUNRPC: RPC callbacks may be split across several..")
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  net/sunrpc/backchannel_rqst.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
> index 339e8c077c2d..7eb251372f94 100644
> --- a/net/sunrpc/backchannel_rqst.c
> +++ b/net/sunrpc/backchannel_rqst.c
> @@ -307,8 +307,8 @@ void xprt_free_bc_rqst(struct rpc_rqst *req)
>  		 */
>  		dprintk("RPC:       Last session removed req=%p\n", req);
>  		xprt_free_allocation(req);
> -		return;
>  	}
> +	xprt_put(xprt);
>  }
>  
>  /*
> @@ -339,7 +339,7 @@ struct rpc_rqst *xprt_lookup_bc_request(struct rpc_xprt *xprt, __be32 xid)
>  		spin_unlock(&xprt->bc_pa_lock);
>  		if (new) {
>  			if (req != new)
> -				xprt_free_bc_rqst(new);
> +				xprt_free_allocation(new);
>  			break;
>  		} else if (req)
>  			break;
> @@ -368,6 +368,7 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied)
>  	set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
>  
>  	dprintk("RPC:       add callback request to list\n");
> +	xprt_get(xprt);
>  	spin_lock(&bc_serv->sv_cb_lock);
>  	list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
>  	wake_up(&bc_serv->sv_cb_waitq);
> -- 
> 2.21.0

Looks good.
This and the next two:
 Reviewed-by: NeilBrown <neilb@suse.de>

It would help me if you could add a Fixes: tag to at least the first
two.

BTW, while reviewing I notices that bc_alloc_count and bc_slot_count are
almost identical.  The three places were that are changed separately are
probably (minor) bugs.
Do you recall why there were two different counters?  Has the reason
disappeared?

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] SUNRPC: The TCP back channel mustn't disappear while requests are outstanding

[Trond Myklebust]

On Thu, 2019-10-17 at 09:24 +1100, NeilBrown wrote:
> On Wed, Oct 16 2019, Trond Myklebust wrote:
> 
> > If there are TCP back channel requests either being processed by
> > the
> > server threads, then we should hold a reference to the transport
> > to ensure it doesn't get freed from underneath us.
> > 
> > Reported-by: Neil Brown <neilb@suse.de>
> > Fixes: 2ea24497a1b3 ("SUNRPC: RPC callbacks may be split across
> > several..")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  net/sunrpc/backchannel_rqst.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/sunrpc/backchannel_rqst.c
> > b/net/sunrpc/backchannel_rqst.c
> > index 339e8c077c2d..7eb251372f94 100644
> > --- a/net/sunrpc/backchannel_rqst.c
> > +++ b/net/sunrpc/backchannel_rqst.c
> > @@ -307,8 +307,8 @@ void xprt_free_bc_rqst(struct rpc_rqst *req)
> >  		 */
> >  		dprintk("RPC:       Last session removed req=%p\n",
> > req);
> >  		xprt_free_allocation(req);
> > -		return;
> >  	}
> > +	xprt_put(xprt);
> >  }
> >  
> >  /*
> > @@ -339,7 +339,7 @@ struct rpc_rqst *xprt_lookup_bc_request(struct
> > rpc_xprt *xprt, __be32 xid)
> >  		spin_unlock(&xprt->bc_pa_lock);
> >  		if (new) {
> >  			if (req != new)
> > -				xprt_free_bc_rqst(new);
> > +				xprt_free_allocation(new);
> >  			break;
> >  		} else if (req)
> >  			break;
> > @@ -368,6 +368,7 @@ void xprt_complete_bc_request(struct rpc_rqst
> > *req, uint32_t copied)
> >  	set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
> >  
> >  	dprintk("RPC:       add callback request to list\n");
> > +	xprt_get(xprt);
> >  	spin_lock(&bc_serv->sv_cb_lock);
> >  	list_add(&req->rq_bc_list, &bc_serv->sv_cb_list);
> >  	wake_up(&bc_serv->sv_cb_waitq);
> > -- 
> > 2.21.0
> 
> Looks good.
> This and the next two:
>  Reviewed-by: NeilBrown <neilb@suse.de>
> 
> It would help me if you could add a Fixes: tag to at least the first
> two.

Neither have Cc:stable, but they both already have Fixes: tags. See
above.

> 
> BTW, while reviewing I notices that bc_alloc_count and bc_slot_count
> are
> almost identical.  The three places were that are changed separately
> are
> probably (minor) bugs.
> Do you recall why there were two different counters?  Has the reason
> disappeared?

IIRC, the former contains the count of preallocated slots, and the
latter the count of preallocated+dynamic slots.

Thanks
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 3/3] SUNRPC: Destroy the back channel when we destroy the host transport

[Trond Myklebust]

On Thu, 2019-10-17 at 09:08 +1100, NeilBrown wrote:
> On Wed, Oct 16 2019, Trond Myklebust wrote:
> 
> > When we're destroying the host transport mechanism, we should
> > ensure
> > that we do not leak memory by failing to release any back channel
> > slots that might still exist.
> > 
> > Reported-by: Neil Brown <neilb@suse.de>
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  net/sunrpc/xprt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > index 8a45b3ccc313..41df4c507193 100644
> > --- a/net/sunrpc/xprt.c
> > +++ b/net/sunrpc/xprt.c
> > @@ -1942,6 +1942,11 @@ static void xprt_destroy_cb(struct
> > work_struct *work)
> >  	rpc_destroy_wait_queue(&xprt->sending);
> >  	rpc_destroy_wait_queue(&xprt->backlog);
> >  	kfree(xprt->servername);
> > +	/*
> > +	 * Destroy any existing back channel
> > +	 */
> > +	xprt_destroy_backchannel(xprt, UINT_MAX);
> > +
> 
> This will cause xprt->bc_alloc_max to become meaningless.

Fixed in v2.

> That isn't really a problem as the xprt is about to be freed, but it
> still seems a little untidy - fragile maybe.
> How about another line in the comment:
> 
>    * Note: this corrupts ->bc_alloc_max, but it is too late for that
> to
>    * matter.
> ??
> 
> Also, possibly add
>  WARN_ON(atomic_read(&xprt->bc_slot_count);
> either before or after the xprt_destroy_backchannel - because there
> really shouldn't be any requests by this stage.

I considered it, but since RDMA doesn't use those variable, it wouldn't
really be a sufficient check.

Thanks
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com