diff mbox series

[15/16] nfsd: Fixes for nfsd4_encode_read_plus_data()

Message ID 20201209144801.700778-16-trondmy@kernel.org (mailing list archive)
State New
Headers show
Series Fixes for the NFSv4.2 READ_PLUS operation | expand

Commit Message

trondmy@kernel.org Dec. 9, 2020, 2:48 p.m. UTC
From: Trond Myklebust <trond.myklebust@hammerspace.com>

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 <trond.myklebust@hammerspace.com>
---
 fs/nfsd/nfs4xdr.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Chuck Lever Dec. 9, 2020, 4:16 p.m. UTC | #1
Hey-

> On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote:
> 
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> 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 <trond.myklebust@hammerspace.com>
> ---
> 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
chucklever@gmail.com
Trond Myklebust Dec. 9, 2020, 4:39 p.m. UTC | #2
On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote:
> Hey-
> 
> > On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote:
> > 
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > 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 <trond.myklebust@hammerspace.com>
> > ---
> > 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;
> > }
Chuck Lever Dec. 9, 2020, 4:57 p.m. UTC | #3
> On Dec 9, 2020, at 11:39 AM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote:
>> Hey-
>> 
>>> On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote:
>>> 
>>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>>> 
>>> 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 <trond.myklebust@hammerspace.com>
>>> ---
>>> 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
> trond.myklebust@hammerspace.com

--
Chuck Lever
chucklever@gmail.com
Trond Myklebust Dec. 9, 2020, 5:01 p.m. UTC | #4
On Wed, 2020-12-09 at 11:57 -0500, Chuck Lever wrote:
> 
> 
> > On Dec 9, 2020, at 11:39 AM, Trond Myklebust <
> > trondmy@hammerspace.com> wrote:
> > 
> > On Wed, 2020-12-09 at 11:16 -0500, Chuck Lever wrote:
> > > Hey-
> > > 
> > > > On Dec 9, 2020, at 9:48 AM, trondmy@kernel.org wrote:
> > > > 
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > 
> > > > 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.
diff mbox series

Patch

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;
 }