Dan Aloni reported a problem with the way NFSD's READ implementation
deals with the very upper end of file sizes, and I got interested in
how some of the other operations handled it. I found some issues,
and have started a (growing) pile of patches to deal with them.
Since at least the SETATTR case appears to cause a crash on some
filesystems, I think several of these are 5.17-rc fodder (i.e.,
priority bug fixes). I see that NLM also has potential problems with
how the max file size is handled, but since locking doesn't involve
the page cache, I think fixes in that area can be delayed a bit.
Dan's still working on the READ issue. I need some input on whether
I understand the problem correctly and whether the NFS status codes
I've chosen to use are going to be reasonable or a problem for NFS
clients. I've attempted to stay within the bound of the NFS specs,
but sometimes the spec doesn't provide a mechanism in the protocol
to indicate that the client passed us a bogus size/offset/count.
---
Chuck Lever (6):
NFSD: Fix NFSv4 SETATTR's handling of large file sizes
NFSD: Fix NFSv3 SETATTR's handling of large file sizes
NFSD: COMMIT operations must not return NFS?ERR_INVAL
NFSD: Replace directory offset placeholder
NFSD: Remove NFS_OFFSET_MAX
NFSD: Clamp WRITE offsets
fs/nfsd/nfs3proc.c | 32 +++++++++++++++++++++------
fs/nfsd/nfs3xdr.c | 4 ++--
fs/nfsd/nfs4proc.c | 7 +++++-
fs/nfsd/nfs4xdr.c | 2 +-
fs/nfsd/vfs.c | 53 ++++++++++++++++++++++++++++++---------------
fs/nfsd/vfs.h | 4 ++--
include/linux/nfs.h | 8 -------
7 files changed, 72 insertions(+), 38 deletions(-)
--
Chuck Lever
This constant was introduced way back in Linux v2.3.y before there
was a kernel-wide OFFSET_MAX value. These days we prefer to check
against the limits in the underlying filesystem instead.
Signed-off-by: Chuck Lever <[email protected]>
---
include/linux/nfs.h | 8 --------
1 file changed, 8 deletions(-)
diff --git a/include/linux/nfs.h b/include/linux/nfs.h
index 0dc7ad38a0da..b06375e88e58 100644
--- a/include/linux/nfs.h
+++ b/include/linux/nfs.h
@@ -36,14 +36,6 @@ static inline void nfs_copy_fh(struct nfs_fh *target, const struct nfs_fh *sourc
memcpy(target->data, source->data, source->size);
}
-
-/*
- * This is really a general kernel constant, but since nothing like
- * this is defined in the kernel headers, I have to do it here.
- */
-#define NFS_OFFSET_MAX ((__s64)((~(__u64)0) >> 1))
-
-
enum nfs3_stable_how {
NFS_UNSTABLE = 0,
NFS_DATA_SYNC = 1,
Ensure that a client cannot specify a WRITE range that falls in a
byte range outside what the kernel's internal types (such as loff_t,
which is signed) can represent. The kiocb iterators, invoked in
nfsd_vfs_write(), should properly limit write operations to within
the underlying file system's s_maxbytes.
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfsd/nfs3proc.c | 7 +++++++
fs/nfsd/nfs4proc.c | 4 +++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 7bca219a8146..ba72bbff53a5 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -216,6 +216,12 @@ nfsd3_proc_write(struct svc_rqst *rqstp)
(unsigned long long) argp->offset,
argp->stable? " stable" : "");
+ resp->status = nfserr_fbig;
+ if (argp->offset >= OFFSET_MAX)
+ goto out;
+ if (argp->offset + argp->len >= OFFSET_MAX)
+ goto out;
+
fh_copy(&resp->fh, &argp->fh);
resp->committed = argp->stable;
nvecs = svc_fill_write_vector(rqstp, &argp->payload);
@@ -224,6 +230,7 @@ nfsd3_proc_write(struct svc_rqst *rqstp)
rqstp->rq_vec, nvecs, &cnt,
resp->committed, resp->verf);
resp->count = cnt;
+out:
return rpc_success;
}
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index b8ac2b9bce74..2baf547b344f 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1022,7 +1022,9 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
int nvecs;
if (write->wr_offset >= OFFSET_MAX)
- return nfserr_inval;
+ return nfserr_fbig;
+ if (write->wr_offset + write->wr_buflen >= OFFSET_MAX)
+ return nfserr_fbig;
cnt = write->wr_buflen;
trace_nfsd_write_start(rqstp, &cstate->current_fh,
Ooh, lots to consider for Ganesha...
And I see you are proposing a new pynfs test case, which I will have to remember the thought behind it and make sure that's covered for NFS v3 when I get back to the pynfs 3 tests.
Frank
> -----Original Message-----
> From: Chuck Lever [mailto:[email protected]]
> Sent: Thursday, January 27, 2022 8:08 AM
> To: [email protected]; [email protected]
> Subject: [PATCH RFC 0/6] NFSD size, offset, and count sanity
>
> Dan Aloni reported a problem with the way NFSD's READ implementation deals
> with the very upper end of file sizes, and I got interested in how some of the
> other operations handled it. I found some issues, and have started a (growing)
> pile of patches to deal with them.
>
> Since at least the SETATTR case appears to cause a crash on some filesystems, I
> think several of these are 5.17-rc fodder (i.e., priority bug fixes). I see that NLM
> also has potential problems with how the max file size is handled, but since
> locking doesn't involve the page cache, I think fixes in that area can be delayed a
> bit.
>
> Dan's still working on the READ issue. I need some input on whether I
> understand the problem correctly and whether the NFS status codes I've chosen
> to use are going to be reasonable or a problem for NFS clients. I've attempted to
> stay within the bound of the NFS specs, but sometimes the spec doesn't provide
> a mechanism in the protocol to indicate that the client passed us a bogus
> size/offset/count.
>
> ---
>
> Chuck Lever (6):
> NFSD: Fix NFSv4 SETATTR's handling of large file sizes
> NFSD: Fix NFSv3 SETATTR's handling of large file sizes
> NFSD: COMMIT operations must not return NFS?ERR_INVAL
> NFSD: Replace directory offset placeholder
> NFSD: Remove NFS_OFFSET_MAX
> NFSD: Clamp WRITE offsets
>
>
> fs/nfsd/nfs3proc.c | 32 +++++++++++++++++++++------
> fs/nfsd/nfs3xdr.c | 4 ++--
> fs/nfsd/nfs4proc.c | 7 +++++-
> fs/nfsd/nfs4xdr.c | 2 +-
> fs/nfsd/vfs.c | 53 ++++++++++++++++++++++++++++++---------------
> fs/nfsd/vfs.h | 4 ++--
> include/linux/nfs.h | 8 -------
> 7 files changed, 72 insertions(+), 38 deletions(-)
>
> --
> Chuck Lever