NFSv4: fix getacl ERANGE for some ACL buffer sizes

PATCH In review Weston Andros Adamson 4 participants 12 messages Nov 21, 2013 – Dec 24, 2015

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Weston Andros Adamson

author

2013-11-21 21:20 UTC #0 lore raw

PATCH v2 1/1 NFSv4: fix getacl ERANGE for some ACL buffer sizes

Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.

Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.

This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.

Signed-off-by Weston Andros Adamson <[email protected]>

---

Version 2 of this patch changes things up a bit:

- rely on xdr_partial_copy_from_skb to allocate pages as needed
- always allocate 1 page as there will be some data
- allow xdr_partial_copy_from_skb to allocate up to NFS4ACL_MAXPAGES + 1 pages to hold the max size ACL and the extra page for the bitmap and acl length
- this allows the first request for an ACL to be cached - not just when it fits in one page

 fs/nfs/nfs4proc.c | 40 +++++++++++++++-------------------------
 fs/nfs/nfs4xdr.c  |  2 --
 2 files changed, 15 insertions(+), 27 deletions(-)

fs/nfs/nfs4proc.c +15 ~~-25~~

index ca36d0d..dc8e4f5 100644
@@ -4430,20 +4430,17 @@ out:
 /*
  * The getxattr API returns the required buffer length when called with a
  * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
- * the required buf.  On a NULL buf, we send a page of data to the server
- * guessing that the ACL request can be serviced by a page. If so, we cache
- * up to the page of ACL data, and the 2nd call to getxattr is serviced by
- * the cache. If not so, we throw away the page, and cache the required
- * length. The next getxattr call will then produce another round trip to
- * the server, this time with the input buf of the required size.
+ * the required buf. Cache the result from the first getxattr call to avoid
+ * sending another RPC.
  */
 static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
-	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
+	/* enough pages to hold ACL data plus the bitmap and acl length */
+	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
+		/* The xdr layer may allocate pages here. */
 		.acl_pages = pages,
-		.acl_len = buflen,
 	};
 	struct nfs_getaclres res = {
 		.acl_len = buflen,

@@ -4453,32 +4450,25 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
-	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
 	int ret = -ENOMEM, i;
 
-	/* As long as we're doing a round trip to the server anyway,
-	 * let's be prepared for a page of acl data. */
-	if (npages == 0)
-		npages = 1;
-	if (npages > ARRAY_SIZE(pages))
-		return -ERANGE;
-
-	for (i = 0; i < npages; i++) {
-		pages[i] = alloc_page(GFP_KERNEL);
-		if (!pages[i])
-			goto out_free;
-	}
+	/*
+	 * There will be some data returned by the server, how much is not
+	 * known yet. Allocate one page and let the XDR layer allocate
+	 * more if needed.
+	 */
+	pages[0] = alloc_page(GFP_KERNEL);
 
 	/* for decoding across pages */
 	res.acl_scratch = alloc_page(GFP_KERNEL);
 	if (!res.acl_scratch)
 		goto out_free;
 
-	args.acl_len = npages * PAGE_SIZE;
+	args.acl_len = ARRAY_SIZE(pages) << PAGE_SHIFT;
 	args.acl_pgbase = 0;
 
-	dprintk("%s  buf %p buflen %zu npages %d args.acl_len %zu\n",
-		__func__, buf, buflen, npages, args.acl_len);
+	dprintk("%s  buf %p buflen %zu args.acl_len %zu\n",
+		__func__, buf, buflen, args.acl_len);
 	ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
 			     &msg, &args.seq_args, &res.seq_res, 0);
 	if (ret)

@@ -4503,7 +4493,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 out_ok:
 	ret = res.acl_len;
 out_free:
-	for (i = 0; i < npages; i++)
+	for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
 		if (pages[i])
 			__free_page(pages[i]);
 	if (res.acl_scratch)

fs/nfs/nfs4xdr.c +0 -2

