diff mbox series

[RFC,3/6] NFSD: Avoid using rqstp->rq_vers in nfsd_set_fh_dentry()

Message ID 20240828004445.22634-4-cel@kernel.org (mailing list archive)
State New
Headers show
Series Split up refactoring of fh_verify() | expand

Commit Message

Chuck Lever Aug. 28, 2024, 12:44 a.m. UTC 
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 when 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>
---
 fs/nfsd/lockd.c |  6 ++++--
 fs/nfsd/nfsfh.c | 11 +++++++----
 2 files changed, 11 insertions(+), 6 deletions(-)

Comments

NeilBrown Aug. 28, 2024, 5:02 a.m. UTC | #1 
On Wed, 28 Aug 2024, cel@kernel.org 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 when during initialization.
> 
> write_filehandle() is the other question mark, as it looks possible
> to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.

The file handle used by lockd and the one created by write_filehandle
never need any of the version-specific fields.
Those fields affect things like write requests and getattr requests and
pre/post attributes.

I wonder if the filehandle is really the best place of these flag.
Having them in the file handle works really well for fh_fill_pre_attrs()
and reasonably well in other places, But it makes nfsd_set_fh_dentry a
little clumsy.  Maybe it would be better to moved them to
rqstp->rq_flags.  Or possibly in the "Catering to nfsd" section of
'struct svc_rqst'.

NeilBrown


> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  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;
> -- 
> 2.45.2
> 
>
Chuck Lever Aug. 28, 2024, 1:45 p.m. UTC | #2 
On Wed, Aug 28, 2024 at 03:02:42PM +1000, NeilBrown wrote:
> On Wed, 28 Aug 2024, cel@kernel.org 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 when during initialization.
> > 
> > write_filehandle() is the other question mark, as it looks possible
> > to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.
> 
> The file handle used by lockd and the one created by write_filehandle
> never need any of the version-specific fields.
> Those fields affect things like write requests and getattr requests and
> pre/post attributes.

Then we could simply drop the "case 0:" arm and maybe the lockd hunk
from this patch.


> I wonder if the filehandle is really the best place of these flag.
> Having them in the file handle works really well for fh_fill_pre_attrs()
> and reasonably well in other places, But it makes nfsd_set_fh_dentry a
> little clumsy.

The file handle initialization code in here is indeed a bit awkward.
I didn't take it further in this patch set because I didn't have any
brilliant ideas, other than perhaps a version-specific
initialization call-out.

I don't think we need to go there to implement LOCALIO, but we could
save such a clean-up for another day.


> Maybe it would be better to moved them to
> rqstp->rq_flags.  Or possibly in the "Catering to nfsd" section of
> 'struct svc_rqst'.

Jeff and I have agreed to migrate NFS (or any upper layer protocol)
specifics out of the svc_rqst, where possible, simply to maintain
proper layering. I don't have any better ideas, though.


> NeilBrown
> 
> 
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  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;
> > -- 
> > 2.45.2
> > 
> > 
>
Mike Snitzer Aug. 28, 2024, 2:19 p.m. UTC | #3 
On Wed, Aug 28, 2024 at 09:45:31AM -0400, Chuck Lever wrote:
> On Wed, Aug 28, 2024 at 03:02:42PM +1000, NeilBrown wrote:
> > On Wed, 28 Aug 2024, cel@kernel.org 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 when during initialization.
> > > 
> > > write_filehandle() is the other question mark, as it looks possible
> > > to specify a maxsize between NFS_FHSIZE and NFS3_FHSIZE here.
> > 
> > The file handle used by lockd and the one created by write_filehandle
> > never need any of the version-specific fields.
> > Those fields affect things like write requests and getattr requests and
> > pre/post attributes.
> 
> Then we could simply drop the "case 0:" arm and maybe the lockd hunk
> from this patch.

OK, will do.
diff mbox series

Patch

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;