| Message ID | 164935340273.76813.7096678268046264254.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix request deferral for NFS/RDMA | expand |
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 <trondmy@hammerspace.com> > Link: > https://lore.kernel.org/linux-nfs/82662b7190f26fb304eb0ab1bb04279072439d4e.camel@hammerspace.com/ > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > Cc: <stable@vger.kernel.org> > --- > 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.
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 <trondmy@hammerspace.com> > Link: https://lore.kernel.org/linux-nfs/82662b7190f26fb304eb0ab1bb04279072439d4e.camel@hammerspace.com/ > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > Cc: <stable@vger.kernel.org> > --- > 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 <neilb@suse.de> 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 <trondmy@hammerspace.com> >> Link: https://lore.kernel.org/linux-nfs/82662b7190f26fb304eb0ab1bb04279072439d4e.camel@hammerspace.com/ >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> Cc: <stable@vger.kernel.org> >> --- >> 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
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;
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 <trondmy@hammerspace.com> Link: https://lore.kernel.org/linux-nfs/82662b7190f26fb304eb0ab1bb04279072439d4e.camel@hammerspace.com/ Signed-off-by: Chuck Lever <chuck.lever@oracle.com> Cc: <stable@vger.kernel.org> --- 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(-)