index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
-	xdr_enter_page(xdr, xdr->buf->page_len);
-
 	/* Calculate the offset of the page data */
 	pg_offset = xdr->buf->head[0].iov_len;

Signature · 1 line

1.8.3.1 (Apple Git-46)

Trond Myklebust

2013-12-10 18:13 UTC #1 lore raw

Quoted 40 lines · Weston Andros Adamson

Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.
This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.
Signed-off-by Weston Andros Adamson <[email protected]>
---
Version 2 of this patch changes things up a bit:
- rely on xdr_partial_copy_from_skb to allocate pages as needed
- always allocate 1 page as there will be some data
- allow xdr_partial_copy_from_skb to allocate up to NFS4ACL_MAXPAGES + 1 pages to hold the max size ACL and the extra page for the bitmap and acl length
- this allows the first request for an ACL to be cached - not just when it fits in one page
 fs/nfs/nfs4proc.c | 40 +++++++++++++++-------------------------
 fs/nfs/nfs4xdr.c  |  2 --
 2 files changed, 15 insertions(+), 27 deletions(-)
fs/nfs/nfs4proc.c +0 -4
index ca36d0d..dc8e4f5 100644
@@ -4430,20 +4430,17 @@ out:
 /*
  * The getxattr API returns the required buffer length when called with a
  * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
- * the required buf.  On a NULL buf, we send a page of data to the server
- * guessing that the ACL request can be serviced by a page. If so, we cache
- * up to the page of ACL data, and the 2nd call to getxattr is serviced by
- * the cache. If not so, we throw away the page, and cache the required

fs/nfs/nfs4proc.c out: · line 4439

Quoted 10 lines

fs/nfs/nfs4proc.c +4 -3

- * length. The next getxattr call will then produce another round trip to
- * the server, this time with the input buf of the required size.
+ * the required buf. Cache the result from the first getxattr call to avoid
+ * sending another RPC.
  */
 static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
-	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
+	/* enough pages to hold ACL data plus the bitmap and acl length */
+	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };

Why the change to the array length? From what I can see, you are making everything else depend on ARRAY_LENGTH(pages).

Quoted 51 lines

fs/nfs/nfs4proc.c +10 ~~-17~~

 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
+		/* The xdr layer may allocate pages here. */
 		.acl_pages = pages,
-		.acl_len = buflen,
 	};
 	struct nfs_getaclres res = {
 		.acl_len = buflen,

@@ -4453,32 +4450,25 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
-	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
 	int ret = -ENOMEM, i;
 
-	/* As long as we're doing a round trip to the server anyway,
-	 * let's be prepared for a page of acl data. */
-	if (npages == 0)
-		npages = 1;
-	if (npages > ARRAY_SIZE(pages))
-		return -ERANGE;
-
-	for (i = 0; i < npages; i++) {
-		pages[i] = alloc_page(GFP_KERNEL);
-		if (!pages[i])
-			goto out_free;
-	}
+	/*
+	 * There will be some data returned by the server, how much is not
+	 * known yet. Allocate one page and let the XDR layer allocate
+	 * more if needed.
+	 */
+	pages[0] = alloc_page(GFP_KERNEL);
 
 	/* for decoding across pages */
 	res.acl_scratch = alloc_page(GFP_KERNEL);
 	if (!res.acl_scratch)
 		goto out_free;
 
-	args.acl_len = npages * PAGE_SIZE;
+	args.acl_len = ARRAY_SIZE(pages) << PAGE_SHIFT;
 	args.acl_pgbase = 0;
 
-	dprintk("%s  buf %p buflen %zu npages %d args.acl_len %zu\n",
-		__func__, buf, buflen, npages, args.acl_len);
+	dprintk("%s  buf %p buflen %zu args.acl_len %zu\n",
+		__func__, buf, buflen, args.acl_len);
 	ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
 			     &msg, &args.seq_args, &res.seq_res, 0);
 	if (ret)

