[RFC PATCH] NFSD: Fix READDIR on NFSv3 mounts of ext4 exports

[RFC PATCH] NFSD: Fix READDIR on NFSv3 mounts of ext4 exports

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

I noticed that recently, simple operations like "make" started
failing on NFSv3 mounts of ext4 exports. Network capture shows that
READDIRPLUS operated correctly but READDIR failed with
NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is
passed a non-zero starting directory cookie.

I bisected to commit c689bdd3bffa ("nfsd: further centralize
protocol version checks.").

Turns out that nfsd3_proc_readdir() does not call fh_verify() before
it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is
not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when
the directory is opened.

For ext4, this causes the wrong "max file size" value to be used
when sanity checking the incoming directory cookie (which is a seek
offset value).

Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure
the new boolean fields are properly initialized.

There is a risk that these procedures might now return a status code
that is not valid (by spec), or that operations that are currently
allowed might no longer be.

Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs3proc.c | 6 ++++++
 fs/nfsd/nfsproc.c  | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index dfcc957e460d..48bcdc96b867 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
 	resp->cookie_offset = 0;
 	resp->rqstp = rqstp;
 	offset = argp->cookie;
+
+	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
+	if (resp->status != nfs_ok)
+		goto out;
+
 	resp->status = nfsd_readdir(rqstp, &resp->fh, &offset,
 				    &resp->common, nfs3svc_encode_entry3);
 	memcpy(resp->verf, argp->verf, 8);
@@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
 	/* Recycle only pages that were part of the reply */
 	rqstp->rq_next_page = resp->xdr.page_ptr + 1;
 
+out:
 	return rpc_success;
 }
 
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index 97aab34593ef..ebe8fd3c9ddd 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp)
 	resp->common.err = nfs_ok;
 	resp->cookie_offset = 0;
 	offset = argp->cookie;
+
+	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
+	if (resp->status != nfs_ok)
+		goto out;
+
 	resp->status = nfsd_readdir(rqstp, &argp->fh, &offset,
 				    &resp->common, nfssvc_encode_entry);
 	nfssvc_encode_nfscookie(resp, offset);
-
 	fh_put(&argp->fh);
+
+out:
 	return rpc_success;
 }
 
-- 
2.47.0

Re: [RFC PATCH] NFSD: Fix READDIR on NFSv3 mounts of ext4 exports

[Jeff Layton]

On Sun, 2024-11-03 at 16:37 -0500, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> I noticed that recently, simple operations like "make" started
> failing on NFSv3 mounts of ext4 exports. Network capture shows that
> READDIRPLUS operated correctly but READDIR failed with
> NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is
> passed a non-zero starting directory cookie.
> 
> I bisected to commit c689bdd3bffa ("nfsd: further centralize
> protocol version checks.").
> 
> Turns out that nfsd3_proc_readdir() does not call fh_verify() before
> it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is
> not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when
> the directory is opened.
> 
> For ext4, this causes the wrong "max file size" value to be used
> when sanity checking the incoming directory cookie (which is a seek
> offset value).
> 
> Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure
> the new boolean fields are properly initialized.
> 
> There is a risk that these procedures might now return a status code
> that is not valid (by spec), or that operations that are currently
> allowed might no longer be.
> 
> Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs3proc.c | 6 ++++++
>  fs/nfsd/nfsproc.c  | 8 +++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index dfcc957e460d..48bcdc96b867 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
>  	resp->cookie_offset = 0;
>  	resp->rqstp = rqstp;
>  	offset = argp->cookie;
> +
> +	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
> +	if (resp->status != nfs_ok)
> +		goto out;
> +
>  	resp->status = nfsd_readdir(rqstp, &resp->fh, &offset,
>  				    &resp->common, nfs3svc_encode_entry3);
>  	memcpy(resp->verf, argp->verf, 8);
> @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
>  	/* Recycle only pages that were part of the reply */
>  	rqstp->rq_next_page = resp->xdr.page_ptr + 1;
>  
> +out:
>  	return rpc_success;
>  }
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 97aab34593ef..ebe8fd3c9ddd 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp)
>  	resp->common.err = nfs_ok;
>  	resp->cookie_offset = 0;
>  	offset = argp->cookie;
> +
> +	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
> +	if (resp->status != nfs_ok)
> +		goto out;
> +
>  	resp->status = nfsd_readdir(rqstp, &argp->fh, &offset,
>  				    &resp->common, nfssvc_encode_entry);
>  	nfssvc_encode_nfscookie(resp, offset);
> -
>  	fh_put(&argp->fh);
> +
> +out:
>  	return rpc_success;
>  }
>  

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [RFC PATCH] NFSD: Fix READDIR on NFSv3 mounts of ext4 exports

