Re: [PATCH] svcrdma: Fix NFS server crash triggered by 1MB NFS WRITE

[J. Bruce Fields]

On Mon, Oct 12, 2015 at 10:53:39AM -0400, Chuck Lever wrote:
> Now that the NFS server advertises a maximum payload size of 1MB
> for RPC/RDMA again, it crashes in svc_process_common() when NFS
> client sends a 1MB NFS WRITE on an NFS/RDMA mount.
> 
> The server has set up a 259 element array of struct page pointers
> in rq_pages[] for each incoming request. The last element of the
> array is NULL.
> 
> When an incoming request has been completely received,
> rdma_read_complete() attempts to set the starting page of the
> incoming page vector:
> 
>   rqstp->rq_arg.pages = >rq_pages[head->hdr_count];
> 
> and the page to use for the reply:
> 
>   rqstp->rq_respages = >rq_arg.pages[page_no];
> 
> But the value of page_no has already accounted for head->hdr_count.
> Thus rq_respages now points past the end of the incoming pages.
> 
> For NFS WRITE operations smaller than the maximum, this is harmless.
> But when the NFS WRITE operation is as large as the server's max
> payload size, rq_respages now points at the last entry in rq_pages,
> which is NULL.
> 
> Fixes: cc9a903d915c ('svcrdma: Change maximum server payload . . .')
> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=270
> Signed-off-by: Chuck Lever 
> Reviewed-by: Sagi Grimberg 
> Reviewed-by: Steve Wise 
> Reviewed-by: Shirley Ma 
> ---
> 
> Hi Bruce-
> 
> This is a regression in 4.3. Can you send this to Linus?

OK, queuing for 4.3, thanks.--b.

> 
> 
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c 
> b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index cb51742..37b4341 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -531,7 +531,7 @@ static int rdma_read_complete(struct svc_rqst *rqstp,
>   rqstp->rq_arg.page_base = head->arg.page_base;
>  
>   /* rq_respages starts after the last arg page */
> - rqstp->rq_respages = >rq_arg.pages[page_no];
> + rqstp->rq_respages = >rq_pages[page_no];
>   rqstp->rq_next_page = rqstp->rq_respages + 1;
>  
>   /* Rebuild rq_arg head and tail. */
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] svcrdma: Fix NFS server crash triggered by 1MB NFS WRITE

[Steve Wise]

> -Original Message-
> From: linux-nfs-ow...@vger.kernel.org 
> [mailto:linux-nfs-ow...@vger.kernel.org] On Behalf Of Chuck Lever
> Sent: Sunday, October 04, 2015 10:03 PM
> To: linux-...@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: [PATCH] svcrdma: Fix NFS server crash triggered by 1MB NFS WRITE
> 
> Now that the NFS server advertises a maximum payload size of 1MB
> for RPC/RDMA again, it crashes in svc_process_common() when NFS
> client sends a 1MB NFS WRITE on an NFS/RDMA mount.
> 
> The server has set up a 259 element array of struct page pointers
> in rq_pages[] for each incoming request. The last element of the
> array is NULL.
> 
> When an incoming request has been completely received,
> rdma_read_complete() attempts to set the starting page of the
> incoming page vector:
> 
>   rqstp->rq_arg.pages = >rq_pages[head->hdr_count];
> 
> and the page to use for the reply:
> 
>   rqstp->rq_respages = >rq_arg.pages[page_no];
> 
> But the value of page_no has already accounted for head->hdr_count.
> Thus rq_respages now points past the end of the incoming pages. For
> NFS WRITE operations smaller than the maximum, this is harmless.
> 
> But when the NFS WRITE operation is as large as the server's max
> payload size, rq_respages now points at the last entry in rq_pages,
> which is NULL.
> 
> Fixes: cc9a903d915c ('svcrdma: Change maximum server payload . . .')
> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=270
> Signed-off-by: Chuck Lever 
> ---
> 
> This fixes a 4.3-rc regression. Please apply to 4.3-rc when this
> patch passes review.
> 
> It could also be appropriate for stable kernels which do not have
> commit 7e5be28827bf ("svcrdma: advertise the correct max payload"),
> though I have not tested them with this patch.
> 
>  net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c 
> b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index cb51742..37b4341 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -531,7 +531,7 @@ static int rdma_read_complete(struct svc_rqst *rqstp,
>   rqstp->rq_arg.page_base = head->arg.page_base;
> 
>   /* rq_respages starts after the last arg page */
> - rqstp->rq_respages = >rq_arg.pages[page_no];
> + rqstp->rq_respages = >rq_pages[page_no];
>   rqstp->rq_next_page = rqstp->rq_respages + 1;
> 
>   /* Rebuild rq_arg head and tail. */
> 

Reviewed-by: Steve Wise 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] svcrdma: Fix NFS server crash triggered by 1MB NFS WRITE

[Chuck Lever]

> On Oct 5, 2015, at 3:42 PM, Steve Wise  wrote:
> 
> 
> 
>> -Original Message-
>> From: linux-nfs-ow...@vger.kernel.org 
>> [mailto:linux-nfs-ow...@vger.kernel.org] On Behalf Of Chuck Lever
>> Sent: Sunday, October 04, 2015 10:03 PM
>> To: linux-...@vger.kernel.org; linux-rdma@vger.kernel.org
>> Subject: [PATCH] svcrdma: Fix NFS server crash triggered by 1MB NFS WRITE
>> 
>> Now that the NFS server advertises a maximum payload size of 1MB
>> for RPC/RDMA again, it crashes in svc_process_common() when NFS
>> client sends a 1MB NFS WRITE on an NFS/RDMA mount.
>> 
>> The server has set up a 259 element array of struct page pointers
>> in rq_pages[] for each incoming request. The last element of the
>> array is NULL.
>> 
>> When an incoming request has been completely received,
>> rdma_read_complete() attempts to set the starting page of the
>> incoming page vector:
>> 
>>  rqstp->rq_arg.pages = >rq_pages[head->hdr_count];
>> 
>> and the page to use for the reply:
>> 
>>  rqstp->rq_respages = >rq_arg.pages[page_no];
>> 
>> But the value of page_no has already accounted for head->hdr_count.
>> Thus rq_respages now points past the end of the incoming pages. For
>> NFS WRITE operations smaller than the maximum, this is harmless.
>> 
>> But when the NFS WRITE operation is as large as the server's max
>> payload size, rq_respages now points at the last entry in rq_pages,
>> which is NULL.
>> 
>> Fixes: cc9a903d915c ('svcrdma: Change maximum server payload . . .')
>> BugLink: https://bugzilla.linux-nfs.org/show_bug.cgi?id=270
>> Signed-off-by: Chuck Lever 
>> ---
>> 
>> This fixes a 4.3-rc regression. Please apply to 4.3-rc when this
>> patch passes review.
>> 
>> It could also be appropriate for stable kernels which do not have
>> commit 7e5be28827bf ("svcrdma: advertise the correct max payload"),
>> though I have not tested them with this patch.
>> 
>> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c 
>> b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> index cb51742..37b4341 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> @@ -531,7 +531,7 @@ static int rdma_read_complete(struct svc_rqst *rqstp,
>>rqstp->rq_arg.page_base = head->arg.page_base;
>> 
>>/* rq_respages starts after the last arg page */
>> -rqstp->rq_respages = >rq_arg.pages[page_no];
>> +rqstp->rq_respages = >rq_pages[page_no];
>>rqstp->rq_next_page = rqstp->rq_respages + 1;
>> 
>>/* Rebuild rq_arg head and tail. */
>> 
> 
> Reviewed-by: Steve Wise 

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html