MIME-Version: 1.0
References: <20211217215046.40316-1-trondmy@kernel.org> <20211217215046.40316-2-trondmy@kernel.org>
 <20211217215046.40316-3-trondmy@kernel.org> <20211217215046.40316-4-trondmy@kernel.org>
 <20211217215046.40316-5-trondmy@kernel.org> <20211217215046.40316-6-trondmy@kernel.org>
In-Reply-To: <20211217215046.40316-6-trondmy@kernel.org>
From:   Bruce Fields <bfields@redhat.com>
Date:   Fri, 17 Dec 2021 17:23:28 -0500
Message-ID: <CAPL3RVFaWgdWQnWOe5B_6=1pNGSOZXp=SVFOBs24aucXphi6wQ@mail.gmail.com>
Subject: Re: [PATCH 5/9] nfsd: NFSv3 should allow zero length writes
To:     trondmy@kernel.org
Cc:     Chuck Lever <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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

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