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 <[email protected]>
> ---
> 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
>
--
Chuck Lever
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 <[email protected]>
> > ---
> > 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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
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 <[email protected]>
> > > ---
> > > ?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.
--
Jeff Layton <[email protected]>