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

[PATCH v2] 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).

The fhp->fh_64bit_cookies boolean is /always/ properly initialized
after nfsd_open() returns. There doesn't seem to be a reason for the
generic NFSD open helper to handle the f_mode fix-up for
directories, so just move that to the one caller that tries to open
an S_IFDIR with NFSD_MAY_64BIT_COOKIE.

Suggested-by: NeilBrown <neilb@suse.de>
Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/vfs.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

I'd like to get rolling with CI on this fix so it can go into
v6.12-rc this week, so I authored this version based on Neil's
suggestion.

Note that removing the NFSD_MAY_64BIT_COOKIE flag entirely conflicts
with the addition of NFSD_MAY_LOCALIO in v6.13. I've postponed the
clean-up parts of this patch until then to help make merging this
fix more smooth.


diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 22325b590e17..d6d4f2a0e898 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -903,11 +903,6 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
 		goto out;
 	}
 
-	if (may_flags & NFSD_MAY_64BIT_COOKIE)
-		file->f_mode |= FMODE_64BITHASH;
-	else
-		file->f_mode |= FMODE_32BITHASH;
-
 	*filp = file;
 out:
 	return host_err;
@@ -2174,13 +2169,15 @@ 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;
 
+	if (fhp->fh_64bit_cookies)
+		file->f_mode |= FMODE_64BITHASH;
+	else
+		file->f_mode |= FMODE_32BITHASH;
+
 	offset = vfs_llseek(file, offset, SEEK_SET);
 	if (offset < 0) {
 		err = nfserrno((int)offset);
-- 
2.47.0

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

[NeilBrown]

On Thu, 07 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).
> 
> The fhp->fh_64bit_cookies boolean is /always/ properly initialized
> after nfsd_open() returns. There doesn't seem to be a reason for the
> generic NFSD open helper to handle the f_mode fix-up for
> directories, so just move that to the one caller that tries to open
> an S_IFDIR with NFSD_MAY_64BIT_COOKIE.

Thanks.  Looks good.  I like that you moved the f_mode setting out of
nfsd_open().  It really is only needed for directories.

Reviewed-by: NeilBrown <neilb@suse.de>

NeilBrown


> 
> Suggested-by: NeilBrown <neilb@suse.de>
> Fixes: c689bdd3bffa ("nfsd: further centralize protocol version checks.")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/vfs.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> I'd like to get rolling with CI on this fix so it can go into
> v6.12-rc this week, so I authored this version based on Neil's
> suggestion.
> 
> Note that removing the NFSD_MAY_64BIT_COOKIE flag entirely conflicts
> with the addition of NFSD_MAY_LOCALIO in v6.13. I've postponed the
> clean-up parts of this patch until then to help make merging this
> fix more smooth.
> 
> 
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 22325b590e17..d6d4f2a0e898 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -903,11 +903,6 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
>  		goto out;
>  	}
>  
> -	if (may_flags & NFSD_MAY_64BIT_COOKIE)
> -		file->f_mode |= FMODE_64BITHASH;
> -	else
> -		file->f_mode |= FMODE_32BITHASH;
> -
>  	*filp = file;
>  out:
>  	return host_err;
> @@ -2174,13 +2169,15 @@ 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;
>  
> +	if (fhp->fh_64bit_cookies)
> +		file->f_mode |= FMODE_64BITHASH;
> +	else
> +		file->f_mode |= FMODE_32BITHASH;
> +
>  	offset = vfs_llseek(file, offset, SEEK_SET);
>  	if (offset < 0) {
>  		err = nfserrno((int)offset);
> -- 
> 2.47.0
> 
>