| Message ID | 20181130211846.89000-3-trond.myklebust@hammerspace.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] SUNRPC: call_connect_status() must handle tasks that got transmitted | expand |
> 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? > req->rq_task->tk_status = xdr_alloc_bvec(&req->rq_rcv_buf, GFP_NOIO); > } > > -- > 2.19.2 > -- Chuck Lever
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 > > >
> 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
On Fri, 2018-11-30 at 16:32 -0500, Chuck Lever wrote: > > > 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. > Argh... Yes, I was too quick off the trigger. The call to xdr_buf_init() in rpc_xdr_encode() will actually clobber the ->bvec array...
> On Nov 30, 2018, at 4:37 PM, Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Fri, 2018-11-30 at 16:32 -0500, Chuck Lever wrote: >>> >> 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. >> > > Argh... Yes, I was too quick off the trigger. The call to > xdr_buf_init() in rpc_xdr_encode() will actually clobber the ->bvec > array... Duh. That sounds very plausible! -- Chuck Lever
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 = xdr_alloc_bvec(&req->rq_rcv_buf, GFP_NOIO); }
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(+)