[v1,05/10] svcrdma: Find rmsgp more reliably
diff mbox

Message ID 20150109192237.4901.92644.stgit@klimt.1015granger.net
State New, archived
Headers show

Commit Message

Chuck Lever Jan. 9, 2015, 7:22 p.m. UTC
xdr_start() can return the wrong rmsgp address if an assumption
about how the xdr_buf was constructed changes.  When it gets it
wrong, the client receives a reply that has gibberish in the
RPC/RDMA header, preventing it from matching a waiting RPC request.

Instead, make (and document) just one assumption: that the RDMA
header for the client's RPC call is at the start of the first page
in rq_pages.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 net/sunrpc/xprtrdma/svc_rdma_sendto.c |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)


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

Comments

Sagi Grimberg Jan. 11, 2015, 5:37 p.m. UTC | #1
On 1/9/2015 9:22 PM, Chuck Lever wrote:
> xdr_start() can return the wrong rmsgp address if an assumption
> about how the xdr_buf was constructed changes.  When it gets it
> wrong, the client receives a reply that has gibberish in the
> RPC/RDMA header, preventing it from matching a waiting RPC request.
>
> Instead, make (and document) just one assumption: that the RDMA
> header for the client's RPC call is at the start of the first page
> in rq_pages.

Would it make more sense to add another pointer assigned at req
initialization (maybe in the RDMA request context) instead of hard
coding this assumption? I may be completely wrong here though...

