DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 235F52080A
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH v2 3/3] SUNRPC: Ensure we refresh the bvec after
 RPCSEC_GSS encoding
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <eaa3259a1aa12e29b8bd824f7cd417d6346239f9.camel@gmail.com>
Date:   Fri, 30 Nov 2018 16:32:18 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: 7bit
Message-Id: <93C4273C-DBBD-4B75-9F1F-EB103F2F430A@oracle.com>
References: <20181130211846.89000-1-trond.myklebust@hammerspace.com>
 <20181130211846.89000-2-trond.myklebust@hammerspace.com>
 <20181130211846.89000-3-trond.myklebust@hammerspace.com>
 <9D04832A-FC88-409C-8742-CBF742B6CB4A@oracle.com>
 <eaa3259a1aa12e29b8bd824f7cd417d6346239f9.camel@gmail.com>
To:     Trond Myklebust <trondmy@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Nov 30, 2018, at 4:26 PM, Trond Myklebust <trondmy@gmail.com> wrote:
> 
> On Fri, 2018-11-30 at 16:23 -0500, Chuck Lever wrote:
>>> On Nov 30, 2018, at 4:18 PM, Trond Myklebust <trondmy@gmail.com>
>>> wrote:
>>> 
>>> 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.
>>> 
>>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> ---
>>> net/sunrpc/xprtsock.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> 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);
>> 
>> I've added a pr_warn that fires whenever rq_rcv_buf.bvec != NULL,
>> and it never made a peep. Am I missing something?
>> 
> 
> It may be non-NULL,

I never found a case where call_encode was passing in a stale bvec array.


> but contain an array of pointers to the wrong
> pages. That's going to be the case when we re-encode the request,
> because (as your patch pointed out) we free the old array of
> rq_enc_pages (and its contents) and allocate a whole new array + set of
> pages.

I actually had a patch that looked exactly like yours (except for the
description text). It didn't seem to do anything. So there may be an
issue here, but I don't believe I'm seeing it with my reproducer.

I'll keep an eye out.


>>> 	req->rq_task->tk_status = xdr_alloc_bvec(&req->rq_rcv_buf,
>>> GFP_NOIO);
>>> }
>>> 
>>> -- 
>>> 2.19.2
>>> 
>> 
>> --
>> Chuck Lever

--
Chuck Lever