fs/nfs/nfs4proc.c static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu · line 4498

Quoted 7 lines

fs/nfs/nfs4proc.c +1 -1

@@ -4503,7 +4493,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 out_ok:
 	ret = res.acl_len;
 out_free:
-	for (i = 0; i < npages; i++)
+	for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
 		if (pages[i])
 			__free_page(pages[i]);

What happens now when the server tries to return an ACL that won't fit in the above array? Do we still return ERANGE?

Also, are we still safe w.r.t. checking that we don't copy more than 'buflen' bytes of data?

fs/nfs/nfs4xdr.c static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, · line 5210

Quoted 10 lines

fs/nfs/nfs4proc.c +0 -0

 	if (res.acl_scratch)

fs/nfs/nfs4xdr.c +0 -1

index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
-	xdr_enter_page(xdr, xdr->buf->page_len);

Why is this being removed?

Quoted 4 lines

fs/nfs/nfs4xdr.c +0 -1

-
 	/* Calculate the offset of the page data */
 	pg_offset = xdr->buf->head[0].iov_len;

Weston Andros Adamson

author

2013-12-10 19:11 UTC #2 lore raw

Quoted 46 lines · Trond Myklebust

Quoted 45 lines · Weston Andros Adamson
Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.
This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.
Signed-off-by Weston Andros Adamson <[email protected]>
---
Version 2 of this patch changes things up a bit:
- rely on xdr_partial_copy_from_skb to allocate pages as needed
- always allocate 1 page as there will be some data
- allow xdr_partial_copy_from_skb to allocate up to NFS4ACL_MAXPAGES + 1 pages to hold the max size ACL and the extra page for the bitmap and acl length
- this allows the first request for an ACL to be cached - not just when it fits in one page
fs/nfs/nfs4proc.c | 40 +++++++++++++++------------------------- fs/nfs/nfs4xdr.c | 2 --
2 files changed, 15 insertions(+), 27 deletions(-)
fs/nfs/nfs4proc.c +2 -6
index ca36d0d..dc8e4f5 100644
@@ -4430,20 +4430,17 @@ out:
 /*
 * The getxattr API returns the required buffer length when called with a
 * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
- * the required buf.  On a NULL buf, we send a page of data to the server
- * guessing that the ACL request can be serviced by a page. If so, we cache
- * up to the page of ACL data, and the 2nd call to getxattr is serviced by
- * the cache. If not so, we throw away the page, and cache the required
- * length. The next getxattr call will then produce another round trip to
- * the server, this time with the input buf of the required size.
+ * the required buf. Cache the result from the first getxattr call to avoid
+ * sending another RPC.
 */

Quoted 8 lines

fs/nfs/nfs4proc.c out: · line 4439
Quoted 5 lines
fs/nfs/nfs4proc.c +2 -1
 static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
-	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
+	/* enough pages to hold ACL data plus the bitmap and acl length */
+	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
Why the change to the array length? From what I can see, you are making everything else depend on ARRAY_LENGTH(pages).

So we can fit NFS4ACL_MAXPAGES (which is XATTR_SIZE_MAX / PAGE_SIZE) AND the bitmap (which being variable length must also use the inline pages), otherwise we will always fail for ACL data sized near XATTR_SIZE_MAX, even though we should support it. I figure that a page is more than enough for the bitmap even when future-proofing nfsv4.X, but we’ll always need that extra page when the actual ACL data is XATTR_SIZE bytes long.

Quoted 54 lines

fs/nfs/nfs4proc.c +10 ~~-17~~

 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
+		/* The xdr layer may allocate pages here. */
 		.acl_pages = pages,