>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>
>   net/sunrpc/xprtrdma/svc_rdma_sendto.c |   18 ++++--------------
>   1 files changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> index 7d79897..7de33d1 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> @@ -483,18 +483,6 @@ void svc_rdma_prep_reply_hdr(struct svc_rqst *rqstp)
>   {
>   }
>
> -/*
> - * Return the start of an xdr buffer.
> - */
> -static void *xdr_start(struct xdr_buf *xdr)
> -{
> -	return xdr->head[0].iov_base -
> -		(xdr->len -
> -		 xdr->page_len -
> -		 xdr->tail[0].iov_len -
> -		 xdr->head[0].iov_len);
> -}
> -
>   int svc_rdma_sendto(struct svc_rqst *rqstp)
>   {
>   	struct svc_xprt *xprt = rqstp->rq_xprt;
> @@ -512,8 +500,10 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
>
>   	dprintk("svcrdma: sending response for rqstp=%p\n", rqstp);
>
> -	/* Get the RDMA request header. */
> -	rdma_argp = xdr_start(&rqstp->rq_arg);
> +	/* Get the RDMA request header. The receive logic always
> +	 * places this at the start of page 0.
> +	 */
> +	rdma_argp = page_address(rqstp->rq_pages[0]);
>
>   	/* Build an req vec for the XDR */
>   	ctxt = svc_rdma_get_context(rdma);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
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
Chuck Lever Jan. 12, 2015, 12:30 a.m. UTC | #2
Hi Sagi-

Thanks for the review.

On Jan 11, 2015, at 12:37 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:

> On 1/9/2015 9:22 PM, Chuck Lever wrote:
>> xdr_start() can return the wrong rmsgp address if an assumption
>> about how the xdr_buf was constructed changes.  When it gets it
>> wrong, the client receives a reply that has gibberish in the
>> RPC/RDMA header, preventing it from matching a waiting RPC request.
>> 
>> Instead, make (and document) just one assumption: that the RDMA
>> header for the client's RPC call is at the start of the first page
>> in rq_pages.
> 
> Would it make more sense to add another pointer assigned at req
> initialization (maybe in the RDMA request context) instead of hard
> coding this assumption? I may be completely wrong here though...

I considered this. I couldn’t find an appropriate place to add
such a pointer.

I think that’s why xdr_start() was there in the first place: there
is no convenient place to save a pointer to the request’s RDMA
header.

Bruce might have other thoughts about this.

>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>>  net/sunrpc/xprtrdma/svc_rdma_sendto.c |   18 ++++--------------
>>  1 files changed, 4 insertions(+), 14 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
>> index 7d79897..7de33d1 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
>> @@ -483,18 +483,6 @@ void svc_rdma_prep_reply_hdr(struct svc_rqst *rqstp)
>>  {
>>  }
>> 
>> -/*
>> - * Return the start of an xdr buffer.
>> - */
>> -static void *xdr_start(struct xdr_buf *xdr)
>> -{
>> -	return xdr->head[0].iov_base -
>> -		(xdr->len -
>> -		 xdr->page_len -
>> -		 xdr->tail[0].iov_len -
>> -		 xdr->head[0].iov_len);
>> -}
>> -
>>  int svc_rdma_sendto(struct svc_rqst *rqstp)
>>  {
>>  	struct svc_xprt *xprt = rqstp->rq_xprt;
>> @@ -512,8 +500,10 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
>> 
>>  	dprintk("svcrdma: sending response for rqstp=%p\n", rqstp);
>> 
>> -	/* Get the RDMA request header. */
>> -	rdma_argp = xdr_start(&rqstp->rq_arg);
>> +	/* Get the RDMA request header. The receive logic always
>> +	 * places this at the start of page 0.
>> +	 */
>> +	rdma_argp = page_address(rqstp->rq_pages[0]);
>> 
>>  	/* Build an req vec for the XDR */
>>  	ctxt = svc_rdma_get_context(rdma);
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
> 
> --
> 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

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com



--
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
Sagi Grimberg Jan. 13, 2015, 10:07 a.m. UTC | #3
On 1/12/2015 2:30 AM, Chuck Lever wrote:
> Hi Sagi-
>
> Thanks for the review.
>
> On Jan 11, 2015, at 12:37 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>
>> On 1/9/2015 9:22 PM, Chuck Lever wrote:
>>> xdr_start() can return the wrong rmsgp address if an assumption
>>> about how the xdr_buf was constructed changes.  When it gets it
>>> wrong, the client receives a reply that has gibberish in the
>>> RPC/RDMA header, preventing it from matching a waiting RPC request.
>>>
>>> Instead, make (and document) just one assumption: that the RDMA
>>> header for the client's RPC call is at the start of the first page
>>> in rq_pages.
>>
>> Would it make more sense to add another pointer assigned at req
>> initialization (maybe in the RDMA request context) instead of hard
>> coding this assumption? I may be completely wrong here though...
>
> I considered this. I couldn’t find an appropriate place to add
> such a pointer.
>
> I think that’s why xdr_start() was there in the first place: there
> is no convenient place to save a pointer to the request’s RDMA
> header.
>
> Bruce might have other thoughts about this.

Yep, I didn't find any nice place to put that also, thought you might
have an idea...

Sagi.
--
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

Patch
diff mbox

diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
index 7d79897..7de33d1 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
@@ -483,18 +483,6 @@  void svc_rdma_prep_reply_hdr(struct svc_rqst *rqstp)
 {
 }
 
-/*
- * Return the start of an xdr buffer.
- */
-static void *xdr_start(struct xdr_buf *xdr)
-{
-	return xdr->head[0].iov_base -
-		(xdr->len -
-		 xdr->page_len -
-		 xdr->tail[0].iov_len -
-		 xdr->head[0].iov_len);
-}
-
 int svc_rdma_sendto(struct svc_rqst *rqstp)
 {
 	struct svc_xprt *xprt = rqstp->rq_xprt;
@@ -512,8 +500,10 @@  int svc_rdma_sendto(struct svc_rqst *rqstp)
 
 	dprintk("svcrdma: sending response for rqstp=%p\n", rqstp);
 
-	/* Get the RDMA request header. */
-	rdma_argp = xdr_start(&rqstp->rq_arg);
+	/* Get the RDMA request header. The receive logic always
+	 * places this at the start of page 0.
+	 */
+	rdma_argp = page_address(rqstp->rq_pages[0]);
 
 	/* Build an req vec for the XDR */
 	ctxt = svc_rdma_get_context(rdma);