| Message ID | 20211217215046.40316-6-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Assorted patches for knfsd | expand |
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 >
> 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
On Sat, Dec 18, 2021 at 1:42 PM Chuck Lever III <chuck.lever@oracle.com> wrote: > But it seems to me WRT4 should already tickle any problems > with nfsd_vfs_write(), shouldn't it? I was just taking Trond's word that this is NFSv3-specific, but it's not clear to me why from the code, and I know that WRT4 is passing. Something's weird. I'm travelling or I'd test it. --b.
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;