-		.acl_len = buflen,
 	};
 	struct nfs_getaclres res = {
 		.acl_len = buflen,

@@ -4453,32 +4450,25 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
-	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
 	int ret = -ENOMEM, i;
 
-	/* As long as we're doing a round trip to the server anyway,
-	 * let's be prepared for a page of acl data. */
-	if (npages == 0)
-		npages = 1;
-	if (npages > ARRAY_SIZE(pages))
-		return -ERANGE;
-
-	for (i = 0; i < npages; i++) {
-		pages[i] = alloc_page(GFP_KERNEL);
-		if (!pages[i])
-			goto out_free;
-	}
+	/*
+	 * There will be some data returned by the server, how much is not
+	 * known yet. Allocate one page and let the XDR layer allocate
+	 * more if needed.
+	 */
+	pages[0] = alloc_page(GFP_KERNEL);
 
 	/* for decoding across pages */
 	res.acl_scratch = alloc_page(GFP_KERNEL);
 	if (!res.acl_scratch)
 		goto out_free;
 
-	args.acl_len = npages * PAGE_SIZE;
+	args.acl_len = ARRAY_SIZE(pages) << PAGE_SHIFT;
 	args.acl_pgbase = 0;
 
-	dprintk("%s  buf %p buflen %zu npages %d args.acl_len %zu\n",
-		__func__, buf, buflen, npages, args.acl_len);
+	dprintk("%s  buf %p buflen %zu args.acl_len %zu\n",
+		__func__, buf, buflen, args.acl_len);
 	ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
 			     &msg, &args.seq_args, &res.seq_res, 0);
 	if (ret)

@@ -4503,7 +4493,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu

out_ok:

	ret = res.acl_len;

Quoted 8 lines

Quoted 5 lines
out_free:
fs/nfs/nfs4proc.c +1 -1
-	for (i = 0; i < npages; i++)
+	for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
		if (pages[i])
			__free_page(pages[i]);
What happens now when the server tries to return an ACL that won't fit in the above array? Do we still return ERANGE?

Yes, the XDR layer will set NFS4_ACL_TRUNC if the ACL data doesn’t fit in args.acl_len, and __nfs4_get_acl_uncached still checks for this flag, returning -ERANGE if set.

Quoted 3 lines

Also, are we still safe w.r.t. checking that we don't copy more than 'buflen' bytes of data?

Yes, the xdr inline page filler enforces this - and we set the max in nfs4_xdr_enc_getacl:

      xdr_inline_pages(&req->rq_rcv_buf, replen << 2,
                args->acl_pages, args->acl_pgbase, args->acl_len);

Quoted 10 lines

if (res.acl_scratch)

fs/nfs/nfs4xdr.c +0 -0

index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;

Quoted 3 lines

fs/nfs/nfs4xdr.c static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, · line 5210
Quoted 1 line
fs/nfs/nfs4xdr.c +0 -1
-	xdr_enter_page(xdr, xdr->buf->page_len);
Why is this being removed?

Because it BUG()s! ;)

I must admit that I don’t get why xdr_enter_page() doesn’t work when the XDR layer does the page allocation, but it does not — and the nfs3xdr ACL code (which uses the XDR layer allocation) also doesn’t call this. It worked well without this, so I just removed it and didn’t investigate why.

-dros

Quoted 11 lines

fs/nfs/nfs4xdr.c static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, · line 5212
Quoted 5 lines
fs/nfs/nfs4xdr.c +0 -1
-
 	/* Calculate the offset of the page data */
 	pg_offset = xdr->buf->head[0].iov_len;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Trond Myklebust

2013-12-10 19:19 UTC #3 lore raw

Quoted 15 lines · Weston Andros Adamson

Quoted 12 lines · Trond Myklebust
Quoted 9 lines · Weston Andros Adamson
if (res.acl_scratch)
fs/nfs/nfs4xdr.c +0 -0
index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 

Quoted 7 lines

Quoted 3 lines
fs/nfs/nfs4xdr.c static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, · line 5210
Quoted 1 line
fs/nfs/nfs4xdr.c +0 -1
-	xdr_enter_page(xdr, xdr->buf->page_len);
Why is this being removed?
Because it BUG()s! ;)
I must admit that I don’t get why xdr_enter_page() doesn’t work when the XDR layer does the page allocation, but it does not — and the nfs3xdr ACL code (which uses the XDR layer allocation) also doesn’t call this. It worked well without this, so I just removed it and didn’t investigate why.

