On Sun, 2019-09-15 at 12:43 -0400, Chuck Lever wrote:
> Hi Ben-
>
>
> > On Sep 15, 2019, at 11:41 AM, Benjamin Coddington <
> > bcodding@xxxxxxxxxx> wrote:
> >
> > The GSS Message Integrity Check data for krb5i may lie partially in
> > the XDR
> > reply buffer's pages and tail. If so, we try to copy the entire
> > MIC into
> > free space in the tail. But as the estimations of the slack space
> > required
> > for authentication and verification have improved there may be less
> > free
> > space in the tail to complete this copy -- see commit 2c94b8eca1a2
> > ("SUNRPC: Use au_rslack when computing reply buffer size"). In
> > fact, there
> > may only be room in the tail for a single copy of the MIC, and not
> > part of
> > the MIC and then another complete copy.
> >
> > The real world failure reported is that `ls` of a directory on NFS
> > may
> > sometimes return -EIO, which can be traced back to
> > xdr_buf_read_netobj()
> > failing to find available free space in the tail to copy the MIC.
> >
> > Fix this by checking for the case of the MIC crossing the
> > boundaries of
> > head, pages, and tail. If so, shift the buffer until the MIC is
> > contained
> > completely within the pages or tail. This allows the remainder of
> > the
> > function to create a sub buffer that directly address the complete
> > MIC.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx # v5.1
> > ---
> > net/sunrpc/xdr.c | 32 +++++++++++++++++++-------------
> > 1 file changed, 19 insertions(+), 13 deletions(-)
> >
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 48c93b9e525e..a29ce73c3029 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -1237,16 +1237,29 @@ xdr_encode_word(struct xdr_buf *buf,
> > unsigned int base, u32 obj)
> > EXPORT_SYMBOL_GPL(xdr_encode_word);
> >
> > /* If the netobj starting offset bytes from the start of xdr_buf is
> > contained
> > - * entirely in the head or the tail, set object to point to it;
> > otherwise
> > - * try to find space for it at the end of the tail, copy it there,
> > and
> > - * set obj to point to it. */
> > + * entirely in the head, pages, or tail, set object to point to
> > it; otherwise
> > + * shift the buffer until it is contained entirely within the
> > pages or tail.
> > + */
> > int xdr_buf_read_netobj(struct xdr_buf *buf, struct xdr_netobj
> > *obj, unsigned int offset)
> > {
> > struct xdr_buf subbuf;
> > + unsigned int boundary;
> >
> > if (xdr_decode_word(buf, offset, &obj->len))
> > return -EFAULT;
> > - if (xdr_buf_subsegment(buf, &subbuf, offset + 4, obj->len))
> > + offset += 4;
> > +
> > + /* Is the obj partially in the head? */
> > + boundary = buf->head[0].iov_len;
> > + if (offset < boundary && (offset + obj->len) > boundary)
> > + xdr_shift_buf(buf, boundary - offset);
> > +
> > + /* Is the obj partially in the pages? */
> > + boundary += buf->page_len;
> > + if (offset < boundary && (offset + obj->len) > boundary)
> > + xdr_shrink_pagelen(buf, boundary - offset);
> > +
> > + if (xdr_buf_subsegment(buf, &subbuf, offset, obj->len))
> > return -EFAULT;
> >
> > /* Is the obj contained entirely in the head? */
> > @@ -1258,17 +1271,10 @@ int xdr_buf_read_netobj(struct xdr_buf
> > *buf, struct xdr_netobj *obj, unsigned in
> > if (subbuf.tail[0].iov_len == obj->len)
> > return 0;
> >
> > - /* use end of tail as storage for obj:
> > - * (We don't copy to the beginning because then we'd have
> > - * to worry about doing a potentially overlapping copy.
> > - * This assumes the object is at most half the length of the
> > - * tail.) */
> > + /* obj is in the pages: move to end of the tail */
>
> How about "/* Find a contiguous area in @buf to hold all of @obj */"
> ?
>
>
> > if (obj->len > buf->buflen - buf->len)
> > return -ENOMEM;
> > - if (buf->tail[0].iov_len != 0)
> > - obj->data = buf->tail[0].iov_base + buf-
> > >tail[0].iov_len;
> > - else
> > - obj->data = buf->head[0].iov_base + buf-
> > >head[0].iov_len;
> > + obj->data = buf->tail[0].iov_base + buf->tail[0].iov_len;
>
> Your new code assumes that when krb5i is in use, the upper layer will
> always
> provide a non-NULL tail->iov_len. I wouldn't swear that will always
> be true:
> The reason for the "if (buf->tail[0].iov_len)" check is to see
> whether the
> upper layer indeed has set up a tail. iov_len will be non-zero only
> when the
> upper layer has provided a tail buffer.
For the case where we only have kvec data, then xdr_buf_init() assigns
all the allocated buffer to the header, so the mic should be
contiguously buffered there.
If we have page data, then the fact that auth->au_rslack > auth-
>au_ralign should ensure that we have allocated a sufficiently large
tail buffer (see rpc_prepare_reply_pages()).
BTW: this also means we should really only need to move the mic in the
case where buf->page_len != 0.
It also means that we should probably change xdr_inline_pages() to be a
static function, since the assumption above fails if the user does not
call rpc_prepare_reply_pages() in order to calculate the header
alignment correctly.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
