Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 8 Apr 2020 09:02:42 +0300
From:   Leon Romanovsky <leon@kernel.org>
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-nfs@vger.kernel.org, linux-rdma@vger.kernel.org
Subject: Re: [PATCH v1 3/3] svcrdma: Fix leak of svc_rdma_recv_ctxt objects
Message-ID: <20200408060242.GB3310@unreal>
References: <20200407190938.24045.64947.stgit@klimt.1015granger.net>
 <20200407191106.24045.88035.stgit@klimt.1015granger.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200407191106.24045.88035.stgit@klimt.1015granger.net>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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@oracle.com>
> ---
>  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:
>   *
>