I’m guessing that xdr->buf>page_len is being set incorrectly by the code that does the page allocation and/or that it is failing to update xdr->buf->len and/or xdr->buf->buflen.

We really do need the xdr_enter_page() here, or the acl bitmap + data won’t be aligned properly on the page boundary.

Cheers
Trond

Weston Andros Adamson

author

2013-12-10 20:10 UTC #4 lore raw

Quoted 22 lines · Trond Myklebust

Quoted 19 lines · Weston Andros Adamson
Quoted 15 lines · Trond Myklebust
fs/nfs/nfs4xdr.c static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, · line 5210
Quoted 10 lines · Weston Andros Adamson
if (res.acl_scratch)
fs/nfs/nfs4xdr.c +0 -1
index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
-	xdr_enter_page(xdr, xdr->buf->page_len);
Why is this being removed?

Quoted 7 lines

Quoted 3 lines
Because it BUG()s! ;)
I must admit that I don’t get why xdr_enter_page() doesn’t work when the XDR layer does the page allocation, but it does not — and the nfs3xdr ACL code (which uses the XDR layer allocation) also doesn’t call this. It worked well without this, so I just removed it and didn’t investigate why.
I’m guessing that xdr->buf>page_len is being set incorrectly by the code that does the page allocation and/or that it is failing to update xdr->buf->len and/or xdr->buf->buflen.
We really do need the xdr_enter_page() here, or the acl bitmap + data won’t be aligned properly on the page boundary.

Ok, I’ll add that back and then fix the oops… and investigate if the v3 ACL code needs it too…

Thanks,
-dros

Weston Andros Adamson

author

2013-12-11 20:29 UTC #5 lore raw

Indeed, buf->page_len is still set to the maximum number of bytes in the pages array (set by xdr_inline_pages to tell xdr layer the max allocation size), and not the number of bytes that are actually present.

Working on a fix.

-dros

Quoted 43 lines · Weston Andros Adamson

Quoted 22 lines · Trond Myklebust
Quoted 19 lines · Weston Andros Adamson
Quoted 15 lines · Trond Myklebust
fs/nfs/nfs4xdr.c static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req, · line 5210
Quoted 10 lines · Weston Andros Adamson
if (res.acl_scratch)
fs/nfs/nfs4xdr.c +0 -1
index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
-	xdr_enter_page(xdr, xdr->buf->page_len);
Why is this being removed?
Quoted 7 lines
Quoted 3 lines
Because it BUG()s! ;)
I must admit that I don’t get why xdr_enter_page() doesn’t work when the XDR layer does the page allocation, but it does not — and the nfs3xdr ACL code (which uses the XDR layer allocation) also doesn’t call this. It worked well without this, so I just removed it and didn’t investigate why.
I’m guessing that xdr->buf>page_len is being set incorrectly by the code that does the page allocation and/or that it is failing to update xdr->buf->len and/or xdr->buf->buflen.
We really do need the xdr_enter_page() here, or the acl bitmap + data won’t be aligned properly on the page boundary.
Ok, I’ll add that back and then fix the oops… and investigate if the v3 ACL code needs it too…
Thanks,
-dros
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

J. Bruce Fields

2015-12-14 22:18 UTC #6 lore raw

In reply to Weston Andros Adamson

Quoted 11 lines · Weston Andros Adamson

Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.
This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.

Was there some objection to this patch, or did it just get lost in the shuffle?

