| Message ID | 80ed0167773dfee0807c79233555c7d0c1fae11a.1448031054.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On Nov 20, 2015, at 9:55 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > A truncated cb_compound request will cause the client to decode null or > data from a previous callback for nfs4.1 backchannel case, or uninitialized > data for the nfs4.0 case. This is because the path through > svc_process_common() advances the request's iov_base and decrements iov_len > without adjusting the overall xdr_buf's len field. That causes > xdr_init_decode() to set up the xdr_stream with an incorrect length in > nfs4_callback_compound(). > > Fixing this for the nfs4.1 backchannel case first requires setting the > correct iov_len and page_len based on the length of received data in the > same manner as the nfs4.0 case. > > Then the request's xdr_buf length can be adjusted for both cases based upon > the remaining iov_len and page_len. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/callback_xdr.c | 7 +++++-- > net/sunrpc/backchannel_rqst.c | 8 ++++++++ > net/sunrpc/svc.c | 1 + > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > index 6b1697a..1c8213e 100644 > --- a/fs/nfs/callback_xdr.c > +++ b/fs/nfs/callback_xdr.c > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) > > p = xdr_inline_decode(xdr, nbytes); > if (unlikely(p == NULL)) > - printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n"); > + printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed " > + "or truncated request.\n"); > return p; > } > > @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > struct cb_compound_hdr_arg hdr_arg = { 0 }; > struct cb_compound_hdr_res hdr_res = { NULL }; > struct xdr_stream xdr_in, xdr_out; > + struct xdr_buf *rq_arg = &rqstp->rq_arg; > __be32 *p, status; > struct cb_process_state cps = { > .drc_status = 0, > @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > > dprintk("%s: start\n", __func__); > > - xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base); > + rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len; > + xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base); > > p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len); > xdr_init_encode(&xdr_out, &rqstp->rq_res, p); > diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c > index 6255d14..d92cee1 100644 > --- a/net/sunrpc/backchannel_rqst.c > +++ b/net/sunrpc/backchannel_rqst.c > @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) > { > struct rpc_xprt *xprt = req->rq_xprt; > struct svc_serv *bc_serv = xprt->bc_serv; > + struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf; > > spin_lock(&xprt->bc_pa_lock); > list_del(&req->rq_bc_pa_list); > xprt_dec_alloc_count(xprt, 1); > spin_unlock(&xprt->bc_pa_lock); > > + if (copied <= rq_rcv_buf->head[0].iov_len) { > + rq_rcv_buf->head[0].iov_len = copied; > + rq_rcv_buf->page_len = 0; > + } else { > + rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len; > + } > + Are similar changes needed in rpcrdma_bc_receive_call ? > req->rq_private_buf.len = copied; > set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state); > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index a8f579d..af9f987 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, > memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen); > memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg)); > memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res)); > + rqstp->rq_arg.len = req->rq_private_buf.len; > > /* reset result send buffer "put" position */ > resv->iov_len = 0; > -- > 1.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 20 Nov 2015, Chuck Lever wrote: > > > On Nov 20, 2015, at 9:55 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > > > A truncated cb_compound request will cause the client to decode null or > > data from a previous callback for nfs4.1 backchannel case, or uninitialized > > data for the nfs4.0 case. This is because the path through > > svc_process_common() advances the request's iov_base and decrements iov_len > > without adjusting the overall xdr_buf's len field. That causes > > xdr_init_decode() to set up the xdr_stream with an incorrect length in > > nfs4_callback_compound(). > > > > Fixing this for the nfs4.1 backchannel case first requires setting the > > correct iov_len and page_len based on the length of received data in the > > same manner as the nfs4.0 case. > > > > Then the request's xdr_buf length can be adjusted for both cases based upon > > the remaining iov_len and page_len. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfs/callback_xdr.c | 7 +++++-- > > net/sunrpc/backchannel_rqst.c | 8 ++++++++ > > net/sunrpc/svc.c | 1 + > > 3 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > > index 6b1697a..1c8213e 100644 > > --- a/fs/nfs/callback_xdr.c > > +++ b/fs/nfs/callback_xdr.c > > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) > > > > p = xdr_inline_decode(xdr, nbytes); > > if (unlikely(p == NULL)) > > - printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n"); > > + printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed " > > + "or truncated request.\n"); > > return p; > > } > > > > @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > > struct cb_compound_hdr_arg hdr_arg = { 0 }; > > struct cb_compound_hdr_res hdr_res = { NULL }; > > struct xdr_stream xdr_in, xdr_out; > > + struct xdr_buf *rq_arg = &rqstp->rq_arg; > > __be32 *p, status; > > struct cb_process_state cps = { > > .drc_status = 0, > > @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > > > > dprintk("%s: start\n", __func__); > > > > - xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base); > > + rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len; > > + xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base); > > > > p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len); > > xdr_init_encode(&xdr_out, &rqstp->rq_res, p); > > diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c > > index 6255d14..d92cee1 100644 > > --- a/net/sunrpc/backchannel_rqst.c > > +++ b/net/sunrpc/backchannel_rqst.c > > @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) > > { > > struct rpc_xprt *xprt = req->rq_xprt; > > struct svc_serv *bc_serv = xprt->bc_serv; > > + struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf; > > > > spin_lock(&xprt->bc_pa_lock); > > list_del(&req->rq_bc_pa_list); > > xprt_dec_alloc_count(xprt, 1); > > spin_unlock(&xprt->bc_pa_lock); > > > > + if (copied <= rq_rcv_buf->head[0].iov_len) { > > + rq_rcv_buf->head[0].iov_len = copied; > > + rq_rcv_buf->page_len = 0; > > + } else { > > + rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len; > > + } > > + > > Are similar changes needed in rpcrdma_bc_receive_call ? Good question.. I was working on 4.3+ which was missing the rdma stuff, so I overlooked this. net/sunrpc/xprtrdma/backchannel.c: 324 size = rep->rr_len - RPCRDMA_HDRLEN_MIN; ... 347 memset(buf, 0, sizeof(*buf)); 348 buf->head[0].iov_base = p; 349 buf->head[0].iov_len = size; ^^ that looks like it is going to set the iov_len to the size of the XDR portion of the data, sans transport headers.. which means that it shouldn't need the fixup just above. In the case requiring the fixup iov_len was always set to 4096, and decremented from there no matter the size of the request. Ben > > req->rq_private_buf.len = copied; > > set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state); > > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > > index a8f579d..af9f987 100644 > > --- a/net/sunrpc/svc.c > > +++ b/net/sunrpc/svc.c > > @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, > > memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen); > > memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg)); > > memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res)); > > + rqstp->rq_arg.len = req->rq_private_buf.len; > > > > /* reset result send buffer "put" position */ > > resv->iov_len = 0; > > -- > > 1.7.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- > Chuck Lever > > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Nov 20, 2015, at 11:20 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > > On Fri, 20 Nov 2015, Chuck Lever wrote: > >> >>> On Nov 20, 2015, at 9:55 AM, Benjamin Coddington <bcodding@redhat.com> wrote: >>> >>> A truncated cb_compound request will cause the client to decode null or >>> data from a previous callback for nfs4.1 backchannel case, or uninitialized >>> data for the nfs4.0 case. This is because the path through >>> svc_process_common() advances the request's iov_base and decrements iov_len >>> without adjusting the overall xdr_buf's len field. That causes >>> xdr_init_decode() to set up the xdr_stream with an incorrect length in >>> nfs4_callback_compound(). >>> >>> Fixing this for the nfs4.1 backchannel case first requires setting the >>> correct iov_len and page_len based on the length of received data in the >>> same manner as the nfs4.0 case. >>> >>> Then the request's xdr_buf length can be adjusted for both cases based upon >>> the remaining iov_len and page_len. >>> >>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com> >>> --- >>> fs/nfs/callback_xdr.c | 7 +++++-- >>> net/sunrpc/backchannel_rqst.c | 8 ++++++++ >>> net/sunrpc/svc.c | 1 + >>> 3 files changed, 14 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c >>> index 6b1697a..1c8213e 100644 >>> --- a/fs/nfs/callback_xdr.c >>> +++ b/fs/nfs/callback_xdr.c >>> @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) >>> >>> p = xdr_inline_decode(xdr, nbytes); >>> if (unlikely(p == NULL)) >>> - printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n"); >>> + printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed " >>> + "or truncated request.\n"); >>> return p; >>> } >>> >>> @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r >>> struct cb_compound_hdr_arg hdr_arg = { 0 }; >>> struct cb_compound_hdr_res hdr_res = { NULL }; >>> struct xdr_stream xdr_in, xdr_out; >>> + struct xdr_buf *rq_arg = &rqstp->rq_arg; >>> __be32 *p, status; >>> struct cb_process_state cps = { >>> .drc_status = 0, >>> @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r >>> >>> dprintk("%s: start\n", __func__); >>> >>> - xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base); >>> + rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len; >>> + xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base); >>> >>> p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len); >>> xdr_init_encode(&xdr_out, &rqstp->rq_res, p); >>> diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c >>> index 6255d14..d92cee1 100644 >>> --- a/net/sunrpc/backchannel_rqst.c >>> +++ b/net/sunrpc/backchannel_rqst.c >>> @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) >>> { >>> struct rpc_xprt *xprt = req->rq_xprt; >>> struct svc_serv *bc_serv = xprt->bc_serv; >>> + struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf; >>> >>> spin_lock(&xprt->bc_pa_lock); >>> list_del(&req->rq_bc_pa_list); >>> xprt_dec_alloc_count(xprt, 1); >>> spin_unlock(&xprt->bc_pa_lock); >>> >>> + if (copied <= rq_rcv_buf->head[0].iov_len) { >>> + rq_rcv_buf->head[0].iov_len = copied; >>> + rq_rcv_buf->page_len = 0; >>> + } else { >>> + rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len; >>> + } >>> + >> >> Are similar changes needed in rpcrdma_bc_receive_call ? > > Good question.. I was working on 4.3+ which was missing the rdma stuff, so > I overlooked this. > > net/sunrpc/xprtrdma/backchannel.c: > 324 size = rep->rr_len - RPCRDMA_HDRLEN_MIN; > ... > 347 memset(buf, 0, sizeof(*buf)); > 348 buf->head[0].iov_base = p; > 349 buf->head[0].iov_len = size; > > ^^ that looks like it is going to set the iov_len to the size of the XDR > portion of the data, sans transport headers.. which means that it shouldn't > need the fixup just above. > > In the case requiring the fixup iov_len was always set to 4096, and > decremented from there no matter the size of the request. Thanks for checking! > Ben > >>> req->rq_private_buf.len = copied; >>> set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state); >>> >>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c >>> index a8f579d..af9f987 100644 >>> --- a/net/sunrpc/svc.c >>> +++ b/net/sunrpc/svc.c >>> @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, >>> memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen); >>> memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg)); >>> memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res)); >>> + rqstp->rq_arg.len = req->rq_private_buf.len; >>> >>> /* reset result send buffer "put" position */ >>> resv->iov_len = 0; >>> -- >>> 1.7.1 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> -- >> Chuck Lever >> >> >> >> >> -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ben, I've hit a few problems with this patch after it went upstream. On Fri, Nov 20, 2015 at 6:55 AM, Benjamin Coddington <bcodding@redhat.com> wrote: > A truncated cb_compound request will cause the client to decode null or > data from a previous callback for nfs4.1 backchannel case, or uninitialized > data for the nfs4.0 case. This is because the path through > svc_process_common() advances the request's iov_base and decrements iov_len > without adjusting the overall xdr_buf's len field. That causes > xdr_init_decode() to set up the xdr_stream with an incorrect length in > nfs4_callback_compound(). > > Fixing this for the nfs4.1 backchannel case first requires setting the > correct iov_len and page_len based on the length of received data in the > same manner as the nfs4.0 case. > > Then the request's xdr_buf length can be adjusted for both cases based upon > the remaining iov_len and page_len. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/callback_xdr.c | 7 +++++-- > net/sunrpc/backchannel_rqst.c | 8 ++++++++ > net/sunrpc/svc.c | 1 + > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > index 6b1697a..1c8213e 100644 > --- a/fs/nfs/callback_xdr.c > +++ b/fs/nfs/callback_xdr.c > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) > > p = xdr_inline_decode(xdr, nbytes); > if (unlikely(p == NULL)) > - printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n"); > + printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed " > + "or truncated request.\n"); > return p; > } > > @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > struct cb_compound_hdr_arg hdr_arg = { 0 }; > struct cb_compound_hdr_res hdr_res = { NULL }; > struct xdr_stream xdr_in, xdr_out; > + struct xdr_buf *rq_arg = &rqstp->rq_arg; > __be32 *p, status; > struct cb_process_state cps = { > .drc_status = 0, > @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > > dprintk("%s: start\n", __func__); > > - xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base); > + rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len; This is redundant. > + xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base); > > p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len); > xdr_init_encode(&xdr_out, &rqstp->rq_res, p); > diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c > index 6255d14..d92cee1 100644 > --- a/net/sunrpc/backchannel_rqst.c > +++ b/net/sunrpc/backchannel_rqst.c > @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) > { > struct rpc_xprt *xprt = req->rq_xprt; > struct svc_serv *bc_serv = xprt->bc_serv; > + struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf; > > spin_lock(&xprt->bc_pa_lock); > list_del(&req->rq_bc_pa_list); > xprt_dec_alloc_count(xprt, 1); > spin_unlock(&xprt->bc_pa_lock); > > + if (copied <= rq_rcv_buf->head[0].iov_len) { > + rq_rcv_buf->head[0].iov_len = copied; > + rq_rcv_buf->page_len = 0; > + } else { > + rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len; > + } > + There is a problem with this approach: req->rq_rcv_buf is never reset, and so NFSv4 callbacks are now broken. The correct fix for this issue is to do the above adjustment in bc_svc_process(), so that it only applies to the resulting rqstp->rq_arg. > req->rq_private_buf.len = copied; > set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state); > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > index a8f579d..af9f987 100644 > --- a/net/sunrpc/svc.c > +++ b/net/sunrpc/svc.c > @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, > memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen); > memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg)); > memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res)); > + rqstp->rq_arg.len = req->rq_private_buf.len; Cheers Trond -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 7 Dec 2015, Trond Myklebust wrote: > Hi Ben, > > I've hit a few problems with this patch after it went upstream. > > On Fri, Nov 20, 2015 at 6:55 AM, Benjamin Coddington > <bcodding@redhat.com> wrote: > > A truncated cb_compound request will cause the client to decode null or > > data from a previous callback for nfs4.1 backchannel case, or uninitialized > > data for the nfs4.0 case. This is because the path through > > svc_process_common() advances the request's iov_base and decrements iov_len > > without adjusting the overall xdr_buf's len field. That causes > > xdr_init_decode() to set up the xdr_stream with an incorrect length in > > nfs4_callback_compound(). > > > > Fixing this for the nfs4.1 backchannel case first requires setting the > > correct iov_len and page_len based on the length of received data in the > > same manner as the nfs4.0 case. > > > > Then the request's xdr_buf length can be adjusted for both cases based upon > > the remaining iov_len and page_len. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfs/callback_xdr.c | 7 +++++-- > > net/sunrpc/backchannel_rqst.c | 8 ++++++++ > > net/sunrpc/svc.c | 1 + > > 3 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c > > index 6b1697a..1c8213e 100644 > > --- a/fs/nfs/callback_xdr.c > > +++ b/fs/nfs/callback_xdr.c > > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) > > > > p = xdr_inline_decode(xdr, nbytes); > > if (unlikely(p == NULL)) > > - printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n"); > > + printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed " > > + "or truncated request.\n"); > > return p; > > } > > > > @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > > struct cb_compound_hdr_arg hdr_arg = { 0 }; > > struct cb_compound_hdr_res hdr_res = { NULL }; > > struct xdr_stream xdr_in, xdr_out; > > + struct xdr_buf *rq_arg = &rqstp->rq_arg; > > __be32 *p, status; > > struct cb_process_state cps = { > > .drc_status = 0, > > @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r > > > > dprintk("%s: start\n", __func__); > > > > - xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base); > > + rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len; > > This is redundant. The decoding routines in svc_process_common() and svc_authenticate() are advancing the xdr_buf's iov_base and decremening iov_len without adjusting the overall xdr_buf.len field, so I believed this adjustment was necessary after they had moved things around. I'm simulating this problem by sending CB_COMPOUND instead of CB_NULL when setting up the callback channel (since that is easy to do in pynfs). In that scenario, just before xdr_init_decode() my xdr_buf.len is 80 (the size of the RPC headers), but the xdr_buf.kvec iov_len is 0 (all the headers have been decoded). Then xdr_init_decode() then skips xdr_set_iov() so xdr->end is never initialized, and xdr->nwords ends up at 20. Then, as we start decoding, we end up in __xdr_inline_decode(), which expects xdr->end to have a valid value if we haven't run out of nwords, or we start decoding a previous request or uninitialized data. Usually, this is harmless, as the string decode overflows those 20 words, and NULL is returned in my test; however I believe it is possible to decode part of a previous request here.. With this adjustment, nwords ends up at 0, so __xdr_inline_decode() returns before using xdr->end. Maybe the right fix is to adjust xdr_init_decode() to make sure xdr->end is always initialized based on iov_len. Let me know if it would be helpful for me to provide exactly the steps to reproduce my test. > > + xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base); > > > > p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len); > > xdr_init_encode(&xdr_out, &rqstp->rq_res, p); > > diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c > > index 6255d14..d92cee1 100644 > > --- a/net/sunrpc/backchannel_rqst.c > > +++ b/net/sunrpc/backchannel_rqst.c > > @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) > > { > > struct rpc_xprt *xprt = req->rq_xprt; > > struct svc_serv *bc_serv = xprt->bc_serv; > > + struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf; > > > > spin_lock(&xprt->bc_pa_lock); > > list_del(&req->rq_bc_pa_list); > > xprt_dec_alloc_count(xprt, 1); > > spin_unlock(&xprt->bc_pa_lock); > > > > + if (copied <= rq_rcv_buf->head[0].iov_len) { > > + rq_rcv_buf->head[0].iov_len = copied; > > + rq_rcv_buf->page_len = 0; > > + } else { > > + rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len; > > + } > > + > > There is a problem with this approach: req->rq_rcv_buf is never reset, > and so NFSv4 callbacks are now broken. > The correct fix for this issue is to do the above adjustment in > bc_svc_process(), so that it only applies to the resulting > rqstp->rq_arg. I assumed it was being reset, and my test didn't catch it. Very sorry for the trouble, Trond. It makes much more sense to adjust the service's rqstp after the copy. Ben > > req->rq_private_buf.len = copied; > > set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state); > > > > diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c > > index a8f579d..af9f987 100644 > > --- a/net/sunrpc/svc.c > > +++ b/net/sunrpc/svc.c > > @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, > > memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen); > > memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg)); > > memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res)); > > + rqstp->rq_arg.len = req->rq_private_buf.len; > > Cheers > Trond > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c index 6b1697a..1c8213e 100644 --- a/fs/nfs/callback_xdr.c +++ b/fs/nfs/callback_xdr.c @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_stream *xdr, int nbytes) p = xdr_inline_decode(xdr, nbytes); if (unlikely(p == NULL)) - printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed!\n"); + printk(KERN_WARNING "NFS: NFSv4 callback reply buffer overflowed " + "or truncated request.\n"); return p; } @@ -892,6 +893,7 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r struct cb_compound_hdr_arg hdr_arg = { 0 }; struct cb_compound_hdr_res hdr_res = { NULL }; struct xdr_stream xdr_in, xdr_out; + struct xdr_buf *rq_arg = &rqstp->rq_arg; __be32 *p, status; struct cb_process_state cps = { .drc_status = 0, @@ -903,7 +905,8 @@ static __be32 nfs4_callback_compound(struct svc_rqst *rqstp, void *argp, void *r dprintk("%s: start\n", __func__); - xdr_init_decode(&xdr_in, &rqstp->rq_arg, rqstp->rq_arg.head[0].iov_base); + rq_arg->len = rq_arg->head[0].iov_len + rq_arg->page_len; + xdr_init_decode(&xdr_in, rq_arg, rq_arg->head[0].iov_base); p = (__be32*)((char *)rqstp->rq_res.head[0].iov_base + rqstp->rq_res.head[0].iov_len); xdr_init_encode(&xdr_out, &rqstp->rq_res, p); diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c index 6255d14..d92cee1 100644 --- a/net/sunrpc/backchannel_rqst.c +++ b/net/sunrpc/backchannel_rqst.c @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc_rqst *req, uint32_t copied) { struct rpc_xprt *xprt = req->rq_xprt; struct svc_serv *bc_serv = xprt->bc_serv; + struct xdr_buf *rq_rcv_buf = &req->rq_rcv_buf; spin_lock(&xprt->bc_pa_lock); list_del(&req->rq_bc_pa_list); xprt_dec_alloc_count(xprt, 1); spin_unlock(&xprt->bc_pa_lock); + if (copied <= rq_rcv_buf->head[0].iov_len) { + rq_rcv_buf->head[0].iov_len = copied; + rq_rcv_buf->page_len = 0; + } else { + rq_rcv_buf->page_len = copied - rq_rcv_buf->head[0].iov_len; + } + req->rq_private_buf.len = copied; set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state); diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c index a8f579d..af9f987 100644 --- a/net/sunrpc/svc.c +++ b/net/sunrpc/svc.c @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req, memcpy(&rqstp->rq_addr, &req->rq_xprt->addr, rqstp->rq_addrlen); memcpy(&rqstp->rq_arg, &req->rq_rcv_buf, sizeof(rqstp->rq_arg)); memcpy(&rqstp->rq_res, &req->rq_snd_buf, sizeof(rqstp->rq_res)); + rqstp->rq_arg.len = req->rq_private_buf.len; /* reset result send buffer "put" position */ resv->iov_len = 0;
A truncated cb_compound request will cause the client to decode null or data from a previous callback for nfs4.1 backchannel case, or uninitialized data for the nfs4.0 case. This is because the path through svc_process_common() advances the request's iov_base and decrements iov_len without adjusting the overall xdr_buf's len field. That causes xdr_init_decode() to set up the xdr_stream with an incorrect length in nfs4_callback_compound(). Fixing this for the nfs4.1 backchannel case first requires setting the correct iov_len and page_len based on the length of received data in the same manner as the nfs4.0 case. Then the request's xdr_buf length can be adjusted for both cases based upon the remaining iov_len and page_len. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/callback_xdr.c | 7 +++++-- net/sunrpc/backchannel_rqst.c | 8 ++++++++ net/sunrpc/svc.c | 1 + 3 files changed, 14 insertions(+), 2 deletions(-)