[NeilBrown]

On Mon, 04 Nov 2024, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> I noticed that recently, simple operations like "make" started
> failing on NFSv3 mounts of ext4 exports. Network capture shows that
> READDIRPLUS operated correctly but READDIR failed with
> NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is
> passed a non-zero starting directory cookie.
> 
> I bisected to commit c689bdd3bffa ("nfsd: further centralize
> protocol version checks.").
> 
> Turns out that nfsd3_proc_readdir() does not call fh_verify() before
> it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is
> not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when
> the directory is opened.
> 
> For ext4, this causes the wrong "max file size" value to be used
> when sanity checking the incoming directory cookie (which is a seek
> offset value).
> 
> Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure
> the new boolean fields are properly initialized.
> 
> There is a risk that these procedures might now return a status code
> that is not valid (by spec), or that operations that are currently
> allowed might no longer be.

This seems like the wrong fix to me.  Why should nfsd4_proc_readdir()
call fh_verify() when nfsd_readdir() already does that via 
nfsd_open()??

The only reason is to set NFSD_MAY_64BIT_COOKIE.  Let's just get rid of
that flag instead and use fhp->fh_64bit_cookies directly.

Like this:

diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index b8470d4cbe99..51b2074d92a5 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -86,7 +86,6 @@ DEFINE_NFSD_XDR_ERR_EVENT(cant_encode);
 		{ NFSD_MAY_NOT_BREAK_LEASE,	"NOT_BREAK_LEASE" },	\
 		{ NFSD_MAY_BYPASS_GSS,		"BYPASS_GSS" },		\
 		{ NFSD_MAY_READ_IF_EXEC,	"READ_IF_EXEC" },	\
-		{ NFSD_MAY_64BIT_COOKIE,	"64BIT_COOKIE" },	\
 		{ NFSD_MAY_LOCALIO,		"LOCALIO" })
 
 TRACE_EVENT(nfsd_compound,
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 22325b590e17..a86b6f6a3b98 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -903,7 +903,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 		goto out;
 	}
 
-	if (may_flags & NFSD_MAY_64BIT_COOKIE)
+	if (fhp->fh_64bit_cookies)
 		file->f_mode |= FMODE_64BITHASH;
 	else
 		file->f_mode |= FMODE_32BITHASH;
@@ -2174,9 +2174,6 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
 	loff_t		offset = *offsetp;
 	int             may_flags = NFSD_MAY_READ;
 
-	if (fhp->fh_64bit_cookies)
-		may_flags |= NFSD_MAY_64BIT_COOKIE;
-
 	err = nfsd_open(rqstp, fhp, S_IFDIR, may_flags, &file);
 	if (err)
 		goto out;
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 3ff146522556..2ba4265a22a0 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -31,8 +31,6 @@
 #define NFSD_MAY_BYPASS_GSS		0x400
 #define NFSD_MAY_READ_IF_EXEC		0x800
 
-#define NFSD_MAY_64BIT_COOKIE		0x1000 /* 64 bit readdir cookies for >= NFSv3 */
-
 #define NFSD_MAY_LOCALIO		0x2000 /* for tracing, reflects when localio used */
 
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)


If you agree I can send that as a proper patch.

Thanks,
NeilBrown