Looks like it's still a bug upstream. And it's easier to reproduce now that we've got a) numeric id's and b) a server that can handle larger ACLs. I'm reliably reproducing with:

	mount -onoac f21-1:/exports/xfs2 /mnt/
	echo "A::OWNER@:RW" >ACL
	for ((i=9802; i < 10002; i++)); do echo "A::$i:RW" >>ACL; done
	echo "A::GROUP@:RW" >>ACL
	echo "A::EVERYONE@:RW" >>ACL
	nfs4_setfacl -S ACL /mnt/TEST
	strace -e getxattr nfs4_getfacl /mnt/TEST >/dev/null

which gets me

	getxattr("/mnt/TEST", "system.nfs4_acl", 0x0, 0) = 4088
	getxattr("/mnt/TEST", "system.nfs4_acl", 0x564186d89010, 4088) = -1 ERANGE (Numerical result out of range)

--b.

Quoted 118 lines

Signed-off-by Weston Andros Adamson <[email protected]>

---

Version 2 of this patch changes things up a bit:

 fs/nfs/nfs4proc.c | 40 +++++++++++++++-------------------------
 fs/nfs/nfs4xdr.c  |  2 --
 2 files changed, 15 insertions(+), 27 deletions(-)

fs/nfs/nfs4proc.c +15 ~~-25~~

index ca36d0d..dc8e4f5 100644
@@ -4430,20 +4430,17 @@ out:
 /*
  * The getxattr API returns the required buffer length when called with a
  * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
- * the required buf.  On a NULL buf, we send a page of data to the server
- * guessing that the ACL request can be serviced by a page. If so, we cache
- * up to the page of ACL data, and the 2nd call to getxattr is serviced by
- * the cache. If not so, we throw away the page, and cache the required
- * length. The next getxattr call will then produce another round trip to
- * the server, this time with the input buf of the required size.
+ * the required buf. Cache the result from the first getxattr call to avoid
+ * sending another RPC.
  */
 static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
-	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
+	/* enough pages to hold ACL data plus the bitmap and acl length */
+	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
+		/* The xdr layer may allocate pages here. */
 		.acl_pages = pages,
-		.acl_len = buflen,
 	};
 	struct nfs_getaclres res = {
 		.acl_len = buflen,

@@ -4453,32 +4450,25 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
-	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
 	int ret = -ENOMEM, i;
 
-	/* As long as we're doing a round trip to the server anyway,
-	 * let's be prepared for a page of acl data. */
-	if (npages == 0)
-		npages = 1;
-	if (npages > ARRAY_SIZE(pages))
-		return -ERANGE;
-
-	for (i = 0; i < npages; i++) {
-		pages[i] = alloc_page(GFP_KERNEL);
-		if (!pages[i])
-			goto out_free;
-	}
+	/*
+	 * There will be some data returned by the server, how much is not
+	 * known yet. Allocate one page and let the XDR layer allocate
+	 * more if needed.
+	 */
+	pages[0] = alloc_page(GFP_KERNEL);
 
 	/* for decoding across pages */
 	res.acl_scratch = alloc_page(GFP_KERNEL);
 	if (!res.acl_scratch)
 		goto out_free;
 
-	args.acl_len = npages * PAGE_SIZE;
+	args.acl_len = ARRAY_SIZE(pages) << PAGE_SHIFT;
 	args.acl_pgbase = 0;
 
-	dprintk("%s  buf %p buflen %zu npages %d args.acl_len %zu\n",
-		__func__, buf, buflen, npages, args.acl_len);
+	dprintk("%s  buf %p buflen %zu args.acl_len %zu\n",
+		__func__, buf, buflen, args.acl_len);
 	ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
 			     &msg, &args.seq_args, &res.seq_res, 0);
 	if (ret)

@@ -4503,7 +4493,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 out_ok:
 	ret = res.acl_len;
 out_free:
-	for (i = 0; i < npages; i++)
+	for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
 		if (pages[i])
 			__free_page(pages[i]);
 	if (res.acl_scratch)

fs/nfs/nfs4xdr.c +0 -2

