[PATCH] svcrdma: backchannel cannot share a page for send and rcv buffers

[PATCH] svcrdma: backchannel cannot share a page for send and rcv buffers

[Chuck Lever]

The underlying transport releases the page pointed to by rq_buffer
during xprt_rdma_bc_send_request. When the backchannel reply arrives,
rq_rbuffer then points to freed memory.

Fixes: 68778945e46f ('SUNRPC: Separate buffer pointers for RPC ...')
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/xprtrdma/svc_rdma_backchannel.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Hi Bruce-

This applies on top of Jeff's recent patch in the same area. It's
an obvious quick fix rather than a deep review of that code path.

I've tested with iozone, git "make test", and some xfstests with
NFSv4.1 / RDMA; I ran into another crasher that is preventing more
extensive testing. The prepare_creds crash has not re-appeared so
far.

I enabled RPC client debugging on the server during these tests to
confirm that the CB_RECALL operations were successful.

diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
index fc4535e..20027f8 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
@@ -177,19 +177,26 @@ static int svc_rdma_bc_sendto(struct svcxprt_rdma *rdma,
 		return -EINVAL;
 	}
 
+	/* svc_rdma_sendto releases this page */
 	page = alloc_page(RPCRDMA_DEF_GFP);
 	if (!page)
 		return -ENOMEM;
-
 	rqst->rq_buffer = page_address(page);
-	rqst->rq_rbuffer = (char *)rqst->rq_buffer + rqst->rq_callsize;
+
+	rqst->rq_rbuffer = kmalloc(rqst->rq_rcvsize, RPCRDMA_DEF_GFP);
+	if (!rqst->rq_rbuffer) {
+		put_page(page);
+		return -ENOMEM;
+	}
 	return 0;
 }
 
 static void
 xprt_rdma_bc_free(struct rpc_task *task)
 {
-	/* No-op: ctxt and page have already been freed. */
+	struct rpc_rqst *rqst = task->tk_rqstp;
+
+	kfree(rqst->rq_rbuffer);
 }
 
 static int

Re: [PATCH] svcrdma: backchannel cannot share a page for send and rcv buffers

[Jeff Layton]

On Fri, 2016-10-28 at 22:22 -0400, Chuck Lever wrote:
> The underlying transport releases the page pointed to by rq_buffer
> during xprt_rdma_bc_send_request. When the backchannel reply arrives,
> rq_rbuffer then points to freed memory.
> 
> Fixes: 68778945e46f ('SUNRPC: Separate buffer pointers for RPC ...')
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/xprtrdma/svc_rdma_backchannel.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> Hi Bruce-
> 
> This applies on top of Jeff's recent patch in the same area. It's
> an obvious quick fix rather than a deep review of that code path.
> 
> I've tested with iozone, git "make test", and some xfstests with
> NFSv4.1 / RDMA; I ran into another crasher that is preventing more
> extensive testing. The prepare_creds crash has not re-appeared so
> far.
> 
> I enabled RPC client debugging on the server during these tests to
> confirm that the CB_RECALL operations were successful.
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
> index fc4535e..20027f8 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_backchannel.c
> @@ -177,19 +177,26 @@ static int svc_rdma_bc_sendto(struct svcxprt_rdma *rdma,
>  		return -EINVAL;
>  	}
>  
> +	/* svc_rdma_sendto releases this page */
>  	page = alloc_page(RPCRDMA_DEF_GFP);
>  	if (!page)
>  		return -ENOMEM;
> -
>  	rqst->rq_buffer = page_address(page);
> -	rqst->rq_rbuffer = (char *)rqst->rq_buffer + rqst->rq_callsize;
> +
> +	rqst->rq_rbuffer = kmalloc(rqst->rq_rcvsize, RPCRDMA_DEF_GFP);
> +	if (!rqst->rq_rbuffer) {
> +		put_page(page);
> +		return -ENOMEM;
> +	}
>  	return 0;
>  }
>  
>  static void
>  xprt_rdma_bc_free(struct rpc_task *task)
>  {
> -	/* No-op: ctxt and page have already been freed. */
> +	struct rpc_rqst *rqst = task->tk_rqstp;
> +
> +	kfree(rqst->rq_rbuffer);
>  }
>  
>  static int
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Obviously, mine was not based on any deep reading of this code either,
so I'll take your word for it on this:

Acked-by: Jeff Layton <jlayton@redhat.com>