On Tue, Apr 07, 2020 at 03:11:06PM -0400, Chuck Lever wrote:
> Utilize the xpo_release_rqst transport method to ensure that each
> rqstp's svc_rdma_recv_ctxt object is released even when the server
> cannot return a Reply for that rqstp.
>
> Without this fix, each RPC whose Reply cannot be sent leaks one
> svc_rdma_recv_ctxt. This is a 2.5KB structure, a 4KB DMA-mapped
> Receive buffer, and any pages that might be part of the Reply
> message.
>
> The leak is infrequent unless the network fabric is unreliable or
> Kerberos is in use, as GSS sequence window overruns, which result
> in connection loss, are more common on fast transports.
>
> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
Chuck,
Can you please don't mangle the Fixes line?
A lot of automatization is relying on the fact that this line is canonical,
both in format and in the actual content.
Thanks
> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> ---
> include/linux/sunrpc/svc_rdma.h | 1 +
> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 22 ++++++++++++++++++++++
> net/sunrpc/xprtrdma/svc_rdma_sendto.c | 13 +++----------
> net/sunrpc/xprtrdma/svc_rdma_transport.c | 5 -----
> 4 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/sunrpc/svc_rdma.h b/include/linux/sunrpc/svc_rdma.h
> index 78fe2ac6dc6c..cbcfbd0521e3 100644
> --- a/include/linux/sunrpc/svc_rdma.h
> +++ b/include/linux/sunrpc/svc_rdma.h
> @@ -170,6 +170,7 @@ extern bool svc_rdma_post_recvs(struct svcxprt_rdma *rdma);
> extern void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
> struct svc_rdma_recv_ctxt *ctxt);
> extern void svc_rdma_flush_recv_queues(struct svcxprt_rdma *rdma);
> +extern void svc_rdma_release_rqst(struct svc_rqst *rqstp);
> extern int svc_rdma_recvfrom(struct svc_rqst *);
>
> /* svc_rdma_rw.c */
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> index 54469b72b25f..efa5fcb5793f 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
> @@ -223,6 +223,26 @@ void svc_rdma_recv_ctxt_put(struct svcxprt_rdma *rdma,
> svc_rdma_recv_ctxt_destroy(rdma, ctxt);
> }
>
> +/**
> + * svc_rdma_release_rqst - Release transport-specific per-rqst resources
> + * @rqstp: svc_rqst being released
> + *
> + * Ensure that the recv_ctxt is released whether or not a Reply
> + * was sent. For example, the client could close the connection,
> + * or svc_process could drop an RPC, before the Reply is sent.
> + */
> +void svc_rdma_release_rqst(struct svc_rqst *rqstp)
> +{
> + struct svc_rdma_recv_ctxt *ctxt = rqstp->rq_xprt_ctxt;
> + struct svc_xprt *xprt = rqstp->rq_xprt;
> + struct svcxprt_rdma *rdma =
> + container_of(xprt, struct svcxprt_rdma, sc_xprt);
> +
> + rqstp->rq_xprt_ctxt = NULL;
> + if (ctxt)
> + svc_rdma_recv_ctxt_put(rdma, ctxt);
> +}
> +
> static int __svc_rdma_post_recv(struct svcxprt_rdma *rdma,
> struct svc_rdma_recv_ctxt *ctxt)
> {
> @@ -820,6 +840,8 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
> __be32 *p;
> int ret;
>
> + rqstp->rq_xprt_ctxt = NULL;
> +
> spin_lock(&rdma_xprt->sc_rq_dto_lock);
> ctxt = svc_rdma_next_recv_ctxt(&rdma_xprt->sc_read_complete_q);
> if (ctxt) {
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> index 6a87a2379e91..b6c8643867f2 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> @@ -926,12 +926,7 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
> ret = svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp);
> if (ret < 0)
> goto err1;
> - ret = 0;
> -
> -out:
> - rqstp->rq_xprt_ctxt = NULL;
> - svc_rdma_recv_ctxt_put(rdma, rctxt);
> - return ret;
> + return 0;
>
> err2:
> if (ret != -E2BIG && ret != -EINVAL)
> @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
> ret = svc_rdma_send_error_msg(rdma, sctxt, rqstp);
> if (ret < 0)
> goto err1;
> - ret = 0;
> - goto out;
> + return 0;
>
> err1:
> svc_rdma_send_ctxt_put(rdma, sctxt);
> err0:
> trace_svcrdma_send_failed(rqstp, ret);
> set_bit(XPT_CLOSE, &xprt->xpt_flags);
> - ret = -ENOTCONN;
> - goto out;
> + return -ENOTCONN;
> }
>
> /**
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 8bb99980ae85..ea54785db4f8 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -71,7 +71,6 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> struct sockaddr *sa, int salen,
> int flags);
> static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt);
> -static void svc_rdma_release_rqst(struct svc_rqst *);
> static void svc_rdma_detach(struct svc_xprt *xprt);
> static void svc_rdma_free(struct svc_xprt *xprt);
> static int svc_rdma_has_wspace(struct svc_xprt *xprt);
> @@ -552,10 +551,6 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
> return NULL;
> }
>
> -static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
> -{
> -}
> -
> /*
> * When connected, an svc_xprt has at least two references:
> *
>