index 5be2868..4e2e2da 100644
@@ -5207,8 +5207,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
-	xdr_enter_page(xdr, xdr->buf->page_len);
-
 	/* Calculate the offset of the page data */
 	pg_offset = xdr->buf->head[0].iov_len;

Signature · 6 lines

1.8.3.1 (Apple Git-46)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

J. Bruce Fields

2015-12-14 22:21 UTC #7 lore raw

PATCH 1/1 NFSv4: fix getacl ERANGE for some ACL buffer sizes

From: Weston Andros Adamson <[email protected]>

Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.

Signed-off-by Weston Andros Adamson <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfs/nfs4proc.c | 40 +++++++++++++++-------------------------
 fs/nfs/nfs4xdr.c  |  2 --
 2 files changed, 15 insertions(+), 27 deletions(-)

Quoted 3 lines · bfields

Looks like it's still a bug upstream. And it's easier to reproduce now that we've got a) numeric id's and b) a server that can handle larger ACLs. I'm reliably reproducing with:

And the same patch still applies after minor conflict resolution; updated patch below. I've confirmed that it eliminates the ERANGE.

fs/nfs/nfs4proc.c +15 ~~-25~~

index 765a03559363..2e679c295185 100644
@@ -4702,20 +4702,17 @@ out:
 /*
  * The getxattr API returns the required buffer length when called with a
  * NULL buf. The NFSv4 acl tool then calls getxattr again after allocating
- * the required buf.  On a NULL buf, we send a page of data to the server
- * guessing that the ACL request can be serviced by a page. If so, we cache
- * up to the page of ACL data, and the 2nd call to getxattr is serviced by
- * the cache. If not so, we throw away the page, and cache the required
- * length. The next getxattr call will then produce another round trip to
- * the server, this time with the input buf of the required size.
+ * the required buf. Cache the result from the first getxattr call to avoid
+ * sending another RPC.
  */
 static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
-	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
+	/* enough pages to hold ACL data plus the bitmap and acl length */
+	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
+		/* The xdr layer may allocate pages here. */
 		.acl_pages = pages,
-		.acl_len = buflen,
 	};
 	struct nfs_getaclres res = {
 		.acl_len = buflen,

@@ -4725,31 +4722,24 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
-	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
 	int ret = -ENOMEM, i;
 
-	/* As long as we're doing a round trip to the server anyway,
-	 * let's be prepared for a page of acl data. */
-	if (npages == 0)
-		npages = 1;
-	if (npages > ARRAY_SIZE(pages))
-		return -ERANGE;
-
-	for (i = 0; i < npages; i++) {
-		pages[i] = alloc_page(GFP_KERNEL);
-		if (!pages[i])
-			goto out_free;
-	}
+	/*
+	 * There will be some data returned by the server, how much is not
+	 * known yet. Allocate one page and let the XDR layer allocate
+	 * more if needed.
+	 */
+	pages[0] = alloc_page(GFP_KERNEL);
 
 	/* for decoding across pages */
 	res.acl_scratch = alloc_page(GFP_KERNEL);
 	if (!res.acl_scratch)
 		goto out_free;
 
-	args.acl_len = npages * PAGE_SIZE;
+	args.acl_len = ARRAY_SIZE(pages) << PAGE_SHIFT;
 
-	dprintk("%s  buf %p buflen %zu npages %d args.acl_len %zu\n",
-		__func__, buf, buflen, npages, args.acl_len);
+	dprintk("%s  buf %p buflen %zu args.acl_len %zu\n",
+		__func__, buf, buflen, args.acl_len);
 	ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
 			     &msg, &args.seq_args, &res.seq_res, 0);
 	if (ret)

@@ -4774,7 +4764,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 out_ok:
 	ret = res.acl_len;
 out_free:
-	for (i = 0; i < npages; i++)
+	for (i = 0; i < ARRAY_SIZE(pages) && pages[i]; i++)
 		if (pages[i])
 			__free_page(pages[i]);
 	if (res.acl_scratch)

fs/nfs/nfs4xdr.c +0 -2

