[PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[Chuck Lever]

The Linux NFS server currently responds to a zero-length NFSv3 WRITE
request with NFS3ERR_IO. It responds to a zero-length NFSv4 WRITE
with NFS4_OK and count of zero.

RFC 1813 says of the WRITE procedure's @count argument:

count
         The number of bytes of data to be written. If count is
         0, the WRITE will succeed and return a count of 0,
         barring errors due to permissions checking.

RFC 8881 has similar language for NFSv4, though NFSv4 removed the
explicit @count argument because that value is already contained in
the opaque payload array.

The synthetic client pynfs's WRT4 and WRT15 tests do emit zero-
length WRITEs to exercise this spec requirement, but interestingly
the Linux NFS client does not appear to emit zero-length WRITEs,
instead squelching them.

I'm not aware of a test that can generate such WRITEs for NFSv3, so
I wrote a naive C program to generate a zero-length WRITE and test
this fix.

Fixes: 14168d678a0f ("NFSD: Remove the RETURN_STATUS() macro")
Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com>
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

Here's an alternate approach to addressing the zero-length NFSv3
WRITE failures.


 fs/nfsd/nfs3proc.c |    6 +-----
 fs/nfsd/nfsproc.c  |    5 -----
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 4418517f6f12..2c681785186f 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -202,15 +202,11 @@ nfsd3_proc_write(struct svc_rqst *rqstp)
 	fh_copy(&resp->fh, &argp->fh);
 	resp->committed = argp->stable;
 	nvecs = svc_fill_write_vector(rqstp, &argp->payload);
-	if (!nvecs) {
-		resp->status = nfserr_io;
-		goto out;
-	}
+
 	resp->status = nfsd_write(rqstp, &resp->fh, argp->offset,
 				  rqstp->rq_vec, nvecs, &cnt,
 				  resp->committed, resp->verf);
 	resp->count = cnt;
-out:
 	return rpc_success;
 }
 
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index eea5b59b6a6c..1743ed04197e 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -235,10 +235,6 @@ nfsd_proc_write(struct svc_rqst *rqstp)
 		argp->len, argp->offset);
 
 	nvecs = svc_fill_write_vector(rqstp, &argp->payload);
-	if (!nvecs) {
-		resp->status = nfserr_io;
-		goto out;
-	}
 
 	resp->status = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh),
 				  argp->offset, rqstp->rq_vec, nvecs,
@@ -247,7 +243,6 @@ nfsd_proc_write(struct svc_rqst *rqstp)
 		resp->status = fh_getattr(&resp->fh, &resp->stat);
 	else if (resp->status == nfserr_jukebox)
 		return rpc_drop_reply;
-out:
 	return rpc_success;
 }
 

Re: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[J. Bruce Fields]

On Tue, Dec 21, 2021 at 10:23:25AM -0500, Chuck Lever wrote:
> The Linux NFS server currently responds to a zero-length NFSv3 WRITE
> request with NFS3ERR_IO. It responds to a zero-length NFSv4 WRITE
> with NFS4_OK and count of zero.
> 
> RFC 1813 says of the WRITE procedure's @count argument:
> 
> count
>          The number of bytes of data to be written. If count is
>          0, the WRITE will succeed and return a count of 0,
>          barring errors due to permissions checking.
> 
> RFC 8881 has similar language for NFSv4, though NFSv4 removed the
> explicit @count argument because that value is already contained in
> the opaque payload array.
> 
> The synthetic client pynfs's WRT4 and WRT15 tests do emit zero-
> length WRITEs to exercise this spec requirement, but interestingly
> the Linux NFS client does not appear to emit zero-length WRITEs,
> instead squelching them.
> 
> I'm not aware of a test that can generate such WRITEs for NFSv3, so
> I wrote a naive C program to generate a zero-length WRITE and test
> this fix.

I know it's probably only a few lines, but still may be worth posting
somewhere and making it the start of a collection of protocol-level v3
tests.

--b.

> 
> Fixes: 14168d678a0f ("NFSD: Remove the RETURN_STATUS() macro")
> Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> Here's an alternate approach to addressing the zero-length NFSv3
> WRITE failures.
> 
> 
>  fs/nfsd/nfs3proc.c |    6 +-----
>  fs/nfsd/nfsproc.c  |    5 -----
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index 4418517f6f12..2c681785186f 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -202,15 +202,11 @@ nfsd3_proc_write(struct svc_rqst *rqstp)
>  	fh_copy(&resp->fh, &argp->fh);
>  	resp->committed = argp->stable;
>  	nvecs = svc_fill_write_vector(rqstp, &argp->payload);
> -	if (!nvecs) {
> -		resp->status = nfserr_io;
> -		goto out;
> -	}
> +
>  	resp->status = nfsd_write(rqstp, &resp->fh, argp->offset,
>  				  rqstp->rq_vec, nvecs, &cnt,
>  				  resp->committed, resp->verf);
>  	resp->count = cnt;
> -out:
>  	return rpc_success;
>  }
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index eea5b59b6a6c..1743ed04197e 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -235,10 +235,6 @@ nfsd_proc_write(struct svc_rqst *rqstp)
>  		argp->len, argp->offset);
>  
>  	nvecs = svc_fill_write_vector(rqstp, &argp->payload);
> -	if (!nvecs) {
> -		resp->status = nfserr_io;
> -		goto out;
> -	}
>  
>  	resp->status = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh),
>  				  argp->offset, rqstp->rq_vec, nvecs,
> @@ -247,7 +243,6 @@ nfsd_proc_write(struct svc_rqst *rqstp)
>  		resp->status = fh_getattr(&resp->fh, &resp->stat);
>  	else if (resp->status == nfserr_jukebox)
>  		return rpc_drop_reply;
> -out:
>  	return rpc_success;
>  }
>  

