Re: [PATCH 5/9] nfsd: NFSv3 should allow zero length writes

From: Chuck Lever III <chuck.lever@oracle.com>
To: Bruce Fields <bfields@redhat.com>
Cc: "trondmy@kernel.org" <trondmy@kernel.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 5/9] nfsd: NFSv3 should allow zero length writes
Date: Sat, 18 Dec 2021 18:41:57 +0000	[thread overview]
Message-ID: <3A406AE7-B088-4618-9FA1-63BA6E939578@oracle.com> (raw)
In-Reply-To: <CAPL3RVFaWgdWQnWOe5B_6=1pNGSOZXp=SVFOBs24aucXphi6wQ@mail.gmail.com>

> On Dec 17, 2021, at 5:23 PM, Bruce Fields <bfields@redhat.com> wrote:
> 
> On Fri, Dec 17, 2021 at 5:07 PM <trondmy@kernel.org> wrote:
>> 
>> From: Trond Myklebust <trond.myklebust@hammerspace.com>
>> 
>> According to RFC1813: "If count is 0, the WRITE will succeed
>> and return a count of 0, barring errors due to permissions checking."
> 
> Yes, I'm surprised we're not already doing this right.
> 
> I wonder how far back this bug goes.
> 
> The svc.c code is from 8154ef2776aa "NFSD: Clean up legacy NFS WRITE
> argument XDR decoders", but the behavior might predate that code.  The
> nfsd_vfs_write() logic, I'm not sure I understand.

The new check in nfsd_vfs_write() might circumvent the VFS
layer's security checks, so I agree, that is a little suspect.

> We have a pynfs test for the v4 case (WRT4), but I guess we must have
> nothing testing the v3 case.

I guess cthon04 doesn't check this case.

But it seems to me WRT4 should already tickle any problems
with nfsd_vfs_write(), shouldn't it?

> --b.
> 
>> 
>> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
>> ---
>> fs/nfsd/vfs.c    | 3 +++
>> net/sunrpc/svc.c | 2 +-
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>> index 4d57befdac6e..38fdfcbb079e 100644
>> --- a/fs/nfsd/vfs.c
>> +++ b/fs/nfsd/vfs.c
>> @@ -969,6 +969,9 @@ nfsd_vfs_write(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfsd_file *nf,
>> 
>>        trace_nfsd_write_opened(rqstp, fhp, offset, *cnt);
>> 
>> +       if (!*cnt)
>> +               return nfs_ok;
>> +
>>        if (sb->s_export_op)
>>                exp_op_flags = sb->s_export_op->flags;
>> 
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index a3bbe5ce4570..d1ccf37a4588 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -1692,7 +1692,7 @@ unsigned int svc_fill_write_vector(struct svc_rqst *rqstp, struct page **pages,
>>         * entirely in rq_arg.pages. In this case, @first is empty.
>>         */
>>        i = 0;
>> -       if (first->iov_len) {
>> +       if (first->iov_len || !total) {
>>                vec[i].iov_base = first->iov_base;
>>                vec[i].iov_len = min_t(size_t, total, first->iov_len);
>>                total -= vec[i].iov_len;
>> --
>> 2.33.1
>> 
> 

--
Chuck Lever