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 <[email protected]>
---
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;
+
xdr->buf->page_len += frag2bytes;
xdr->buf->len += nbytes;
return p;
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 <[email protected]>
> ---
> 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.
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);
?
(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;
>
> On Jun 6, 2022, at 8:59 PM, NeilBrown <[email protected]> wrote:
>
> 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 <[email protected]>
>>> ---
>>> 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".
Exactly.
> 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.
IIUC, it's correct only if space_avail is an unsigned integer type.
Otherwise, comparison of space_avail with a size_t will cause it
to get converted to an unsigned integer, risking an underflow.
I've toyed with the idea of changing all of these signed integer
local variables to size_t, since they are used for pointer
arithmetic. Bruce has taught me to be wary about that kind of
change, however.
> 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 ???
That strikes me as a good change. I will add it to this series as
another patch.
Yes, this code is called by the server's READDIR encoder with iov = NULL.
See nfsd3_init_dirlist_pages().
> 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;
This part of the code can become too clever by half very quickly.
Since this function is called infrequently, we can afford to err
on the side of easy readability. The way it is after my patch seems
straightforward to me, but I will keep your suggestion in mind.
> 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;
>>>
>>
--
Chuck Lever
On Tue, 07 Jun 2022, Chuck Lever III wrote:
> > On Jun 6, 2022, at 10:35 PM, Chuck Lever III <[email protected]> wrote:
> >
> >> On Jun 6, 2022, at 8:59 PM, NeilBrown <[email protected]> wrote:
> >> 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 ???
> >
> > That strikes me as a good change. I will add it to this series as
> > another patch.
> >
> > Yes, this code is called by the server's READDIR encoder with iov = NULL.
> > See nfsd3_init_dirlist_pages().
>
> This change breaks READDIR, looks like.
Thanks for testing...
I looked again, and I see that svcxdr_init_encode() initialises
->page_ptr to "buf->page - 1". So we need a +1 when moving from the
'head' to the pages.
Commit 05638dc73af2 ("nfsd4: simplify server xdr->next_page use")
explains why. I'm not sure I completely agree with the reasoning (head
really is a special case), but it probably isn't worth "fixing".
Thanks,
NeilBrown
>
> --
> Chuck Lever
>
>
>
>
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 <[email protected]>
> > ---
> > 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;
> >
>
> On Jun 6, 2022, at 10:35 PM, Chuck Lever III <[email protected]> wrote:
>
>> On Jun 6, 2022, at 8:59 PM, NeilBrown <[email protected]> wrote:
>> 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 ???
>
> That strikes me as a good change. I will add it to this series as
> another patch.
>
> Yes, this code is called by the server's READDIR encoder with iov = NULL.
> See nfsd3_init_dirlist_pages().
This change breaks READDIR, looks like.
--
Chuck Lever