DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 62EAA20863
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH v3 3/3] SUNRPC: Ensure we refresh the bvec after
 RPCSEC_GSS encoding
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <545d13619ffe92a69097f8f68fc36df85944cac2.camel@hammerspace.com>
Date:   Fri, 30 Nov 2018 17:23:25 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <7B7D615F-8A5B-4EBD-96DC-FDDA9DC35DD9@oracle.com>
References: <20181130215555.91107-1-trond.myklebust@hammerspace.com>
 <20181130215555.91107-2-trond.myklebust@hammerspace.com>
 <20181130215555.91107-3-trond.myklebust@hammerspace.com>
 <E9450F2D-D888-4EFC-B0BD-6506E4EC2887@oracle.com>
 <545d13619ffe92a69097f8f68fc36df85944cac2.camel@hammerspace.com>
To:     Trond Myklebust <trondmy@hammerspace.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Nov 30, 2018, at 5:18 PM, Trond Myklebust <trondmy@hammerspace.com> =
wrote:
>=20
> On Fri, 2018-11-30 at 17:15 -0500, Chuck Lever wrote:
>>> On Nov 30, 2018, at 4:55 PM, Trond Myklebust <trondmy@gmail.com>
>>> wrote:
>>>=20
>>> A call to gss_wrap_req_priv() will end up replacing the original
>>> array
>>> in rqstp->rq_snd_buf.pages with a new one containing the encrypted
>>> data. In order to avoid having the rqstp->rq_snd_buf.bvec point to
>>> the
>>> wrong page data, we need to refresh that too.
>>=20
>> I would add a note here that this patch fixes a memory leak. And
>> you might want to add a Fixes: tag.
>>=20
>=20
> It only applies to new code that went into 4.20, so it won't need any
> stable backporting.
>=20
> That said, I'm realising (slowly - apparently I'm asleep today) that
> this is receive side code, so
>=20
> a) The contents of rq_snd_buf are irrelevant.
> b) We want to beware of changing it while there is a copy in
> rq_private_buf.
>=20
> IOW: a v4 is forthcoming.

fwiw, i see the soft IRQ warnings with NFS/RDMA too.

I would like to turn those into a pr_warn_rate_limited. I don't
see much point in the backtrace blather.


>> I'm trying it out now.
>>=20
>>=20
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>> include/linux/sunrpc/xdr.h | 1 -
>>> net/sunrpc/xprt.c          | 2 ++
>>> net/sunrpc/xprtsock.c      | 1 +
>>> 3 files changed, 3 insertions(+), 1 deletion(-)
>>>=20
>>> diff --git a/include/linux/sunrpc/xdr.h
>>> b/include/linux/sunrpc/xdr.h
>>> index 43106ffa6788..2ec128060239 100644
>>> --- a/include/linux/sunrpc/xdr.h
>>> +++ b/include/linux/sunrpc/xdr.h
>>> @@ -72,7 +72,6 @@ xdr_buf_init(struct xdr_buf *buf, void *start,
>>> size_t len)
>>> 	buf->head[0].iov_base =3D start;
>>> 	buf->head[0].iov_len =3D len;
>>> 	buf->tail[0].iov_len =3D 0;
>>> -	buf->bvec =3D NULL;
>>> 	buf->pages =3D NULL;
>>> 	buf->page_len =3D 0;
>>> 	buf->flags =3D 0;
>>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>>> index 86bea4520c4d..122c91c28e7c 100644
>>> --- a/net/sunrpc/xprt.c
>>> +++ b/net/sunrpc/xprt.c
>>> @@ -1623,6 +1623,8 @@ xprt_request_init(struct rpc_task *task)
>>> 	req->rq_snd_buf.buflen =3D 0;
>>> 	req->rq_rcv_buf.len =3D 0;
>>> 	req->rq_rcv_buf.buflen =3D 0;
>>> +	req->rq_snd_buf.bvec =3D NULL;
>>> +	req->rq_rcv_buf.bvec =3D NULL;
>>> 	req->rq_release_snd_buf =3D NULL;
>>> 	xprt_reset_majortimeo(req);
>>> 	dprintk("RPC: %5u reserved req %p xid %08x\n", task->tk_pid,
>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
>>> index ae77c71c1f64..615ef2397fc5 100644
>>> --- a/net/sunrpc/xprtsock.c
>>> +++ b/net/sunrpc/xprtsock.c
>>> @@ -843,6 +843,7 @@ static int xs_nospace(struct rpc_rqst *req)
>>> static void
>>> xs_stream_prepare_request(struct rpc_rqst *req)
>>> {
>>> +	xdr_free_bvec(&req->rq_rcv_buf);
>>> 	req->rq_task->tk_status =3D xdr_alloc_bvec(&req->rq_rcv_buf,
>>> GFP_NOIO);
>>> }
>>>=20
>>> --=20
>>> 2.19.2
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
> --=20
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com

--
Chuck Lever