Re: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[Chuck Lever III]

> On Jan 3, 2022, at 4:00 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> 
> On Tue, Dec 21, 2021 at 10:23:25AM -0500, Chuck Lever wrote:
>> The Linux NFS server currently responds to a zero-length NFSv3 WRITE
>> request with NFS3ERR_IO. It responds to a zero-length NFSv4 WRITE
>> with NFS4_OK and count of zero.
>> 
>> RFC 1813 says of the WRITE procedure's @count argument:
>> 
>> count
>>         The number of bytes of data to be written. If count is
>>         0, the WRITE will succeed and return a count of 0,
>>         barring errors due to permissions checking.
>> 
>> RFC 8881 has similar language for NFSv4, though NFSv4 removed the
>> explicit @count argument because that value is already contained in
>> the opaque payload array.
>> 
>> The synthetic client pynfs's WRT4 and WRT15 tests do emit zero-
>> length WRITEs to exercise this spec requirement, but interestingly
>> the Linux NFS client does not appear to emit zero-length WRITEs,
>> instead squelching them.
>> 
>> I'm not aware of a test that can generate such WRITEs for NFSv3, so
>> I wrote a naive C program to generate a zero-length WRITE and test
>> this fix.
> 
> I know it's probably only a few lines,

It is the same kind of code that Dr. Morris has posted recently.
I've saved it (somewhere). However...


> but still may be worth posting
> somewhere and making it the start of a collection of protocol-level v3
> tests.

... I question whether it's worth posting anything until there
is a framework for collecting and maintaining such things. I
do agree that the community should be working up a set of NFSv3
specific tests like this. I like Frank's idea of making them a
part of pynfs, fwiw.


> --b.
> 
>> 
>> Fixes: 14168d678a0f ("NFSD: Remove the RETURN_STATUS() macro")
>> Reported-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> Here's an alternate approach to addressing the zero-length NFSv3
>> WRITE failures.
>> 
>> 
>> fs/nfsd/nfs3proc.c |    6 +-----
>> fs/nfsd/nfsproc.c  |    5 -----
>> 2 files changed, 1 insertion(+), 10 deletions(-)
>> 
>> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
>> index 4418517f6f12..2c681785186f 100644
>> --- a/fs/nfsd/nfs3proc.c
>> +++ b/fs/nfsd/nfs3proc.c
>> @@ -202,15 +202,11 @@ nfsd3_proc_write(struct svc_rqst *rqstp)
>> 	fh_copy(&resp->fh, &argp->fh);
>> 	resp->committed = argp->stable;
>> 	nvecs = svc_fill_write_vector(rqstp, &argp->payload);
>> -	if (!nvecs) {
>> -		resp->status = nfserr_io;
>> -		goto out;
>> -	}
>> +
>> 	resp->status = nfsd_write(rqstp, &resp->fh, argp->offset,
>> 				  rqstp->rq_vec, nvecs, &cnt,
>> 				  resp->committed, resp->verf);
>> 	resp->count = cnt;
>> -out:
>> 	return rpc_success;
>> }
>> 
>> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
>> index eea5b59b6a6c..1743ed04197e 100644
>> --- a/fs/nfsd/nfsproc.c
>> +++ b/fs/nfsd/nfsproc.c
>> @@ -235,10 +235,6 @@ nfsd_proc_write(struct svc_rqst *rqstp)
>> 		argp->len, argp->offset);
>> 
>> 	nvecs = svc_fill_write_vector(rqstp, &argp->payload);
>> -	if (!nvecs) {
>> -		resp->status = nfserr_io;
>> -		goto out;
>> -	}
>> 
>> 	resp->status = nfsd_write(rqstp, fh_copy(&resp->fh, &argp->fh),
>> 				  argp->offset, rqstp->rq_vec, nvecs,
>> @@ -247,7 +243,6 @@ nfsd_proc_write(struct svc_rqst *rqstp)
>> 		resp->status = fh_getattr(&resp->fh, &resp->stat);
>> 	else if (resp->status == nfserr_jukebox)
>> 		return rpc_drop_reply;
>> -out:
>> 	return rpc_success;
>> }

