DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3B00C2080A
Message-ID: <eaa3259a1aa12e29b8bd824f7cd417d6346239f9.camel@gmail.com>
Subject: Re: [PATCH v2 3/3] SUNRPC: Ensure we refresh the bvec after
 RPCSEC_GSS encoding
From:   Trond Myklebust <trondmy@gmail.com>
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Fri, 30 Nov 2018 16:26:15 -0500
In-Reply-To: <9D04832A-FC88-409C-8742-CBF742B6CB4A@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>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.2 (3.30.2-2.fc29) 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

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, 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.

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