> 
> Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs3proc.c | 6 ++++++
>  fs/nfsd/nfsproc.c  | 8 +++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> index dfcc957e460d..48bcdc96b867 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
>  	resp->cookie_offset = 0;
>  	resp->rqstp = rqstp;
>  	offset = argp->cookie;
> +
> +	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
> +	if (resp->status != nfs_ok)
> +		goto out;
> +
>  	resp->status = nfsd_readdir(rqstp, &resp->fh, &offset,
>  				    &resp->common, nfs3svc_encode_entry3);
>  	memcpy(resp->verf, argp->verf, 8);
> @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
>  	/* Recycle only pages that were part of the reply */
>  	rqstp->rq_next_page = resp->xdr.page_ptr + 1;
>  
> +out:
>  	return rpc_success;
>  }
>  
> diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> index 97aab34593ef..ebe8fd3c9ddd 100644
> --- a/fs/nfsd/nfsproc.c
> +++ b/fs/nfsd/nfsproc.c
> @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp)
>  	resp->common.err = nfs_ok;
>  	resp->cookie_offset = 0;
>  	offset = argp->cookie;
> +
> +	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
> +	if (resp->status != nfs_ok)
> +		goto out;
> +
>  	resp->status = nfsd_readdir(rqstp, &argp->fh, &offset,
>  				    &resp->common, nfssvc_encode_entry);
>  	nfssvc_encode_nfscookie(resp, offset);
> -
>  	fh_put(&argp->fh);
> +
> +out:
>  	return rpc_success;
>  }
>  
> -- 
> 2.47.0
> 
> 

Re: [RFC PATCH] NFSD: Fix READDIR on NFSv3 mounts of ext4 exports

[Chuck Lever]

On Mon, Nov 04, 2024 at 01:05:27PM +1100, NeilBrown wrote:
> On Mon, 04 Nov 2024, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > I noticed that recently, simple operations like "make" started
> > failing on NFSv3 mounts of ext4 exports. Network capture shows that
> > READDIRPLUS operated correctly but READDIR failed with
> > NFS3ERR_INVAL. The vfs_llseek() call returned EINVAL when it is
> > passed a non-zero starting directory cookie.
> > 
> > I bisected to commit c689bdd3bffa ("nfsd: further centralize
> > protocol version checks.").
> > 
> > Turns out that nfsd3_proc_readdir() does not call fh_verify() before
> > it calls nfsd_readdir(), so the new fhp->fh_64bit_cookies boolean is
> > not set properly. This leaves the NFSD_MAY_64BIT_COOKIE unset when
> > the directory is opened.
> > 
> > For ext4, this causes the wrong "max file size" value to be used
> > when sanity checking the incoming directory cookie (which is a seek
> > offset value).
> > 
> > Both NFSv2 and NFSv3 READDIR need to call fh_verify() now to ensure
> > the new boolean fields are properly initialized.
> > 
> > There is a risk that these procedures might now return a status code
> > that is not valid (by spec), or that operations that are currently
> > allowed might no longer be.
> 
> This seems like the wrong fix to me.

I agree that there is more than one alternative fix.

I chose this one because it's late in the cycle, and this fix seemed
least risky. I intended that further improvement would be done after
the v6.13-rc window closes.


> Why should nfsd4_proc_readdir()
> call fh_verify() when nfsd_readdir() already does that via 
> nfsd_open()??

NFSv4 COMPOUND processing calls fh_verify() already to verify the
current FH. So when nfsd4_encode_dirlist4() calls nfsd_readdir(),
that fhp is already instantiated properly.

nfsd3_proc_readdirplus() needs the fh_verify() call because it needs
to grab a handle on the FH's export to check the "nordplus" export
option. The fhp is instantiated before nfsd_readdir() is called.

So that double fh_verify() is already in the execution path of the
more complex READDIR operations. It doesn't seem harmful to add to
nfsd3_proc_readdir() -- but it's not terribly aesthetic.


> The only reason is to set NFSD_MAY_64BIT_COOKIE.
> 
> Let's just get rid of
> that flag instead and use fhp->fh_64bit_cookies directly.

64BIT_COOKIE doesn't seem to fit with the semantics of the other
MAY flags, so I like this clean-up.


> Like this:
> 
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index b8470d4cbe99..51b2074d92a5 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -86,7 +86,6 @@ DEFINE_NFSD_XDR_ERR_EVENT(cant_encode);
>  		{ NFSD_MAY_NOT_BREAK_LEASE,	"NOT_BREAK_LEASE" },	\
>  		{ NFSD_MAY_BYPASS_GSS,		"BYPASS_GSS" },		\
>  		{ NFSD_MAY_READ_IF_EXEC,	"READ_IF_EXEC" },	\
> -		{ NFSD_MAY_64BIT_COOKIE,	"64BIT_COOKIE" },	\
>  		{ NFSD_MAY_LOCALIO,		"LOCALIO" })
>  
>  TRACE_EVENT(nfsd_compound,
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 22325b590e17..a86b6f6a3b98 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -903,7 +903,7 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
>  		goto out;
>  	}
>  
> -	if (may_flags & NFSD_MAY_64BIT_COOKIE)
> +	if (fhp->fh_64bit_cookies)
>  		file->f_mode |= FMODE_64BITHASH;
>  	else
>  		file->f_mode |= FMODE_32BITHASH;

