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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH v1 3/3] svcrdma: Fix leak of svc_rdma_recv_ctxt objects
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200408060242.GB3310@unreal>
Date:   Thu, 9 Apr 2020 10:33:32 -0400
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-rdma@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <D3CFDCAA-589C-4B3F-B769-099BF775D098@oracle.com>
References: <20200407190938.24045.64947.stgit@klimt.1015granger.net>
 <20200407191106.24045.88035.stgit@klimt.1015granger.net>
 <20200408060242.GB3310@unreal>
To:     Leon Romanovsky <leon@kernel.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Leon-

> On Apr 8, 2020, at 2:02 AM, Leon Romanovsky <leon@kernel.org> wrote:
>=20
> 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.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> Fixes: 3a88092ee319 ("svcrdma: Preserve Receive buffer until ... ")
>=20
> Chuck,
>=20
> Can you please don't mangle the Fixes line?

I've read e-mail from others that advocate this form of mangling
instead of using commit message lines that are too long.


> A lot of automatization is relying on the fact that this line is =
canonical,
> both in format and in the actual content.

Understood, but checkpatch.pl does not complain about it. Perhaps,
therefore, it is the automation that is not correct.

The commit ID is what automation should key off of. The short
description is only for human consumption. In fact, the short
description can easily be reconstituted from the commit ID. The
reverse cannot be done reliably.

I'm not interested in bike-shedding this, however, so I will try
to remember to use complete short descriptions when posting to
linux-rdma.


> Thanks
>=20
>> 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(-)
>>=20
>> 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 *);
>>=20
>> /* 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);
>> }
>>=20
>> +/**
>> + * 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 =3D rqstp->rq_xprt_ctxt;
>> +	struct svc_xprt *xprt =3D rqstp->rq_xprt;
>> +	struct svcxprt_rdma *rdma =3D
>> +		container_of(xprt, struct svcxprt_rdma, sc_xprt);
>> +
>> +	rqstp->rq_xprt_ctxt =3D 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;
>>=20
>> +	rqstp->rq_xprt_ctxt =3D NULL;
>> +
>> 	spin_lock(&rdma_xprt->sc_rq_dto_lock);
>> 	ctxt =3D =
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 =3D svc_rdma_send_reply_msg(rdma, sctxt, rctxt, rqstp);
>> 	if (ret < 0)
>> 		goto err1;
>> -	ret =3D 0;
>> -
>> -out:
>> -	rqstp->rq_xprt_ctxt =3D NULL;
>> -	svc_rdma_recv_ctxt_put(rdma, rctxt);
>> -	return ret;
>> +	return 0;
>>=20
>>  err2:
>> 	if (ret !=3D -E2BIG && ret !=3D -EINVAL)
>> @@ -940,16 +935,14 @@ int svc_rdma_sendto(struct svc_rqst *rqstp)
>> 	ret =3D svc_rdma_send_error_msg(rdma, sctxt, rqstp);
>> 	if (ret < 0)
>> 		goto err1;
>> -	ret =3D 0;
>> -	goto out;
>> +	return 0;
>>=20
>>  err1:
>> 	svc_rdma_send_ctxt_put(rdma, sctxt);
>>  err0:
>> 	trace_svcrdma_send_failed(rqstp, ret);
>> 	set_bit(XPT_CLOSE, &xprt->xpt_flags);
>> -	ret =3D -ENOTCONN;
>> -	goto out;
>> +	return -ENOTCONN;
>> }
>>=20
>> /**
>> 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;
>> }
>>=20
>> -static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
>> -{
>> -}
>> -
>> /*
>>  * When connected, an svc_xprt has at least two references:
>>  *

--
Chuck Lever