Re: [PATCH v14 06/25] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Aug 29, 2024 at 10:28:18AM -0400, Jeff Layton wrote:
> On Wed, 2024-08-28 at 21:04 -0400, Mike Snitzer wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Currently, fh_verify() makes some daring assumptions about which
> > version of file handle the caller wants, based on the things it can
> > find in the passed-in rqstp. The about-to-be-introduced LOCALIO use
> > case sometimes has no svc_rqst context, so this logic won't work in
> > that case.
> > 
> > Instead, examine the passed-in file handle. It's .max_size field
> > should carry information to allow nfsd_set_fh_dentry() to initialize
> > the file handle appropriately.
> > 
> > lockd appears to be the only kernel consumer that does not set the
> > file handle .max_size during initialization.
> > 
> > write_filehandle() is the other question mark, as it looks possible
> > to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfsd/lockd.c |  6 ++++--
> >  fs/nfsd/nfsfh.c | 11 +++++++----
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/nfsd/lockd.c b/fs/nfsd/lockd.c
> > index 46a7f9b813e5..e636d2a1e664 100644
> > --- a/fs/nfsd/lockd.c
> > +++ b/fs/nfsd/lockd.c
> > @@ -32,8 +32,10 @@ nlm_fopen(struct svc_rqst *rqstp, struct nfs_fh *f, struct file **filp,
> >  	int		access;
> >  	struct svc_fh	fh;
> >  
> > -	/* must initialize before using! but maxsize doesn't matter */
> > -	fh_init(&fh,0);
> > +	if (rqstp->rq_vers == 4)
> > +		fh_init(&fh, NFS3_FHSIZE);
> > +	else
> > +		fh_init(&fh, NFS_FHSIZE);
> >  	fh.fh_handle.fh_size = f->size;
> >  	memcpy(&fh.fh_handle.fh_raw, f->data, f->size);
> >  	fh.fh_export = NULL;
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index 4b964a71a504..77acc26e8b02 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -267,25 +267,28 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp)
> >  	fhp->fh_dentry = dentry;
> >  	fhp->fh_export = exp;
> >  
> > -	switch (rqstp->rq_vers) {
> > -	case 4:
> > +	switch (fhp->fh_maxsize) {
> > +	case NFS4_FHSIZE:
> >  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR)
> >  			fhp->fh_no_atomic_attr = true;
> >  		fhp->fh_64bit_cookies = true;
> >  		break;
> > -	case 3:
> > +	case NFS3_FHSIZE:
> >  		if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC)
> >  			fhp->fh_no_wcc = true;
> >  		fhp->fh_64bit_cookies = true;
> >  		if (exp->ex_flags & NFSEXP_V4ROOT)
> >  			goto out;
> >  		break;
> > -	case 2:
> > +	case NFS_FHSIZE:
> >  		fhp->fh_no_wcc = true;
> >  		if (EX_WGATHER(exp))
> >  			fhp->fh_use_wgather = true;
> >  		if (exp->ex_flags & NFSEXP_V4ROOT)
> >  			goto out;
> > +		break;
> > +	case 0:
> > +		WARN_ONCE(1, "Uninitialized file handle");
> >  	}
> >  
> >  	return 0;
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

Thanks for the review!  But please note that you reviewed the stale
patch I mistakenly sent out, I replied to this patch with:

[PATCH v14.5 06/25] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

Thanks.