Is there any reason this logic couldn't be moved out to
nfsd_readdir(), after the nfsd_open() call?


> @@ -2174,9 +2174,6 @@ nfsd_readdir(struct svc_rqst *rqstp, struct svc_fh *fhp, loff_t *offsetp,
>  	loff_t		offset = *offsetp;
>  	int             may_flags = NFSD_MAY_READ;
>  
> -	if (fhp->fh_64bit_cookies)
> -		may_flags |= NFSD_MAY_64BIT_COOKIE;
> -
>  	err = nfsd_open(rqstp, fhp, S_IFDIR, may_flags, &file);
>  	if (err)
>  		goto out;
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 3ff146522556..2ba4265a22a0 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -31,8 +31,6 @@
>  #define NFSD_MAY_BYPASS_GSS		0x400
>  #define NFSD_MAY_READ_IF_EXEC		0x800
>  
> -#define NFSD_MAY_64BIT_COOKIE		0x1000 /* 64 bit readdir cookies for >= NFSv3 */
> -
>  #define NFSD_MAY_LOCALIO		0x2000 /* for tracing, reflects when localio used */
>  
>  #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
> 
> 
> If you agree I can send that as a proper patch.

Yes, please send a proper patch. If it passes testing, I will drop
the RFC patch.


> Thanks,
> NeilBrown
> 
> 
> 
> > 
> > Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs3proc.c | 6 ++++++
> >  fs/nfsd/nfsproc.c  | 8 +++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > index dfcc957e460d..48bcdc96b867 100644
> > --- a/fs/nfsd/nfs3proc.c
> > +++ b/fs/nfsd/nfs3proc.c
> > @@ -592,6 +592,11 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
> >  	resp->cookie_offset = 0;
> >  	resp->rqstp = rqstp;
> >  	offset = argp->cookie;
> > +
> > +	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
> > +	if (resp->status != nfs_ok)
> > +		goto out;
> > +
> >  	resp->status = nfsd_readdir(rqstp, &resp->fh, &offset,
> >  				    &resp->common, nfs3svc_encode_entry3);
> >  	memcpy(resp->verf, argp->verf, 8);
> > @@ -600,6 +605,7 @@ nfsd3_proc_readdir(struct svc_rqst *rqstp)
> >  	/* Recycle only pages that were part of the reply */
> >  	rqstp->rq_next_page = resp->xdr.page_ptr + 1;
> >  
> > +out:
> >  	return rpc_success;
> >  }
> >  
> > diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
> > index 97aab34593ef..ebe8fd3c9ddd 100644
> > --- a/fs/nfsd/nfsproc.c
> > +++ b/fs/nfsd/nfsproc.c
> > @@ -586,11 +586,17 @@ nfsd_proc_readdir(struct svc_rqst *rqstp)
> >  	resp->common.err = nfs_ok;
> >  	resp->cookie_offset = 0;
> >  	offset = argp->cookie;
> > +
> > +	resp->status = fh_verify(rqstp, &resp->fh, S_IFDIR, NFSD_MAY_NOP);
> > +	if (resp->status != nfs_ok)
> > +		goto out;
> > +
> >  	resp->status = nfsd_readdir(rqstp, &argp->fh, &offset,
> >  				    &resp->common, nfssvc_encode_entry);
> >  	nfssvc_encode_nfscookie(resp, offset);
> > -
> >  	fh_put(&argp->fh);
> > +
> > +out:
> >  	return rpc_success;
> >  }
> >  
> > -- 
> > 2.47.0
> > 
> > 
> 

-- 
Chuck Lever