index dfed4f5c8fcc..35d15004b0d6 100644
@@ -5298,8 +5298,6 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 	if ((status = decode_op_hdr(xdr, OP_GETATTR)) != 0)
 		goto out;
 
-	xdr_enter_page(xdr, xdr->buf->page_len);
-
 	/* Calculate the offset of the page data */
 	pg_offset = xdr->buf->head[0].iov_len;

Signature · 1 line

2.5.0

Trond Myklebust

2015-12-14 23:27 UTC #8 lore raw

Quoted 10 lines · J. Bruce Fields

From: Weston Andros Adamson <[email protected]>
Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.

Quoted 6 lines

This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.
Signed-off-by Weston Andros Adamson <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>

The thread ends with:

"Indeed, buf->page_len is still set to the maximum number of bytes in the pages array (set by xdr_inline_pages to tell xdr layer the max allocation size), and not the number of bytes that are actually present.

Working on a fix.

-dros"

So no, I'm not taking this patch until someone fixes the issues identified in the review.

Cheers
Trond

J. Bruce Fields

2015-12-15 14:04 UTC #9 lore raw

Quoted 18 lines · Trond Myklebust

Quoted 10 lines · J. Bruce Fields
From: Weston Andros Adamson <[email protected]>
Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.
Quoted 6 lines
This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.
Signed-off-by Weston Andros Adamson <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>

Quoted 8 lines

The thread ends with:
"Indeed, buf->page_len is still set to the maximum number of bytes in the pages array (set by xdr_inline_pages to tell xdr layer the max allocation size), and not the number of bytes that are actually present.
Working on a fix.

Oh, apologies, for some reason I misfiled the rest of that thread. But I see it in online archives. I'll take a look....

--b.

J. Bruce Fields

2015-12-23 16:58 UTC #10 lore raw

In reply to Trond Myklebust

Quoted 20 lines · Trond Myklebust

Quoted 10 lines · J. Bruce Fields
From: Weston Andros Adamson <[email protected]>
Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.
Quoted 6 lines
This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.
Signed-off-by Weston Andros Adamson <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
The thread ends with:

Quoted 8 lines

"Indeed, buf->page_len is still set to the maximum number of bytes in the pages array (set by xdr_inline_pages to tell xdr layer the max allocation size), and not the number of bytes that are actually present.
Working on a fix.
-dros"

Dros, do you have a reliable reproducer for the BUG you were seeing? I'm not having any luck.

--b.

Jeff Layton

2015-12-24 01:37 UTC #11 lore raw

Quoted 26 lines · On Wed, 23 Dec 2015 11:58:49 -0500 "J. Bruce Fields" <[email protected]>…

Quoted 25 lines · Trond Myklebust
Quoted 10 lines · J. Bruce Fields
From: Weston Andros Adamson <[email protected]>
Fix a bug where getxattr returns ERANGE when the attr buffer length is close enough to the nearest PAGE_SIZE multiple that adding the extra bytes leaves too little room for the ACL buffer.
Besides storing the ACL buffer, the getacl decoder uses the inline pages for the attr bitmap and buffer length. __nfs4_get_acl_uncached must allocate enough page space for all of the data to be decoded.
Quoted 6 lines
This patch uses xdr_partial_copy_from_skb to allocate any needed pages past the first one. This allows the client to always cache the acl on the first getacl call and not just if it fits in one page.
Signed-off-by Weston Andros Adamson <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
The thread ends with:
"Indeed, buf->page_len is still set to the maximum number of bytes in the pages array (set by xdr_inline_pages to tell xdr layer the max allocation size), and not the number of bytes that are actually present.

Quoted 8 lines

Quoted 3 lines
Working on a fix.
-dros"
Dros, do you have a reliable reproducer for the BUG you were seeing? I'm not having any luck.
--b.

I doubt Dros' netapp email addr still works. Sending to his PD one...

Signature · 1 line

Jeff Layton <[email protected]>