On Mon, 04 Nov 2024, cel@xxxxxxxxxx wrote:
> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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
>
>
