diff mbox series

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

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

Commit Message

Jeff Layton March 15, 2024, 4:53 p.m. UTC
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(-)

Comments

Chuck Lever March 17, 2024, 3:09 p.m. UTC | #1
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
>
Trond Myklebust March 17, 2024, 4:03 p.m. UTC | #2
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.
Jeff Layton March 18, 2024, 11:22 a.m. UTC | #3
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 mbox series

Patch

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;