Re: [PATCH RFC 11/24] nfsd: allow DELEGRETURN on directories

[Trond Myklebust]

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@xxxxxxxxxx>
> > ---
> >  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
trond.myklebust@xxxxxxxxxxxxxxx