From: Trond Myklebust <[email protected]>
If the server returns more data than we have buffer space for, then
we need to truncate and exit early.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs42xdr.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c
index 9ef5261a1a70..8386ca45a43f 100644
--- a/fs/nfs/nfs42xdr.c
+++ b/fs/nfs/nfs42xdr.c
@@ -1026,6 +1026,7 @@ static int decode_deallocate(struct xdr_stream *xdr, struct nfs42_falloc_res *re
}
static int decode_read_plus_data(struct xdr_stream *xdr,
+ struct nfs_pgio_args *args,
struct nfs_pgio_res *res)
{
uint32_t count, recvd;
@@ -1041,8 +1042,12 @@ static int decode_read_plus_data(struct xdr_stream *xdr,
recvd = xdr_align_data(xdr, res->count, xdr_align_size(count));
if (recvd > count)
recvd = count;
+ if (res->count + recvd > args->count) {
+ if (args->count > res->count)
+ res->count += args->count - res->count;
+ return 1;
+ }
res->count += recvd;
-
if (count > recvd)
return 1;
return 0;
@@ -1119,7 +1124,7 @@ static int decode_read_plus(struct xdr_stream *xdr, struct nfs_pgio_res *res)
type = be32_to_cpup(p++);
if (type == NFS4_CONTENT_DATA)
- status = decode_read_plus_data(xdr, res);
+ status = decode_read_plus_data(xdr, args, res);
else if (type == NFS4_CONTENT_HOLE)
status = decode_read_plus_hole(xdr, args, res, &eof);
else
--
2.29.2
From: Trond Myklebust <[email protected]>
Ensure that we encode the data payload + padding, and that we truncate
the preallocated buffer to the actual read size.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4xdr.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 833a2c64dfe8..26f6e277101d 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp,
resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof);
if (nfserr)
return nfserr;
+ xdr_truncate_encode(xdr, starting_len + 16 + xdr_align_size(*maxcount));
tmp = htonl(NFS4_CONTENT_DATA);
write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4);
@@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp,
write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64, 8);
tmp = htonl(*maxcount);
write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp, 4);
+
+ tmp = xdr_zero;
+ write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + *maxcount, &tmp,
+ xdr_pad_size(*maxcount));
return nfs_ok;
}
--
2.29.2
From: Trond Myklebust <[email protected]>
If the READ_PLUS operation was truncated due to an error, then ensure we
clear the 'eof' flag.
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfsd/nfs4xdr.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 26f6e277101d..5f5169b9c2e9 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -4736,14 +4736,15 @@ nfsd4_encode_read_plus(struct nfsd4_compoundres *resp, __be32 nfserr,
if (nfserr && segments == 0)
xdr_truncate_encode(xdr, starting_len);
else {
- tmp = htonl(eof);
- write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4);
- tmp = htonl(segments);
- write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4);
if (nfserr) {
xdr_truncate_encode(xdr, last_segment);
nfserr = nfs_ok;
+ eof = 0;
}
+ tmp = htonl(eof);
+ write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4);
+ tmp = htonl(segments);
+ write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp, 4);
}
return nfserr;
--
2.29.2
Hey-
> On Dec 9, 2020, at 9:48 AM, [email protected] wrote:
>
> From: Trond Myklebust <[email protected]>
>
> Ensure that we encode the data payload + padding, and that we truncate
> the preallocated buffer to the actual read size.
Did you intend to merge 15/16 and 16/16 through your tree?
Can the patch descriptions say a little more about why these
changes are necessary? If they fix a misbehavior, describe
the problem.
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfsd/nfs4xdr.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 833a2c64dfe8..26f6e277101d 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp,
> resp->rqstp->rq_vec, read->rd_vlen, maxcount, eof);
> if (nfserr)
> return nfserr;
> + xdr_truncate_encode(xdr, starting_len + 16 + xdr_align_size(*maxcount));
>
> tmp = htonl(NFS4_CONTENT_DATA);
> write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp, 4);
> @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct nfsd4_compoundres *resp,
> write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64, 8);
> tmp = htonl(*maxcount);
> write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp, 4);
> +
> + tmp = xdr_zero;
> + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 + *maxcount, &tmp,
> + xdr_pad_size(*maxcount));
> return nfs_ok;
> }
>
> --
> 2.29.2
>
--
Chuck Lever
[email protected]
On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote:
> Hey-
>
> > On Dec 9, 2020, at 9:48 AM, [email protected] wrote:
> >
> > From: Trond Myklebust <[email protected]>
> >
> > Ensure that we encode the data payload + padding, and that we
> > truncate
> > the preallocated buffer to the actual read size.
>
> Did you intend to merge 15/16 and 16/16 through your tree?
No. They can go through the nfsd tree. I included them here because
they are necessary in order to pass the xfstests.
> Can the patch descriptions say a little more about why these
> changes are necessary? If they fix a misbehavior, describe
> the problem.
It's the same problem and solution that exists in the READ code.
nfsd_readv() doesn't necessarily return the same number of bytes that
we requested and preallocated buffer space for. So to deal with that,
we have to truncate the preallocated buffer.
Finally, we have to write zeros into the padding bytes after the read
buffer.
> > Signed-off-by: Trond Myklebust <[email protected]>
> > ---
> > fs/nfsd/nfs4xdr.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 833a2c64dfe8..26f6e277101d 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct
> > nfsd4_compoundres *resp,
> > resp->rqstp->rq_vec, read->rd_vlen,
> > maxcount, eof);
> > if (nfserr)
> > return nfserr;
> > + xdr_truncate_encode(xdr, starting_len + 16 +
> > xdr_align_size(*maxcount));
> >
> > tmp = htonl(NFS4_CONTENT_DATA);
> > write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp,
> > 4);
> > @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct
> > nfsd4_compoundres *resp,
> > write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64,
> > 8);
> > tmp = htonl(*maxcount);
> > write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp,
> > 4);
> > +
> > + tmp = xdr_zero;
> > + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 +
> > *maxcount, &tmp,
> > + xdr_pad_size(*maxcount));
> > return nfs_ok;
> > }
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
> On Dec 9, 2020, at 11:39 AM, Trond Myklebust <[email protected]> wrote:
>
> On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote:
>> Hey-
>>
>>> On Dec 9, 2020, at 9:48 AM, [email protected] wrote:
>>>
>>> From: Trond Myklebust <[email protected]>
>>>
>>> Ensure that we encode the data payload + padding, and that we
>>> truncate
>>> the preallocated buffer to the actual read size.
>>
>> Did you intend to merge 15/16 and 16/16 through your tree?
>
> No. They can go through the nfsd tree. I included them here because
> they are necessary in order to pass the xfstests.
Would it be OK if they went in 5.11-rc? I've got the initial
merge tag prepared already. If they can't wait, let me know.
>> Can the patch descriptions say a little more about why these
>> changes are necessary? If they fix a misbehavior, describe
>> the problem.
>
> It's the same problem and solution that exists in the READ code.
>
> nfsd_readv() doesn't necessarily return the same number of bytes that
> we requested and preallocated buffer space for. So to deal with that,
> we have to truncate the preallocated buffer.
Huh. I thought it was doing that already? Oh, that's just for
the cases where the server returns an error status. The
READ_PLUS encoder incorrectly does not do that truncation for
short READs... got it.
> Finally, we have to write zeros into the padding bytes after the read
> buffer.
Right. Then the problem statement is that the server's READ_PLUS
XDR encoder isn't padding the read buffer properly.
Quibble: perhaps these are two separate issues that each deserve
their own patches with Fixes: tags (and if you re-post these,
please add a Fixes: tag to 16/16 too).
Thanks!
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> ---
>>> fs/nfsd/nfs4xdr.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index 833a2c64dfe8..26f6e277101d 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -4632,6 +4632,7 @@ nfsd4_encode_read_plus_data(struct
>>> nfsd4_compoundres *resp,
>>> resp->rqstp->rq_vec, read->rd_vlen,
>>> maxcount, eof);
>>> if (nfserr)
>>> return nfserr;
>>> + xdr_truncate_encode(xdr, starting_len + 16 +
>>> xdr_align_size(*maxcount));
>>>
>>> tmp = htonl(NFS4_CONTENT_DATA);
>>> write_bytes_to_xdr_buf(xdr->buf, starting_len, &tmp,
>>> 4);
>>> @@ -4639,6 +4640,10 @@ nfsd4_encode_read_plus_data(struct
>>> nfsd4_compoundres *resp,
>>> write_bytes_to_xdr_buf(xdr->buf, starting_len + 4, &tmp64,
>>> 8);
>>> tmp = htonl(*maxcount);
>>> write_bytes_to_xdr_buf(xdr->buf, starting_len + 12, &tmp,
>>> 4);
>>> +
>>> + tmp = xdr_zero;
>>> + write_bytes_to_xdr_buf(xdr->buf, starting_len + 16 +
>>> *maxcount, &tmp,
>>> + xdr_pad_size(*maxcount));
>>> return nfs_ok;
>>> }
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> [email protected]
--
Chuck Lever
[email protected]
On Wed, 2020-12-09 at 11:57 -0500, Chuck Lever wrote:
>
>
> > On Dec 9, 2020, at 11:39 AM, Trond Myklebust <
> > [email protected]> wrote:
> >
> > On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote:
> > > Hey-
> > >
> > > > On Dec 9, 2020, at 9:48 AM, [email protected] wrote:
> > > >
> > > > From: Trond Myklebust <[email protected]>
> > > >
> > > > Ensure that we encode the data payload + padding, and that we
> > > > truncate
> > > > the preallocated buffer to the actual read size.
> > >
> > > Did you intend to merge 15/16 and 16/16 through your tree?
> >
> > No. They can go through the nfsd tree. I included them here because
> > they are necessary in order to pass the xfstests.
>
> Would it be OK if they went in 5.11-rc? I've got the initial
> merge tag prepared already. If they can't wait, let me know.
I'm fine with that.
>
>
> > > Can the patch descriptions say a little more about why these
> > > changes are necessary? If they fix a misbehavior, describe
> > > the problem.
> >
> > It's the same problem and solution that exists in the READ code.
> >
> > nfsd_readv() doesn't necessarily return the same number of bytes
> > that
> > we requested and preallocated buffer space for. So to deal with
> > that,
> > we have to truncate the preallocated buffer.
>
> Huh. I thought it was doing that already? Oh, that's just for
> the cases where the server returns an error status. The
> READ_PLUS encoder incorrectly does not do that truncation for
> short READs... got it.
>
>
> > Finally, we have to write zeros into the padding bytes after the
> > read
> > buffer.
>
> Right. Then the problem statement is that the server's READ_PLUS
> XDR encoder isn't padding the read buffer properly.
>
> Quibble: perhaps these are two separate issues that each deserve
> their own patches with Fixes: tags (and if you re-post these,
> please add a Fixes: tag to 16/16 too).
I'm not planning on reposting.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]