| Message ID | 20241103213731.85803-2-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] NFSD: Fix READDIR on NFSv3 mounts of ext4 exports | expand |
On Sun, 2024-11-03 at 16:37 -0500, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > I noticed that recently, simple operations like "make" started > failing on NFSv3 mounts of ext4 exports. Network capture shows that > READDIRPLUS operated correctly but READDIR failed with > NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is > passed a non-zero starting directory cookie. > > I bisected to commit c689bdd3bffa ("nfsd: further centralize > protocol version checks."). > > Turns out that nfsd3_proc_readdir() does not call fh_verify() before > it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is > not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when > the directory is opened. > > For ext4, this causes the wrong "max file size" value to be used > when sanity checking the incoming directory cookie (which is a seek > offset value). > > Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure > the new boolean fields are properly initialized. > > There is a risk that these procedures might now return a status code > that is not valid (by spec), or that operations that are currently > allowed might no longer be. > > Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs3proc.c | 6 ++++++ > fs/nfsd/nfsproc.c | 8 +++++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index dfcc957e460d..48bcdc96b867 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) > resp->cookie_offset = 0; > resp->rqstp = rqstp; > offset = argp->cookie; > + > + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); > + if (resp->status != nfs_ok) > + goto out; > + > resp->status = nfsd_readdir(rqstp, &resp->fh, &offset, > &resp->common, nfs3svc_encode_entry3); > memcpy(resp->verf, argp->verf, 8); > @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) > /* Recycle only pages that were part of the reply */ > rqstp->rq_next_page = resp->xdr.page_ptr + 1; > > +out: > return rpc_success; > } > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 97aab34593ef..ebe8fd3c9ddd 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp) > resp->common.err = nfs_ok; > resp->cookie_offset = 0; > offset = argp->cookie; > + > + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); > + if (resp->status != nfs_ok) > + goto out; > + > resp->status = nfsd_readdir(rqstp, &argp->fh, &offset, > &resp->common, nfssvc_encode_entry); > nfssvc_encode_nfscookie(resp, offset); > - > fh_put(&argp->fh); > + > +out: > return rpc_success; > } > Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Mon, 04 Nov 2024, cel@kernel.org wrote: > From: Chuck Lever <chuck.lever@oracle.com> > > I noticed that recently, simple operations like "make" started > failing on NFSv3 mounts of ext4 exports. Network capture shows that > READDIRPLUS operated correctly but READDIR failed with > NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is > passed a non-zero starting directory cookie. > > I bisected to commit c689bdd3bffa ("nfsd: further centralize > protocol version checks."). > > Turns out that nfsd3_proc_readdir() does not call fh_verify() before > it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is > not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when > the directory is opened. > > For ext4, this causes the wrong "max file size" value to be used > when sanity checking the incoming directory cookie (which is a seek > offset value). > > Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure > the new boolean fields are properly initialized. > > There is a risk that these procedures might now return a status code > that is not valid (by spec), or that operations that are currently > allowed might no longer be. This seems like the wrong fix to me. Why should nfsd4_proc_readdir() call fh_verify() when nfsd_readdir() already does that via nfsd_open()?? The only reason is to set NFSD_MAY_64BIT_COOKIE. Let's just get rid of that flag instead and use fhp->fh_64bit_cookies directly. Like this: diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h index b8470d4cbe99..51b2074d92a5 100644 --- a/fs/nfsd/trace.h +++ b/fs/nfsd/trace.h @@ -86,7 +86,6 @@ DEFINE_NFSD_XDR_ERR_EVENT(cant_encode); { NFSD_MAY_NOT_BREAK_LEASE, "NOT_BREAK_LEASE" }, \ { NFSD_MAY_BYPASS_GSS, "BYPASS_GSS" }, \ { NFSD_MAY_READ_IF_EXEC, "READ_IF_EXEC" }, \ - { NFSD_MAY_64BIT_COOKIE, "64BIT_COOKIE" }, \ { NFSD_MAY_LOCALIO, "LOCALIO" }) TRACE_EVENT(nfsd_compound, diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 22325b590e17..a86b6f6a3b98 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -903,7 +903,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, goto out; } - if (may_flags & NFSD_MAY_64BIT_COOKIE) + if (fhp->fh_64bit_cookies) file->f_mode |= FMODE_64BITHASH; else file->f_mode |= FMODE_32BITHASH; @@ -2174,9 +2174,6 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp, loff_t offset = *offsetp; int may_flags = NFSD_MAY_READ; - if (fhp->fh_64bit_cookies) - may_flags |= NFSD_MAY_64BIT_COOKIE; - err = nfsd_open(rqstp, fhp, S_IFDIR, may_flags, &file); if (err) goto out; diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index 3ff146522556..2ba4265a22a0 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -31,8 +31,6 @@ #define NFSD_MAY_BYPASS_GSS 0x400 #define NFSD_MAY_READ_IF_EXEC 0x800 -#define NFSD_MAY_64BIT_COOKIE 0x1000 /* 64 bit readdir cookies for >= NFSv3 */ - #define NFSD_MAY_LOCALIO 0x2000 /* for tracing, reflects when localio used */ #define NFSD_MAY_CREATE (NFSD_MAY_EXEC|NFSD_MAY_WRITE) If you agree I can send that as a proper patch. Thanks, NeilBrown > > Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.") > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs3proc.c | 6 ++++++ > fs/nfsd/nfsproc.c | 8 +++++++- > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > index dfcc957e460d..48bcdc96b867 100644 > --- a/fs/nfsd/nfs3proc.c > +++ b/fs/nfsd/nfs3proc.c > @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) > resp->cookie_offset = 0; > resp->rqstp = rqstp; > offset = argp->cookie; > + > + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); > + if (resp->status != nfs_ok) > + goto out; > + > resp->status = nfsd_readdir(rqstp, &resp->fh, &offset, > &resp->common, nfs3svc_encode_entry3); > memcpy(resp->verf, argp->verf, 8); > @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) > /* Recycle only pages that were part of the reply */ > rqstp->rq_next_page = resp->xdr.page_ptr + 1; > > +out: > return rpc_success; > } > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > index 97aab34593ef..ebe8fd3c9ddd 100644 > --- a/fs/nfsd/nfsproc.c > +++ b/fs/nfsd/nfsproc.c > @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp) > resp->common.err = nfs_ok; > resp->cookie_offset = 0; > offset = argp->cookie; > + > + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); > + if (resp->status != nfs_ok) > + goto out; > + > resp->status = nfsd_readdir(rqstp, &argp->fh, &offset, > &resp->common, nfssvc_encode_entry); > nfssvc_encode_nfscookie(resp, offset); > - > fh_put(&argp->fh); > + > +out: > return rpc_success; > } > > -- > 2.47.0 > >
On Mon, Nov 04, 2024 at 01:05:27PM +1100, NeilBrown wrote: > On Mon, 04 Nov 2024, cel@kernel.org wrote: > > From: Chuck Lever <chuck.lever@oracle.com> > > > > I noticed that recently, simple operations like "make" started > > failing on NFSv3 mounts of ext4 exports. Network capture shows that > > READDIRPLUS operated correctly but READDIR failed with > > NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is > > passed a non-zero starting directory cookie. > > > > I bisected to commit c689bdd3bffa ("nfsd: further centralize > > protocol version checks."). > > > > Turns out that nfsd3_proc_readdir() does not call fh_verify() before > > it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is > > not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when > > the directory is opened. > > > > For ext4, this causes the wrong "max file size" value to be used > > when sanity checking the incoming directory cookie (which is a seek > > offset value). > > > > Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure > > the new boolean fields are properly initialized. > > > > There is a risk that these procedures might now return a status code > > that is not valid (by spec), or that operations that are currently > > allowed might no longer be. > > This seems like the wrong fix to me. I agree that there is more than one alternative fix. I chose this one because it's late in the cycle, and this fix seemed least risky. I intended that further improvement would be done after the v6.13-rc window closes. > Why should nfsd4_proc_readdir() > call fh_verify() when nfsd_readdir() already does that via > nfsd_open()?? NFSv4 COMPOUND processing calls fh_verify() already to verify the current FH. So when nfsd4_encode_dirlist4() calls nfsd_readdir(), that fhp is already instantiated properly. nfsd3_proc_readdirplus() needs the fh_verify() call because it needs to grab a handle on the FH's export to check the "nordplus" export option. The fhp is instantiated before nfsd_readdir() is called. So that double fh_verify() is already in the execution path of the more complex READDIR operations. It doesn't seem harmful to add to nfsd3_proc_readdir() -- but it's not terribly aesthetic. > The only reason is to set NFSD_MAY_64BIT_COOKIE. > > Let's just get rid of > that flag instead and use fhp->fh_64bit_cookies directly. 64BIT_COOKIE doesn't seem to fit with the semantics of the other MAY flags, so I like this clean-up. > Like this: > > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h > index b8470d4cbe99..51b2074d92a5 100644 > --- a/fs/nfsd/trace.h > +++ b/fs/nfsd/trace.h > @@ -86,7 +86,6 @@ DEFINE_NFSD_XDR_ERR_EVENT(cant_encode); > { NFSD_MAY_NOT_BREAK_LEASE, "NOT_BREAK_LEASE" }, \ > { NFSD_MAY_BYPASS_GSS, "BYPASS_GSS" }, \ > { NFSD_MAY_READ_IF_EXEC, "READ_IF_EXEC" }, \ > - { NFSD_MAY_64BIT_COOKIE, "64BIT_COOKIE" }, \ > { NFSD_MAY_LOCALIO, "LOCALIO" }) > > TRACE_EVENT(nfsd_compound, > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 22325b590e17..a86b6f6a3b98 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -903,7 +903,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type, > goto out; > } > > - if (may_flags & NFSD_MAY_64BIT_COOKIE) > + if (fhp->fh_64bit_cookies) > file->f_mode |= FMODE_64BITHASH; > else > file->f_mode |= FMODE_32BITHASH; Is there any reason this logic couldn't be moved out to nfsd_readdir(), after the nfsd_open() call? > @@ -2174,9 +2174,6 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp, > loff_t offset = *offsetp; > int may_flags = NFSD_MAY_READ; > > - if (fhp->fh_64bit_cookies) > - may_flags |= NFSD_MAY_64BIT_COOKIE; > - > err = nfsd_open(rqstp, fhp, S_IFDIR, may_flags, &file); > if (err) > goto out; > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index 3ff146522556..2ba4265a22a0 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -31,8 +31,6 @@ > #define NFSD_MAY_BYPASS_GSS 0x400 > #define NFSD_MAY_READ_IF_EXEC 0x800 > > -#define NFSD_MAY_64BIT_COOKIE 0x1000 /* 64 bit readdir cookies for >= NFSv3 */ > - > #define NFSD_MAY_LOCALIO 0x2000 /* for tracing, reflects when localio used */ > > #define NFSD_MAY_CREATE (NFSD_MAY_EXEC|NFSD_MAY_WRITE) > > > If you agree I can send that as a proper patch. Yes, please send a proper patch. If it passes testing, I will drop the RFC patch. > Thanks, > NeilBrown > > > > > > > Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.") > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > > --- > > fs/nfsd/nfs3proc.c | 6 ++++++ > > fs/nfsd/nfsproc.c | 8 +++++++- > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c > > index dfcc957e460d..48bcdc96b867 100644 > > --- a/fs/nfsd/nfs3proc.c > > +++ b/fs/nfsd/nfs3proc.c > > @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) > > resp->cookie_offset = 0; > > resp->rqstp = rqstp; > > offset = argp->cookie; > > + > > + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); > > + if (resp->status != nfs_ok) > > + goto out; > > + > > resp->status = nfsd_readdir(rqstp, &resp->fh, &offset, > > &resp->common, nfs3svc_encode_entry3); > > memcpy(resp->verf, argp->verf, 8); > > @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) > > /* Recycle only pages that were part of the reply */ > > rqstp->rq_next_page = resp->xdr.page_ptr + 1; > > > > +out: > > return rpc_success; > > } > > > > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c > > index 97aab34593ef..ebe8fd3c9ddd 100644 > > --- a/fs/nfsd/nfsproc.c > > +++ b/fs/nfsd/nfsproc.c > > @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp) > > resp->common.err = nfs_ok; > > resp->cookie_offset = 0; > > offset = argp->cookie; > > + > > + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); > > + if (resp->status != nfs_ok) > > + goto out; > > + > > resp->status = nfsd_readdir(rqstp, &argp->fh, &offset, > > &resp->common, nfssvc_encode_entry); > > nfssvc_encode_nfscookie(resp, offset); > > - > > fh_put(&argp->fh); > > + > > +out: > > return rpc_success; > > } > > > > -- > > 2.47.0 > > > > >
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c index dfcc957e460d..48bcdc96b867 100644 --- a/fs/nfsd/nfs3proc.c +++ b/fs/nfsd/nfs3proc.c @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) resp->cookie_offset = 0; resp->rqstp = rqstp; offset = argp->cookie; + + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); + if (resp->status != nfs_ok) + goto out; + resp->status = nfsd_readdir(rqstp, &resp->fh, &offset, &resp->common, nfs3svc_encode_entry3); memcpy(resp->verf, argp->verf, 8); @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp) /* Recycle only pages that were part of the reply */ rqstp->rq_next_page = resp->xdr.page_ptr + 1; +out: return rpc_success; } diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c index 97aab34593ef..ebe8fd3c9ddd 100644 --- a/fs/nfsd/nfsproc.c +++ b/fs/nfsd/nfsproc.c @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp) resp->common.err = nfs_ok; resp->cookie_offset = 0; offset = argp->cookie; + + resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP); + if (resp->status != nfs_ok) + goto out; + resp->status = nfsd_readdir(rqstp, &argp->fh, &offset, &resp->common, nfssvc_encode_entry); nfssvc_encode_nfscookie(resp, offset); - fh_put(&argp->fh); + +out: return rpc_success; }