Last week Trond reported a server crash in svc_rdma_sendto() that
seemed related to deferral of a request incoming on an RPC/RDMA
transport. A link to that report appears in the first patch below.
The first patch in this series addresses the crash, and the second
fixes a related observability crash. The other patches from v1
of this series have been postponed to 5.19.
Comments, opinions, and Reviewed-by's are appreciated!
Since v2:
- Fix "has unsafe dereference" warnings in patch 2/2
Since v1:
- Series trimmed down to just what will go to 5.18-rc
- 2nd patch re-worked so it can be backported to recent stable
- Addressed a few checkpatch.pl nits
---
Chuck Lever (2):
SUNRPC: Fix NFSD's request deferral on RDMA transports
SUNRPC: Fix the svc_deferred_event trace class
include/linux/sunrpc/svc.h | 1 +
include/trace/events/sunrpc.h | 7 ++++---
net/sunrpc/svc_xprt.c | 3 +++
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +-
4 files changed, 9 insertions(+), 4 deletions(-)
--
Chuck Lever
Trond Myklebust reports an NFSD crash in svc_rdma_sendto(). Further
investigation shows that the crash occurred while NFSD was handling
a deferred request.
This patch addresses two inter-related issues that prevent request
deferral from working correctly for RPC/RDMA requests:
1. Prevent the crash by ensuring that the original
svc_rqst::rq_xprt_ctxt value is available when the request is
revisited. Otherwise svc_rdma_sendto() does not have a Receive
context available with which to construct its reply.
2. Possibly since before commit 71641d99ce03 ("svcrdma: Properly
compute .len and .buflen for received RPC Calls"),
svc_rdma_recvfrom() did not include the transport header in the
returned xdr_buf. There should have been no need for svc_defer()
and friends to save and restore that header, as of that commit.
This issue is addressed in a backport-friendly way by simply
having svc_rdma_recvfrom() set rq_xprt_hlen to zero
unconditionally, just as svc_tcp_recvfrom() does. This enables
svc_deferred_recv() to correctly reconstruct an RPC message
received via RPC/RDMA.
Reported-by: Trond Myklebust <[email protected]>
Link: https://lore.kernel.org/linux-nfs/[email protected]/
Signed-off-by: Chuck Lever <[email protected]>
Cc: <[email protected]>
---
include/linux/sunrpc/svc.h | 1 +
net/sunrpc/svc_xprt.c | 3 +++
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +-
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index a5dda4987e8b..217711fc9cac 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -395,6 +395,7 @@ struct svc_deferred_req {
size_t addrlen;
struct sockaddr_storage daddr; /* where reply must come from */
size_t daddrlen;
+ void *xprt_ctxt;
struct cache_deferred_req handle;
size_t xprt_hlen;
int argslen;
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 0c117d3bfda8..b42cfffa7395 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -1231,6 +1231,8 @@ static struct cache_deferred_req *svc_defer(struct cache_req *req)
dr->daddr = rqstp->rq_daddr;
dr->argslen = rqstp->rq_arg.len >> 2;
dr->xprt_hlen = rqstp->rq_xprt_hlen;
+ dr->xprt_ctxt = rqstp->rq_xprt_ctxt;
+ rqstp->rq_xprt_ctxt = NULL;
/* back up head to the start of the buffer and copy */
skip = rqstp->rq_arg.len - rqstp->rq_arg.head[0].iov_len;
@@ -1269,6 +1271,7 @@ static noinline int svc_deferred_recv(struct svc_rqst *rqstp)
rqstp->rq_xprt_hlen = dr->xprt_hlen;
rqstp->rq_daddr = dr->daddr;
rqstp->rq_respages = rqstp->rq_pages;
+ rqstp->rq_xprt_ctxt = dr->xprt_ctxt;
svc_xprt_received(rqstp->rq_xprt);
return (dr->argslen<<2) - dr->xprt_hlen;
}
diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
index cf76a6ad127b..864131a9fc6e 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
@@ -831,7 +831,7 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
goto out_err;
if (ret == 0)
goto out_drop;
- rqstp->rq_xprt_hlen = ret;
+ rqstp->rq_xprt_hlen = 0;
if (svc_rdma_is_reverse_direction_reply(xprt, ctxt))
goto out_backchannel;
On Thu, 2022-04-07 at 13:43 -0400, Chuck Lever wrote:
> Trond Myklebust reports an NFSD crash in svc_rdma_sendto(). Further
> investigation shows that the crash occurred while NFSD was handling
> a deferred request.
>
> This patch addresses two inter-related issues that prevent request
> deferral from working correctly for RPC/RDMA requests:
>
> 1. Prevent the crash by ensuring that the original
> svc_rqst::rq_xprt_ctxt value is available when the request is
> revisited. Otherwise svc_rdma_sendto() does not have a Receive
> context available with which to construct its reply.
>
> 2. Possibly since before commit 71641d99ce03 ("svcrdma: Properly
> compute .len and .buflen for received RPC Calls"),
> svc_rdma_recvfrom() did not include the transport header in the
> returned xdr_buf. There should have been no need for svc_defer()
> and friends to save and restore that header, as of that commit.
> This issue is addressed in a backport-friendly way by simply
> having svc_rdma_recvfrom() set rq_xprt_hlen to zero
> unconditionally, just as svc_tcp_recvfrom() does. This enables
> svc_deferred_recv() to correctly reconstruct an RPC message
> received via RPC/RDMA.
>
> Reported-by: Trond Myklebust <[email protected]>
> Link:
> https://lore.kernel.org/linux-nfs/[email protected]/
> Signed-off-by: Chuck Lever <[email protected]>
> Cc: <[email protected]>
> ---
> include/linux/sunrpc/svc.h | 1 +
> net/sunrpc/svc_xprt.c | 3 +++
> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +-
> 3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index a5dda4987e8b..217711fc9cac 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -395,6 +395,7 @@ struct svc_deferred_req {
> size_t addrlen;
> struct sockaddr_storage daddr; /* where reply must come from
> */
> size_t daddrlen;
> + void *xprt_ctxt;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 0c117d3bfda8..b42cfffa7395 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -1231,6 +1231,8 @@ static struct cache_deferred_req
> *svc_defer(struct cache_req *req)
> dr->daddr = rqstp->rq_daddr;
> dr->argslen = rqstp->rq_arg.len >> 2;
> dr->xprt_hlen = rqstp->rq_xprt_hlen;
> + dr->xprt_ctxt = rqstp->rq_xprt_ctxt;
> + rqstp->rq_xprt_ctxt = NULL;
>
> /* back up head to the start of the buffer and copy
> */
> skip = rqstp->rq_arg.len - rqstp-
> >rq_arg.head[0].iov_len;
> @@ -1269,6 +1271,7 @@ static noinline int svc_deferred_recv(struct
> svc_rqst *rqstp)
> rqstp->rq_xprt_hlen = dr->xprt_hlen;
> rqstp->rq_daddr = dr->daddr;
> rqstp->rq_respages = rqstp->rq_pages;
> + rqstp->rq_xprt_ctxt = dr->xprt_ctxt;
> svc_xprt_received(rqstp->rq_xprt);
> return (dr->argslen<<2) - dr->xprt_hlen;
> }
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index cf76a6ad127b..864131a9fc6e 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -831,7 +831,7 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
> goto out_err;
> if (ret == 0)
> goto out_drop;
> - rqstp->rq_xprt_hlen = ret;
> + rqstp->rq_xprt_hlen = 0;
>
> if (svc_rdma_is_reverse_direction_reply(xprt, ctxt))
> goto out_backchannel;
>
>
Thanks for the patch, Chuck!
Given how there doesn't appear to be any possible alternative ways to
generate that particular stack trace, I'm fairly sure this will fix the
issue we saw, but I'll make sure we give it a thorough testing.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Fri, 08 Apr 2022, Chuck Lever wrote:
> Trond Myklebust reports an NFSD crash in svc_rdma_sendto(). Further
> investigation shows that the crash occurred while NFSD was handling
> a deferred request.
>
> This patch addresses two inter-related issues that prevent request
> deferral from working correctly for RPC/RDMA requests:
>
> 1. Prevent the crash by ensuring that the original
> svc_rqst::rq_xprt_ctxt value is available when the request is
> revisited. Otherwise svc_rdma_sendto() does not have a Receive
> context available with which to construct its reply.
>
> 2. Possibly since before commit 71641d99ce03 ("svcrdma: Properly
> compute .len and .buflen for received RPC Calls"),
> svc_rdma_recvfrom() did not include the transport header in the
> returned xdr_buf. There should have been no need for svc_defer()
> and friends to save and restore that header, as of that commit.
> This issue is addressed in a backport-friendly way by simply
> having svc_rdma_recvfrom() set rq_xprt_hlen to zero
> unconditionally, just as svc_tcp_recvfrom() does. This enables
> svc_deferred_recv() to correctly reconstruct an RPC message
> received via RPC/RDMA.
I'm a bit late to this party but .... this patch is bad and I don't know
how best to fix it.
It is bad for two reasons.
1/ It can leak memory. When a deferral happens, the context is moved
into an svc_deferred_req. There are a couple of places which assume
that an svc_deferred_req can be freed with kfree(), such as
svc_delete_xprt() and svc_revisit(). These will now leak the
context. This is the bit that is hard to fix.
2/ It can cause a double-free with UDP (and maybe rdma).
When a request is deferred, the ctxt is moved to the dreq.
When that request is revisited the ctxt is copied back into the rqst.
If the rqst is again deferred then the old dreq is reused and,
importantly, the rq_xprt_ctxt is not cleared. So when the release
function is called the ctxt is freed.
When the request is revisited a second time that ctxt (now pointing
to freed and possibly reused memory) is copied back into the rqst.
When the request completes the ctxt is again freed - double free
oops.
For UDP there is no value in saving the ctxt in the dreq - the content
of the ctxt, which is an skbuf, has been copied into the dreq. So maybe
the UDB ctxt is a very different beast than the rdma ctxt and needs to
be handled completely differently.
We can fix the UDP double-free by always doing
rqstp->rq_xprt_ctxt = NULL;
whether a new dreq was needed or not. But that doesn't fix the leaking
of ctxts and is really just a band-aid.
Thoughts?
Do we need to preserve ALL of the svc_rdma_recv_ctxt for deferred
requests? Could we just copy some bits into the dreq (allocate a bit
more space somehow) so that a simple kfree() is still enough?
Or do we need a free_ctxt() handler attached to the xprt?
Thanks,
NeilBrown
>
> Reported-by: Trond Myklebust <[email protected]>
> Link: https://lore.kernel.org/linux-nfs/[email protected]/
> Signed-off-by: Chuck Lever <[email protected]>
> Cc: <[email protected]>
> ---
> include/linux/sunrpc/svc.h | 1 +
> net/sunrpc/svc_xprt.c | 3 +++
> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +-
> 3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index a5dda4987e8b..217711fc9cac 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -395,6 +395,7 @@ struct svc_deferred_req {
> size_t addrlen;
> struct sockaddr_storage daddr; /* where reply must come from */
> size_t daddrlen;
> + void *xprt_ctxt;
> struct cache_deferred_req handle;
> size_t xprt_hlen;
> int argslen;
> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
> index 0c117d3bfda8..b42cfffa7395 100644
> --- a/net/sunrpc/svc_xprt.c
> +++ b/net/sunrpc/svc_xprt.c
> @@ -1231,6 +1231,8 @@ static struct cache_deferred_req *svc_defer(struct cache_req *req)
> dr->daddr = rqstp->rq_daddr;
> dr->argslen = rqstp->rq_arg.len >> 2;
> dr->xprt_hlen = rqstp->rq_xprt_hlen;
> + dr->xprt_ctxt = rqstp->rq_xprt_ctxt;
> + rqstp->rq_xprt_ctxt = NULL;
>
> /* back up head to the start of the buffer and copy */
> skip = rqstp->rq_arg.len - rqstp->rq_arg.head[0].iov_len;
> @@ -1269,6 +1271,7 @@ static noinline int svc_deferred_recv(struct svc_rqst *rqstp)
> rqstp->rq_xprt_hlen = dr->xprt_hlen;
> rqstp->rq_daddr = dr->daddr;
> rqstp->rq_respages = rqstp->rq_pages;
> + rqstp->rq_xprt_ctxt = dr->xprt_ctxt;
> svc_xprt_received(rqstp->rq_xprt);
> return (dr->argslen<<2) - dr->xprt_hlen;
> }
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index cf76a6ad127b..864131a9fc6e 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -831,7 +831,7 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
> goto out_err;
> if (ret == 0)
> goto out_drop;
> - rqstp->rq_xprt_hlen = ret;
> + rqstp->rq_xprt_hlen = 0;
>
> if (svc_rdma_is_reverse_direction_reply(xprt, ctxt))
> goto out_backchannel;
>
>
>
> On May 7, 2023, at 5:20 PM, NeilBrown <[email protected]> wrote:
>
> On Fri, 08 Apr 2022, Chuck Lever wrote:
>> Trond Myklebust reports an NFSD crash in svc_rdma_sendto(). Further
>> investigation shows that the crash occurred while NFSD was handling
>> a deferred request.
>>
>> This patch addresses two inter-related issues that prevent request
>> deferral from working correctly for RPC/RDMA requests:
>>
>> 1. Prevent the crash by ensuring that the original
>> svc_rqst::rq_xprt_ctxt value is available when the request is
>> revisited. Otherwise svc_rdma_sendto() does not have a Receive
>> context available with which to construct its reply.
>>
>> 2. Possibly since before commit 71641d99ce03 ("svcrdma: Properly
>> compute .len and .buflen for received RPC Calls"),
>> svc_rdma_recvfrom() did not include the transport header in the
>> returned xdr_buf. There should have been no need for svc_defer()
>> and friends to save and restore that header, as of that commit.
>> This issue is addressed in a backport-friendly way by simply
>> having svc_rdma_recvfrom() set rq_xprt_hlen to zero
>> unconditionally, just as svc_tcp_recvfrom() does. This enables
>> svc_deferred_recv() to correctly reconstruct an RPC message
>> received via RPC/RDMA.
>
> I'm a bit late to this party but .... this patch is bad and I don't know
> how best to fix it.
> It is bad for two reasons.
> 1/ It can leak memory. When a deferral happens, the context is moved
> into an svc_deferred_req. There are a couple of places which assume
> that an svc_deferred_req can be freed with kfree(), such as
> svc_delete_xprt() and svc_revisit(). These will now leak the
> context. This is the bit that is hard to fix.
>
> 2/ It can cause a double-free with UDP (and maybe rdma).
> When a request is deferred, the ctxt is moved to the dreq.
> When that request is revisited the ctxt is copied back into the rqst.
> If the rqst is again deferred then the old dreq is reused and,
> importantly, the rq_xprt_ctxt is not cleared. So when the release
> function is called the ctxt is freed.
> When the request is revisited a second time that ctxt (now pointing
> to freed and possibly reused memory) is copied back into the rqst.
> When the request completes the ctxt is again freed - double free
> oops.
>
> For UDP there is no value in saving the ctxt in the dreq - the content
> of the ctxt, which is an skbuf, has been copied into the dreq. So maybe
> the UDB ctxt is a very different beast than the rdma ctxt and needs to
> be handled completely differently.
>
> We can fix the UDP double-free by always doing
> rqstp->rq_xprt_ctxt = NULL;
> whether a new dreq was needed or not. But that doesn't fix the leaking
> of ctxts and is really just a band-aid.
A double-free is potentially catastrophic, so I would
say this is a reasonable change that can be back-ported
without fuss (while noting that more is needed).
The RDMA recv_ctxt is persistent for the life of the
connection. Releasing one of those just puts it back on
a free list. So, maybe the second free could cause free
list corruption?
> Thoughts?
> Do we need to preserve ALL of the svc_rdma_recv_ctxt for deferred
> requests? Could we just copy some bits into the dreq (allocate a bit
> more space somehow) so that a simple kfree() is still enough?
> Or do we need a free_ctxt() handler attached to the xprt?
While I take some time to review code and options, did
you know there is a deferral fault injector that might
possibly help you sort through some of this? I doubt I
took the time to try forcing a second deferral of the
same request.
> Thanks,
> NeilBrown
>
>
>
>
>>
>> Reported-by: Trond Myklebust <[email protected]>
>> Link: https://lore.kernel.org/linux-nfs/[email protected]/
>> Signed-off-by: Chuck Lever <[email protected]>
>> Cc: <[email protected]>
>> ---
>> include/linux/sunrpc/svc.h | 1 +
>> net/sunrpc/svc_xprt.c | 3 +++
>> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 2 +-
>> 3 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index a5dda4987e8b..217711fc9cac 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -395,6 +395,7 @@ struct svc_deferred_req {
>> size_t addrlen;
>> struct sockaddr_storage daddr; /* where reply must come from */
>> size_t daddrlen;
>> + void *xprt_ctxt;
>> struct cache_deferred_req handle;
>> size_t xprt_hlen;
>> int argslen;
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index 0c117d3bfda8..b42cfffa7395 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -1231,6 +1231,8 @@ static struct cache_deferred_req *svc_defer(struct cache_req *req)
>> dr->daddr = rqstp->rq_daddr;
>> dr->argslen = rqstp->rq_arg.len >> 2;
>> dr->xprt_hlen = rqstp->rq_xprt_hlen;
>> + dr->xprt_ctxt = rqstp->rq_xprt_ctxt;
>> + rqstp->rq_xprt_ctxt = NULL;
>>
>> /* back up head to the start of the buffer and copy */
>> skip = rqstp->rq_arg.len - rqstp->rq_arg.head[0].iov_len;
>> @@ -1269,6 +1271,7 @@ static noinline int svc_deferred_recv(struct svc_rqst *rqstp)
>> rqstp->rq_xprt_hlen = dr->xprt_hlen;
>> rqstp->rq_daddr = dr->daddr;
>> rqstp->rq_respages = rqstp->rq_pages;
>> + rqstp->rq_xprt_ctxt = dr->xprt_ctxt;
>> svc_xprt_received(rqstp->rq_xprt);
>> return (dr->argslen<<2) - dr->xprt_hlen;
>> }
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> index cf76a6ad127b..864131a9fc6e 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>> @@ -831,7 +831,7 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
>> goto out_err;
>> if (ret == 0)
>> goto out_drop;
>> - rqstp->rq_xprt_hlen = ret;
>> + rqstp->rq_xprt_hlen = 0;
>>
>> if (svc_rdma_is_reverse_direction_reply(xprt, ctxt))
>> goto out_backchannel;
--
Chuck Lever