| Message ID | 20240315-dir-deleg-v1-11-a1d6209a3654@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs, nfsd, nfs: implement directory delegations | expand |
On Fri, Mar 15, 2024 at 12:53:02PM -0400, Jeff Layton wrote: > fh_verify only allows you to filter on a single type of inode, so have > nfsd4_delegreturn not filter by type. Once fh_verify returns, do the > appropriate check of the type and return an error if it's not a regular > file or directory. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/nfsd/nfs4state.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 17d09d72632b..c52e807f9672 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > struct nfs4_delegation *dp; > stateid_t *stateid = &dr->dr_stateid; > struct nfs4_stid *s; > + umode_t mode; > __be32 status; > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); > > - if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0))) > + if ((status = fh_verify(rqstp, &cstate->current_fh, 0, 0))) > return status; > > + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode & S_IFMT; > + switch(mode) { > + case S_IFREG: > + case S_IFDIR: > + break; > + case S_IFLNK: > + return nfserr_symlink; > + default: > + return nfserr_inval; > + } > + RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK among the valid status codes for the DELEGRETURN operation. Maybe the naked fh_verify() call has gotten it wrong all these years...? > status = nfsd4_lookup_stateid(cstate, stateid, SC_TYPE_DELEG, 0, &s, nn); > if (status) > goto out; > > -- > 2.44.0 >
On Sun, 2024-03-17 at 11:09 -0400, Chuck Lever wrote: > On Fri, Mar 15, 2024 at 12:53:02PM -0400, Jeff Layton wrote: > > fh_verify only allows you to filter on a single type of inode, so > > have > > nfsd4_delegreturn not filter by type. Once fh_verify returns, do > > the > > appropriate check of the type and return an error if it's not a > > regular > > file or directory. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > fs/nfsd/nfs4state.c | 14 +++++++++++++- > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 17d09d72632b..c52e807f9672 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, > > struct nfsd4_compound_state *cstate, > > struct nfs4_delegation *dp; > > stateid_t *stateid = &dr->dr_stateid; > > struct nfs4_stid *s; > > + umode_t mode; > > __be32 status; > > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), > > nfsd_net_id); > > > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > > S_IFREG, 0))) > > + if ((status = fh_verify(rqstp, &cstate->current_fh, 0, > > 0))) > > return status; > > > > + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode & > > S_IFMT; > > + switch(mode) { > > + case S_IFREG: > > + case S_IFDIR: > > + break; > > + case S_IFLNK: > > + return nfserr_symlink; > > + default: > > + return nfserr_inval; > > + } > > + > > RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK among the > valid status codes for the DELEGRETURN operation. Maybe the naked > fh_verify() call has gotten it wrong all these years...? The WANT_DELEGATION operation allows the server to hand out delegations for aggressive caching of symlinks. It is not an error to return that delegation using DELEGRETURN. Furthermore, provided that the presented stateid is actually valid, it is also sufficient to uniquely identify the file to which it is associated (see RFC8881 Section 8.2.4), so the filehandle should be considered mostly irrelevant for operations like DELEGRETURN.
On Sun, 2024-03-17 at 16:03 +0000, Trond Myklebust wrote: > On Sun, 2024-03-17 at 11:09 -0400, Chuck Lever wrote: > > On Fri, Mar 15, 2024 at 12:53:02PM -0400, Jeff Layton wrote: > > > fh_verify only allows you to filter on a single type of inode, so > > > have > > > nfsd4_delegreturn not filter by type. Once fh_verify returns, do > > > the > > > appropriate check of the type and return an error if it's not a > > > regular > > > file or directory. > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > fs/nfsd/nfs4state.c | 14 +++++++++++++- > > > 1 file changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > > index 17d09d72632b..c52e807f9672 100644 > > > --- a/fs/nfsd/nfs4state.c > > > +++ b/fs/nfsd/nfs4state.c > > > @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, > > > struct nfsd4_compound_state *cstate, > > > struct nfs4_delegation *dp; > > > stateid_t *stateid = &dr->dr_stateid; > > > struct nfs4_stid *s; > > > + umode_t mode; > > > __be32 status; > > > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), > > > nfsd_net_id); > > > > > > - if ((status = fh_verify(rqstp, &cstate->current_fh, > > > S_IFREG, 0))) > > > + if ((status = fh_verify(rqstp, &cstate->current_fh, 0, > > > 0))) > > > return status; > > > > > > + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode & > > > S_IFMT; > > > + switch(mode) { > > > + case S_IFREG: > > > + case S_IFDIR: > > > + break; > > > + case S_IFLNK: > > > + return nfserr_symlink; > > > + default: > > > + return nfserr_inval; > > > + } > > > + > > > > RFC 8881 Section 15.2 does not list NFS4ERR_SYMLINK among the > > valid status codes for the DELEGRETURN operation. Maybe the naked > > fh_verify() call has gotten it wrong all these years...? > > The WANT_DELEGATION operation allows the server to hand out delegations > for aggressive caching of symlinks. It is not an error to return that > delegation using DELEGRETURN. > > Furthermore, provided that the presented stateid is actually valid, it > is also sufficient to uniquely identify the file to which it is > associated (see RFC8881 Section 8.2.4), so the filehandle should be > considered mostly irrelevant for operations like DELEGRETURN. > Ok. I think we can probably just drop the switch altogether. We already don't validate that the stateid is associated with current_fh, AFAICT. It looks possible to send a valid stateid alongside an FH that refers to a completely different file, and we'll just accept it.
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 17d09d72632b..c52e807f9672 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -7425,12 +7425,24 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfs4_delegation *dp; stateid_t *stateid = &dr->dr_stateid; struct nfs4_stid *s; + umode_t mode; __be32 status; struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id); - if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0))) + if ((status = fh_verify(rqstp, &cstate->current_fh, 0, 0))) return status; + mode = d_inode(cstate->current_fh.fh_dentry)->i_mode & S_IFMT; + switch(mode) { + case S_IFREG: + case S_IFDIR: + break; + case S_IFLNK: + return nfserr_symlink; + default: + return nfserr_inval; + } + status = nfsd4_lookup_stateid(cstate, stateid, SC_TYPE_DELEG, 0, &s, nn); if (status) goto out;
fh_verify only allows you to filter on a single type of inode, so have nfsd4_delegreturn not filter by type. Once fh_verify returns, do the appropriate check of the type and return an error if it's not a regular file or directory. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)