Hi all,
I found xfstests generic/053 generic/105 generic/307 and generic/319
started to fail on NFSv3 mount against Linux server running v4.12-rc1
kernel. They all try to set acl (xattr) on file/dir reside on NFSv3
mount. And this is a regression, 4.11 Linux server worked fine. A simple
reproducer would be:
mount linux-server:/export /mnt/nfs
touch /mnt/nfs/testfile
setfattr -n user.test -v helloworld /mnt/nfs/testfile
The last setfattr returned EACCES
setfattr: /mnt/nfs/testfile: Operation not supported
And I bisected this to commit 51f56777
commit 51f567777799c9d85a778302b9eb61cf15214a98
Author: J. Bruce Fields <[email protected]>
Date: Thu Apr 6 22:36:31 2017 -0400
nfsd: check for oversized NFSv2/v3 arguments
And I only need to revert this hunk to "fix" the regression:
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index e770abe..6ef19cf 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -336,8 +336,7 @@ xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
{
char *cp = (char *)p;
struct kvec *vec = &rqstp->rq_arg.head[0];
- return cp >= (char*)vec->iov_base
- && cp <= (char*)vec->iov_base + vec->iov_len;
+ return cp == (char *)vec->iov_base + vec->iov_len;
}
static inline int
Debug log printing out cp, iov_base and iov_base + iov_len confirmed
that cp can be greater than iov_base but smaller than iov_base + iov_len
[10868.819939] xdr_argsize_check: cp == ffff88020ea6f094, iov_base == ffff88020ea6f068, iov_base + iov_len == ffff88020ea6f104
Please help take a look. If you need more info please let me know.
Thanks,
Eryu
On Mon, May 15, 2017 at 07:26:38PM +0800, Eryu Guan wrote:
> I found xfstests generic/053 generic/105 generic/307 and generic/319
> started to fail on NFSv3 mount against Linux server running v4.12-rc1
> kernel. They all try to set acl (xattr) on file/dir reside on NFSv3
> mount. And this is a regression, 4.11 Linux server worked fine. A simple
> reproducer would be:
>
> mount linux-server:/export /mnt/nfs
> touch /mnt/nfs/testfile
> setfattr -n user.test -v helloworld /mnt/nfs/testfile
>
> The last setfattr returned EACCES
>
> setfattr: /mnt/nfs/testfile: Operation not supported
>
> And I bisected this to commit 51f56777
Oh, yikes, very sorry about that, the patch should not have gone
upstream.
This was my original approach to the problem, but I abandoned it in part
because someone pointed out the problem you found. And Neil suggested
the approach used in the better fix, which is upstream as e6838a29ecb
"nfsd: check for oversized NFSv2/v3 arguments".
But somehow the old version must have been left in the tree branch I
gave Linus to pull?
Apologies, I'll take another look to make sure I'm not still confused
about somethign, and then send a revert. And I'll try to keep out an
eye for the stable backports to NAK those....
--b.
>
> commit 51f567777799c9d85a778302b9eb61cf15214a98
> Author: J. Bruce Fields <[email protected]>
> Date: Thu Apr 6 22:36:31 2017 -0400
>
> nfsd: check for oversized NFSv2/v3 arguments
>
> And I only need to revert this hunk to "fix" the regression:
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index e770abe..6ef19cf 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -336,8 +336,7 @@ xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
> {
> char *cp = (char *)p;
> struct kvec *vec = &rqstp->rq_arg.head[0];
> - return cp >= (char*)vec->iov_base
> - && cp <= (char*)vec->iov_base + vec->iov_len;
> + return cp == (char *)vec->iov_base + vec->iov_len;
> }
>
> static inline int
>
> Debug log printing out cp, iov_base and iov_base + iov_len confirmed
> that cp can be greater than iov_base but smaller than iov_base + iov_len
>
> [10868.819939] xdr_argsize_check: cp == ffff88020ea6f094, iov_base == ffff88020ea6f068, iov_base + iov_len == ffff88020ea6f104
>
> Please help take a look. If you need more info please let me know.
>
> Thanks,
> Eryu
From: "J. Bruce Fields" <[email protected]>
This reverts commit 51f567777799 "nfsd: check for oversized NFSv2/v3
arguments", which breaks support for NFSv3 ACLs.
That patch was actually an earlier draft of a fix for the problem that
was eventually fixed by e6838a29ecb "nfsd: check for oversized NFSv2/v3
arguments". But somehow I accidentally left this earlier draft in the
branch that was part of my 2.12 pull request.
Reported-by: Eryu Guan <[email protected]>
Cc: [email protected]
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfsd/nfs3xdr.c | 23 ++++++-----------------
fs/nfsd/nfsxdr.c | 13 +++----------
include/linux/sunrpc/svc.h | 3 ++-
3 files changed, 11 insertions(+), 28 deletions(-)
The stable cc may be overkill as I don't *think* the original's been
backported to stable yet, but it's there just in case I miss NAK'ing the
backports....
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 12feac6ee2fd..452334694a5d 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -334,11 +334,8 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
if (!p)
return 0;
p = xdr_decode_hyper(p, &args->offset);
- args->count = ntohl(*p++);
-
- if (!xdr_argsize_check(rqstp, p))
- return 0;
+ args->count = ntohl(*p++);
len = min(args->count, max_blocksize);
/* set up the kvec */
@@ -352,7 +349,7 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
v++;
}
args->vlen = v;
- return 1;
+ return xdr_argsize_check(rqstp, p);
}
int
@@ -544,11 +541,9 @@ nfs3svc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p,
p = decode_fh(p, &args->fh);
if (!p)
return 0;
- if (!xdr_argsize_check(rqstp, p))
- return 0;
args->buffer = page_address(*(rqstp->rq_next_page++));
- return 1;
+ return xdr_argsize_check(rqstp, p);
}
int
@@ -574,14 +569,10 @@ nfs3svc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
args->verf = p; p += 2;
args->dircount = ~0;
args->count = ntohl(*p++);
-
- if (!xdr_argsize_check(rqstp, p))
- return 0;
-
args->count = min_t(u32, args->count, PAGE_SIZE);
args->buffer = page_address(*(rqstp->rq_next_page++));
- return 1;
+ return xdr_argsize_check(rqstp, p);
}
int
@@ -599,9 +590,6 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
args->dircount = ntohl(*p++);
args->count = ntohl(*p++);
- if (!xdr_argsize_check(rqstp, p))
- return 0;
-
len = args->count = min(args->count, max_blocksize);
while (len > 0) {
struct page *p = *(rqstp->rq_next_page++);
@@ -609,7 +597,8 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
args->buffer = page_address(p);
len -= PAGE_SIZE;
}
- return 1;
+
+ return xdr_argsize_check(rqstp, p);
}
int
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 6a4947a3f4fa..de07ff625777 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -257,9 +257,6 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
len = args->count = ntohl(*p++);
p++; /* totalcount - unused */
- if (!xdr_argsize_check(rqstp, p))
- return 0;
-
len = min_t(unsigned int, len, NFSSVC_MAXBLKSIZE_V2);
/* set up somewhere to store response.
@@ -275,7 +272,7 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
v++;
}
args->vlen = v;
- return 1;
+ return xdr_argsize_check(rqstp, p);
}
int
@@ -365,11 +362,9 @@ nfssvc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p, struct nfsd_readli
p = decode_fh(p, &args->fh);
if (!p)
return 0;
- if (!xdr_argsize_check(rqstp, p))
- return 0;
args->buffer = page_address(*(rqstp->rq_next_page++));
- return 1;
+ return xdr_argsize_check(rqstp, p);
}
int
@@ -407,11 +402,9 @@ nfssvc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
args->cookie = ntohl(*p++);
args->count = ntohl(*p++);
args->count = min_t(u32, args->count, PAGE_SIZE);
- if (!xdr_argsize_check(rqstp, p))
- return 0;
args->buffer = page_address(*(rqstp->rq_next_page++));
- return 1;
+ return xdr_argsize_check(rqstp, p);
}
/*
diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 94631026f79c..11cef5a7bc87 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -336,7 +336,8 @@ xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
{
char *cp = (char *)p;
struct kvec *vec = &rqstp->rq_arg.head[0];
- return cp == (char *)vec->iov_base + vec->iov_len;
+ return cp >= (char*)vec->iov_base
+ && cp <= (char*)vec->iov_base + vec->iov_len;
}
static inline int
--
2.9.3
On Tue, May 16, 2017 at 04:19:44PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> This reverts commit 51f567777799 "nfsd: check for oversized NFSv2/v3
> arguments", which breaks support for NFSv3 ACLs.
>
> That patch was actually an earlier draft of a fix for the problem that
> was eventually fixed by e6838a29ecb "nfsd: check for oversized NFSv2/v3
> arguments". But somehow I accidentally left this earlier draft in the
> branch that was part of my 2.12 pull request.
>
> Reported-by: Eryu Guan <[email protected]>
> Cc: [email protected]
> Signed-off-by: J. Bruce Fields <[email protected]>
Tested with generic/053 generic/105 generic/307 generic/319 and all
passed with NFSv3. Thanks!
Tested-by: Eryu Guan <[email protected]>
> ---
> fs/nfsd/nfs3xdr.c | 23 ++++++-----------------
> fs/nfsd/nfsxdr.c | 13 +++----------
> include/linux/sunrpc/svc.h | 3 ++-
> 3 files changed, 11 insertions(+), 28 deletions(-)
>
> The stable cc may be overkill as I don't *think* the original's been
> backported to stable yet, but it's there just in case I miss NAK'ing the
> backports....
>
> diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
> index 12feac6ee2fd..452334694a5d 100644
> --- a/fs/nfsd/nfs3xdr.c
> +++ b/fs/nfsd/nfs3xdr.c
> @@ -334,11 +334,8 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
> if (!p)
> return 0;
> p = xdr_decode_hyper(p, &args->offset);
> - args->count = ntohl(*p++);
> -
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
>
> + args->count = ntohl(*p++);
> len = min(args->count, max_blocksize);
>
> /* set up the kvec */
> @@ -352,7 +349,7 @@ nfs3svc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
> v++;
> }
> args->vlen = v;
> - return 1;
> + return xdr_argsize_check(rqstp, p);
> }
>
> int
> @@ -544,11 +541,9 @@ nfs3svc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p,
> p = decode_fh(p, &args->fh);
> if (!p)
> return 0;
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
> args->buffer = page_address(*(rqstp->rq_next_page++));
>
> - return 1;
> + return xdr_argsize_check(rqstp, p);
> }
>
> int
> @@ -574,14 +569,10 @@ nfs3svc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
> args->verf = p; p += 2;
> args->dircount = ~0;
> args->count = ntohl(*p++);
> -
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
> -
> args->count = min_t(u32, args->count, PAGE_SIZE);
> args->buffer = page_address(*(rqstp->rq_next_page++));
>
> - return 1;
> + return xdr_argsize_check(rqstp, p);
> }
>
> int
> @@ -599,9 +590,6 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
> args->dircount = ntohl(*p++);
> args->count = ntohl(*p++);
>
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
> -
> len = args->count = min(args->count, max_blocksize);
> while (len > 0) {
> struct page *p = *(rqstp->rq_next_page++);
> @@ -609,7 +597,8 @@ nfs3svc_decode_readdirplusargs(struct svc_rqst *rqstp, __be32 *p,
> args->buffer = page_address(p);
> len -= PAGE_SIZE;
> }
> - return 1;
> +
> + return xdr_argsize_check(rqstp, p);
> }
>
> int
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index 6a4947a3f4fa..de07ff625777 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -257,9 +257,6 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
> len = args->count = ntohl(*p++);
> p++; /* totalcount - unused */
>
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
> -
> len = min_t(unsigned int, len, NFSSVC_MAXBLKSIZE_V2);
>
> /* set up somewhere to store response.
> @@ -275,7 +272,7 @@ nfssvc_decode_readargs(struct svc_rqst *rqstp, __be32 *p,
> v++;
> }
> args->vlen = v;
> - return 1;
> + return xdr_argsize_check(rqstp, p);
> }
>
> int
> @@ -365,11 +362,9 @@ nfssvc_decode_readlinkargs(struct svc_rqst *rqstp, __be32 *p, struct nfsd_readli
> p = decode_fh(p, &args->fh);
> if (!p)
> return 0;
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
> args->buffer = page_address(*(rqstp->rq_next_page++));
>
> - return 1;
> + return xdr_argsize_check(rqstp, p);
> }
>
> int
> @@ -407,11 +402,9 @@ nfssvc_decode_readdirargs(struct svc_rqst *rqstp, __be32 *p,
> args->cookie = ntohl(*p++);
> args->count = ntohl(*p++);
> args->count = min_t(u32, args->count, PAGE_SIZE);
> - if (!xdr_argsize_check(rqstp, p))
> - return 0;
> args->buffer = page_address(*(rqstp->rq_next_page++));
>
> - return 1;
> + return xdr_argsize_check(rqstp, p);
> }
>
> /*
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index 94631026f79c..11cef5a7bc87 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -336,7 +336,8 @@ xdr_argsize_check(struct svc_rqst *rqstp, __be32 *p)
> {
> char *cp = (char *)p;
> struct kvec *vec = &rqstp->rq_arg.head[0];
> - return cp == (char *)vec->iov_base + vec->iov_len;
> + return cp >= (char*)vec->iov_base
> + && cp <= (char*)vec->iov_base + vec->iov_len;
> }
>
> static inline int
> --
> 2.9.3
>