Re: Patch "nfs4: limit callback decoding to received bytes" has been added to the 4.3-stable tree

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Dec 11, 2015 at 06:14:03PM -0500, Trond Myklebust wrote:
> Hi Greg,
> 
> On Fri, Dec 11, 2015 at 12:20 PM,  <gregkh@linuxfoundation.org> wrote:
> >
> > This is a note to let you know that I've just added the patch titled
> >
> >     nfs4: limit callback decoding to received bytes
> >
> > to the 4.3-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> >
> > The filename of the patch is:
> >      nfs4-limit-callback-decoding-to-received-bytes.patch
> > and it can be found in the queue-4.3 subdirectory.
> >
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> >
> >
> > From 38b7631fbe42e6e247e9fc9879f961b14a687e3b Mon Sep 17 00:00:00 2001
> > From: Benjamin Coddington <bcodding@redhat.com>
> > Date: Fri, 20 Nov 2015 09:55:30 -0500
> > Subject: nfs4: limit callback decoding to received bytes
> >
> > From: Benjamin Coddington <bcodding@redhat.com>
> >
> > commit 38b7631fbe42e6e247e9fc9879f961b14a687e3b upstream.
> >
> > 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>
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > ---
> >  fs/nfs/callback_xdr.c         |    7 +++++--
> >  net/sunrpc/backchannel_rqst.c |    8 ++++++++
> >  net/sunrpc/svc.c              |    1 +
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> >
> > --- a/fs/nfs/callback_xdr.c
> > +++ b/fs/nfs/callback_xdr.c
> > @@ -76,7 +76,8 @@ static __be32 *read_buf(struct xdr_strea
> >
> >         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(str
> >         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(str
> >
> >         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);
> > --- a/net/sunrpc/backchannel_rqst.c
> > +++ b/net/sunrpc/backchannel_rqst.c
> > @@ -333,12 +333,20 @@ void xprt_complete_bc_request(struct rpc
> >  {
> >         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);
> >
> > --- a/net/sunrpc/svc.c
> > +++ b/net/sunrpc/svc.c
> > @@ -1363,6 +1363,7 @@ bc_svc_process(struct svc_serv *serv, st
> >         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;
> >
> 
> This patch turned out to cause some issues with the NFS callback
> channel. I have a fix, but it hasn't been upstreamed yet. Can we
> therefore please hold on this patch for now, and remove it from all
> the stable queues. I'll resubmit when the fix is in mainline.

No problem, now dropped from the 4.3 and 4.2 stable queues.

thanks,

greg k-h