On Tue, 07 Jun 2022, J. Bruce Fields wrote:
> On Sun, Jun 05, 2022 at 03:58:25PM -0400, Chuck Lever wrote:
> > I found that NFSD's new NFSv3 READDIRPLUS XDR encoder was screwing up
> > right at the end of the page array. xdr_get_next_encode_buffer() does
> > not compute the value of xdr->end correctly:
> >
> > * The check to see if we're on the final available page in xdr->buf
> > needs to account for the space consumed by @nbytes.
> >
> > * The new xdr->end value needs to account for the portion of @nbytes
> > that is to be encoded into the previous buffer.
> >
> > Fixes: 2825a7f90753 ("nfsd4: allow encoding across page boundaries")
> > Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > ---
> > net/sunrpc/xdr.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index df194cc07035..b57cf9df4de8 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -979,7 +979,11 @@ static __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr,
> > */
> > xdr->p = (void *)p + frag2bytes;
> > space_left = xdr->buf->buflen - xdr->buf->len;
> > - xdr->end = (void *)p + min_t(int, space_left, PAGE_SIZE);
> > + if (space_left - nbytes >= PAGE_SIZE)
> > + xdr->end = (void *)p + PAGE_SIZE;
> > + else
> > + xdr->end = (void *)p + space_left - frag1bytes;
> > +
>
> I think that's right.
I think I agree.
>
> Couldn't you just make that
>
>
> - xdr->end = (void *)p + min_t(int, space_left, PAGE_SIZE);
> + xdr->end = (void *)p + min_t(int, space_left - nbytes, PAGE_SIZE);
I don't think that is the same.
When space_left is small, this results in "p + space_left - nbytes" but
the one we both this is right results in "p + space_left - frag1bytes".
I'm going to suggest a more radical change.
Let's start off with
int space_avail = xdr->buf->buflen - xdr->buf->len;
In this function we sometime care about space before we consume any, and
something care about space after we consume some. "space_left" sounds
more like the latter. "space_avail" sounds more like the former.
Current space_left is assigned to the former, which I find confused.
Then the second "if" which Bruce highlighted becomes
if (nbytes > space_avail)
goto out_overflow;
which is obviously correct.
We then assign frag{1,2}bytes and have another chunk of code that looks
wrong to me. I'd like
if (xdr->iov) {
xdr->iov->iov_len += frag1bytes;
xdr->iov = NULL;
} else {
xdr->buf->page_len += frag1bytes;
xdr->page_ptr++;
}
Note that this changes the code NOT to increment pagE_ptr if iov was not
NULL. I cannot see how it would be correct to do that. Presumably this
code is never called with iov != NULL ???
Now I want to rearrange the assignments at the end:
xdr->p = (void *)p + frag2bytes;
xdr->buf->page_len += frag2bytes;
xdr->buf->len += nbytes;
space_left = xdr->buf->buflen - xdr->buf->len;
xdr->end = (void *)xdr->p + min_t(int, space_left, PAGE_SIZE);
Note that the last line "xdr->p" in place of "p".
We still have "space_left", but it now is the space that is left
after we have consumed some.
Possibly the space_left line could be
space_left -= nbytes;
NeilBrown
>
> ?
>
> (Note we know space_left >= nbytes from the second "if" of this
> function.)
>
> No strong opinion either way, really, I just wonder if I'm missing
> something.
>
> --b.
>
> > xdr->buf->page_len += frag2bytes;
> > xdr->buf->len += nbytes;
> > return p;
> >
>
