> On Nov 20, 2015, at 11:20 AM, Benjamin Coddington <bcodding@xxxxxxxxxx> wrote:
>
> On Fri, 20 Nov 2015, Chuck Lever wrote:
>
>>
>>> On Nov 20, 2015, at 9:55 AM, Benjamin Coddington <bcodding@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