--
Chuck Lever

Re: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[Bruce Fields]

On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote:
> > but still may be worth posting
> > somewhere and making it the start of a collection of protocol-level v3
> > tests.
> 
> ... I question whether it's worth posting anything until there
> is a framework for collecting and maintaining such things. I
> do agree that the community should be working up a set of NFSv3
> specific tests like this. I like Frank's idea of making them a
> part of pynfs, fwiw.

Somebody did actually do a v3 pynfs that I never got around to merging,
it'd be worth revisiting:

	https://github.com/sthaber/pynfs

--b.

RE: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[Frank Filz]

> On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote:
> > > but still may be worth posting
> > > somewhere and making it the start of a collection of protocol-level
> > > v3 tests.
> >
> > ... I question whether it's worth posting anything until there is a
> > framework for collecting and maintaining such things. I do agree that
> > the community should be working up a set of NFSv3 specific tests like
> > this. I like Frank's idea of making them a part of pynfs, fwiw.
> 
> Somebody did actually do a v3 pynfs that I never got around to merging,
it'd be
> worth revisiting:
> 
> 	https://github.com/sthaber/pynfs

I'm working on that... It requires significant effort, but I have made some
progress:

https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb0d8809
5e

I need to get back to it, but it's lower on my priority list.

Frank

Re: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

['Bruce Fields']

On Thu, Jan 06, 2022 at 10:28:10AM -0800, Frank Filz wrote:
> > On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote:
> > > > but still may be worth posting
> > > > somewhere and making it the start of a collection of protocol-level
> > > > v3 tests.
> > >
> > > ... I question whether it's worth posting anything until there is a
> > > framework for collecting and maintaining such things. I do agree that
> > > the community should be working up a set of NFSv3 specific tests like
> > > this. I like Frank's idea of making them a part of pynfs, fwiw.
> > 
> > Somebody did actually do a v3 pynfs that I never got around to merging,
> it'd be
> > worth revisiting:
> > 
> > 	https://github.com/sthaber/pynfs
> 
> I'm working on that... It requires significant effort, but I have made some
> progress:
> 
> https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb0d8809
> 5e
> 
> I need to get back to it, but it's lower on my priority list.

Oh, good to know, thanks.

--b.

RE: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

[Frank Filz]

> On Thu, Jan 06, 2022 at 10:28:10AM -0800, Frank Filz wrote:
> > > On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote:
> > > > > but still may be worth posting
> > > > > somewhere and making it the start of a collection of
> > > > > protocol-level
> > > > > v3 tests.
> > > >
> > > > ... I question whether it's worth posting anything until there is
> > > > a framework for collecting and maintaining such things. I do agree
> > > > that the community should be working up a set of NFSv3 specific
> > > > tests like this. I like Frank's idea of making them a part of pynfs,
fwiw.
> > >
> > > Somebody did actually do a v3 pynfs that I never got around to
> > > merging,
> > it'd be
> > > worth revisiting:
> > >
> > > 	https://github.com/sthaber/pynfs
> >
> > I'm working on that... It requires significant effort, but I have made
> > some
> > progress:
> >
> > https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb
> > 0d8809
> > 5e
> >
> > I need to get back to it, but it's lower on my priority list.
> 
> Oh, good to know, thanks.

Are you going to continue to maintain pynfs? Your repo is the one we use as
our "gold standard".

Frank

Re: [PATCH RFC] NFSD: Fix zero-length NFSv3 WRITEs

['Bruce Fields']

On Thu, Jan 06, 2022 at 11:37:46AM -0800, Frank Filz wrote:
> > On Thu, Jan 06, 2022 at 10:28:10AM -0800, Frank Filz wrote:
> > > > On Tue, Jan 04, 2022 at 01:12:40AM +0000, Chuck Lever III wrote:
> > > > > > but still may be worth posting
> > > > > > somewhere and making it the start of a collection of
> > > > > > protocol-level
> > > > > > v3 tests.
> > > > >
> > > > > ... I question whether it's worth posting anything until there is
> > > > > a framework for collecting and maintaining such things. I do agree
> > > > > that the community should be working up a set of NFSv3 specific
> > > > > tests like this. I like Frank's idea of making them a part of pynfs,
> fwiw.
> > > >
> > > > Somebody did actually do a v3 pynfs that I never got around to
> > > > merging,
> > > it'd be
> > > > worth revisiting:
> > > >
> > > > 	https://github.com/sthaber/pynfs
> > >
> > > I'm working on that... It requires significant effort, but I have made
> > > some
> > > progress:
> > >
> > > https://github.com/ffilz/pynfs/commit/d3a1610815117cb6bdf6567e575baedb
> > > 0d8809
> > > 5e
> > >
> > > I need to get back to it, but it's lower on my priority list.
> > 
> > Oh, good to know, thanks.
> 
> Are you going to continue to maintain pynfs? Your repo is the one we use as
> our "gold standard".

For now, yeah.

--b.