[PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[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 <dros@netapp.com>
---

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(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index ca36d0d..dc8e4f5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 5be2868..4e2e2da 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -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;
 
-- 
1.8.3.1 (Apple Git-46)

Re: [PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Trond Myklebust]

On Thu, 2013-11-21 at 16:20 -0500, Weston Andros Adamson wrote:
> 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 <dros@netapp.com>
> ---
> 
> 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(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ca36d0d..dc8e4f5 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -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, };

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

>  	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]);

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?

>  	if (res.acl_scratch)
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5be2868..4e2e2da 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -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?

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

Re: [PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

On Dec 10, 2013, at 1:13 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> On Thu, 2013-11-21 at 16:20 -0500, Weston Andros Adamson wrote:
>> 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 <dros@netapp.com>
>> ---
>> 
>> 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(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index ca36d0d..dc8e4f5 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -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, };
> 
> 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. 

> 
>> 	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]);
> 
> 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.

> 
> 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);

> 
>> 	if (res.acl_scratch)
>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>> index 5be2868..4e2e2da 100644
>> --- a/fs/nfs/nfs4xdr.c
>> +++ b/fs/nfs/nfs4xdr.c
>> @@ -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?

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

> 
>> -
>> 	/* 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Trond Myklebust]

On Dec 10, 2013, at 21:11, Weston Andros Adamson <dros@netapp.com> wrote:

> 
> On Dec 10, 2013, at 1:13 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
>> On Thu, 2013-11-21 at 16:20 -0500, Weston Andros Adamson wrote:
> 
>> 
>>> 	if (res.acl_scratch)
>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>> index 5be2868..4e2e2da 100644
>>> --- a/fs/nfs/nfs4xdr.c
>>> +++ b/fs/nfs/nfs4xdr.c
>>> @@ -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?
> 
> 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

Re: [PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

On Dec 10, 2013, at 2:19 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:

> 
> On Dec 10, 2013, at 21:11, Weston Andros Adamson <dros@netapp.com> wrote:
> 
>> 
>> On Dec 10, 2013, at 1:13 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>> 
>>> On Thu, 2013-11-21 at 16:20 -0500, Weston Andros Adamson wrote:
>> 
>>> 
>>>> 	if (res.acl_scratch)
>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>> index 5be2868..4e2e2da 100644
>>>> --- a/fs/nfs/nfs4xdr.c
>>>> +++ b/fs/nfs/nfs4xdr.c
>>>> @@ -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?
>> 
>> 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

Re: [PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

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

On Dec 10, 2013, at 3:10 PM, Weston Andros Adamson <dros@netapp.com> wrote:

> 
> On Dec 10, 2013, at 2:19 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
> 
>> 
>> On Dec 10, 2013, at 21:11, Weston Andros Adamson <dros@netapp.com> wrote:
>> 
>>> 
>>> On Dec 10, 2013, at 1:13 PM, Trond Myklebust <trond.myklebust@primarydata.com> wrote:
>>> 
>>>> On Thu, 2013-11-21 at 16:20 -0500, Weston Andros Adamson wrote:
>>> 
>>>> 
>>>>> 	if (res.acl_scratch)
>>>>> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
>>>>> index 5be2868..4e2e2da 100644
>>>>> --- a/fs/nfs/nfs4xdr.c
>>>>> +++ b/fs/nfs/nfs4xdr.c
>>>>> @@ -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?
>>> 
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[J. Bruce Fields]

On Thu, Nov 21, 2013 at 04:20:13PM -0500, Weston Andros Adamson wrote:
> 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.

> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
> 
> 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(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index ca36d0d..dc8e4f5 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -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)
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 5be2868..4e2e2da 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -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;
>  
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[J. Bruce Fields]

From: Weston Andros Adamson <dros@netapp.com>

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 <dros@netapp.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/nfs4proc.c | 40 +++++++++++++++-------------------------
 fs/nfs/nfs4xdr.c  |  2 --
 2 files changed, 15 insertions(+), 27 deletions(-)

On Mon, Dec 14, 2015 at 05:18:16PM -0500, bfields wrote:
> 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.

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 765a03559363..2e679c295185 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index dfed4f5c8fcc..35d15004b0d6 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -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;
 
-- 
2.5.0

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Trond Myklebust]

On Mon, Dec 14, 2015 at 5:21 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> From: Weston Andros Adamson <dros@netapp.com>
>
> 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 <dros@netapp.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

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

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[J. Bruce Fields]

On Mon, Dec 14, 2015 at 06:27:20PM -0500, Trond Myklebust wrote:
> On Mon, Dec 14, 2015 at 5:21 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > From: Weston Andros Adamson <dros@netapp.com>
> >
> > 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 <dros@netapp.com>
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> 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.

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[J. Bruce Fields]

On Mon, Dec 14, 2015 at 06:27:20PM -0500, Trond Myklebust wrote:
> On Mon, Dec 14, 2015 at 5:21 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > From: Weston Andros Adamson <dros@netapp.com>
> >
> > 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 <dros@netapp.com>
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> 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"

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

--b.

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Jeff Layton]

On Wed, 23 Dec 2015 11:58:49 -0500
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Mon, Dec 14, 2015 at 06:27:20PM -0500, Trond Myklebust wrote:
> > On Mon, Dec 14, 2015 at 5:21 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > > From: Weston Andros Adamson <dros@netapp.com>
> > >
> > > 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 <dros@netapp.com>
> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > 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"
> 
> 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...

-- 
Jeff Layton <jeff.layton@primarydata.com>

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

On Nov 21, 2013, at 9:56 AM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> 
> On Nov 21, 2013, at 9:33, Weston Andros Adamson <dros@netapp.com> wrote:
> 
>> On Nov 16, 2013, at 3:12 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:
>> 
>>> 
>>> On Nov 16, 2013, at 14:15, Weston Andros Adamson <dros@netapp.com> wrote:
>>> 
>>>> I'm going to rethink this, as the server may return arbitrarily long bitmaps.
>>>> 
>>>> I'll probably just always allocate an extra page...
>>> 
>>> Perhaps we should do the same thing for NFSv4 that we do in nfs3_proc_getacl and just have xdr_partial_copy_from_skb allocate the pages for us?
>> 
>> This sounds good - the nice part about this is that we’ll be able to cache the ACL the first time we ask for it instead of having the first getacl fail due to not having enough buffer space (this is currently just used to get the ACL length for the next call).
>> 
>> So I have a patch almost ready, but I still have the same problem as before: what value should we use for the ‘len’ argument of xdr_inline_pages?
> 
> I’d suggest that we keep NFS4ACL_MAXPAGES as the array length, and then use NFS4ACL_MAXPAGES<<PAGE_SHIFT as the argument of xdr_inline_pages.

Ok, so that means we don’t really support NFS4ACL_MAXPAGES worth of ACL data… We support NFS4ACL_MAXPAGES minus the size of bitmaps (and ACL length).

> 
> I also suggest that we continue to preallocate the same number of pages that we do today. The only change to __nfs4_get_acl_uncached would be to NULL all entries of the pages[] array that are unused, so that we can later free all non-NULL entries on exit.

Well, ok… but it seems wrong only allocating room for the ACL data and not the bitmaps.  What makes sense to me is allocating enough room for the common bitmap size, plus ACL data, then letting the auto allocation take care of any extra.

> 
> The effect of those 2 is that we’ll be preallocating pages for the common case where the server is using 1,2 or 3 words for its bitmap, but the ‘automatic allocation’ can deal with the case where the server returns a larger bitmap.

Oh, so you don’t mean "continue to preallocate the same number of pages that we do today”? We currently don’t allocate any room for the bitmap - that’s the problem I’m trying to solve!



Actually, I think we should just get rid of the preallocation, because of how the getxattr interface is used: 
 - the application first calls getxattr() with buf=NULL, len=0 to get the length of the ACL buffer
 - the application then allocates a buffer big enough and calls getxattr() again to actually fetch the buffer.

The first call to getxattr() calls getacl with len=0 - in this case, we allocate 1 page and hope that the ACL data fits. If it does, we cache it, but if it doesn’t we throw it away (but return the length).

The second call to getxattr() calls getacl with the correct length (of the ACL data), so barring any issues with not having enough room to decode the bitmap, this time the ACL data will have enough room and be cached (and returned to application).

… So, if we leave the preallocation as it is, but allow the xdr layer to allocate more pages, the preallocation code will never be used besides the the npages=1 case.

I think we should either:

1) just let the xdr layer do all of the allocation.

2) always only allocate 1 page (we know there will always be *some* data to decode) and let the xdr layer do the rest of the allocation.

> 
>> nfs3_xdr_enc_getacl3args uses "NFSACL_MAXPAGES << PAGE_SHIFT”, which is fine because the inline pages only hold the ACL data, but for nfs4 we must have enough room for:
>> - a bitmap4 type
>> - the length of the ACL data
>> - the ACL data
>> 
>> The ACL length is known beforehand (we know the max is NFS4ACL_MAXPAGES << PAGE_SHIFT), but from the spec it looks like there can be 2^32-1 words in a bitmap4 since it’s defined as "typedef uint32_t bitmap4<>” (no maximum size specified in the <>). We certainly cannot allocate this many pages, so maybe can we agree on a reasonable number and add it as a #define?
>> 
>> I still don’t understand why we need to support more than three bitmaps here, it sounds like supporting a buggy server and that’s usually a no-no.  Also, nfs4xdr.h has #defines:
>> 
>> #define nfs4_fattr_bitmap_maxsz 4
>> ...
>> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
>>                                nfs4_fattr_bitmap_maxsz + 1)
>> 
>> so that looks like we only expect 3 bitmap words...
> 
> Yes, but if you examine the RFCs, you’ll see that there is no requirement that servers or clients ‘trim’ their bitmaps. A server that supports NFSv4.10 may, for instance, decide to return 8 words per bitmap, even when doing NFSv4.0.

Ok, that makes sense.  Maybe allowing allocation of up to (NFS4ACL_MAXPAGES + 1) pages of data makes sense….

-dros

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Myklebust, Trond]

On Nov 21, 2013, at 9:33, Weston Andros Adamson <dros@netapp.com> wrote:

> On Nov 16, 2013, at 3:12 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:
> 
>> 
>> On Nov 16, 2013, at 14:15, Weston Andros Adamson <dros@netapp.com> wrote:
>> 
>>> I'm going to rethink this, as the server may return arbitrarily long bitmaps.
>>> 
>>> I'll probably just always allocate an extra page...
>> 
>> Perhaps we should do the same thing for NFSv4 that we do in nfs3_proc_getacl and just have xdr_partial_copy_from_skb allocate the pages for us?
> 
> This sounds good - the nice part about this is that we’ll be able to cache the ACL the first time we ask for it instead of having the first getacl fail due to not having enough buffer space (this is currently just used to get the ACL length for the next call).
> 
> So I have a patch almost ready, but I still have the same problem as before: what value should we use for the ‘len’ argument of xdr_inline_pages?

I’d suggest that we keep NFS4ACL_MAXPAGES as the array length, and then use NFS4ACL_MAXPAGES<<PAGE_SHIFT as the argument of xdr_inline_pages.

I also suggest that we continue to preallocate the same number of pages that we do today. The only change to __nfs4_get_acl_uncached would be to NULL all entries of the pages[] array that are unused, so that we can later free all non-NULL entries on exit.

The effect of those 2 is that we’ll be preallocating pages for the common case where the server is using 1,2 or 3 words for its bitmap, but the ‘automatic allocation’ can deal with the case where the server returns a larger bitmap.

> nfs3_xdr_enc_getacl3args uses "NFSACL_MAXPAGES << PAGE_SHIFT”, which is fine because the inline pages only hold the ACL data, but for nfs4 we must have enough room for:
> - a bitmap4 type
> - the length of the ACL data
> - the ACL data
> 
> The ACL length is known beforehand (we know the max is NFS4ACL_MAXPAGES << PAGE_SHIFT), but from the spec it looks like there can be 2^32-1 words in a bitmap4 since it’s defined as "typedef uint32_t bitmap4<>” (no maximum size specified in the <>). We certainly cannot allocate this many pages, so maybe can we agree on a reasonable number and add it as a #define?
> 
> I still don’t understand why we need to support more than three bitmaps here, it sounds like supporting a buggy server and that’s usually a no-no.  Also, nfs4xdr.h has #defines:
> 
> #define nfs4_fattr_bitmap_maxsz 4
> ...
> #define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
>                                 nfs4_fattr_bitmap_maxsz + 1)
> 
> so that looks like we only expect 3 bitmap words...

Yes, but if you examine the RFCs, you’ll see that there is no requirement that servers or clients ‘trim’ their bitmaps. A server that supports NFSv4.10 may, for instance, decide to return 8 words per bitmap, even when doing NFSv4.0.

Cheers
  Trond


--
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

On Nov 16, 2013, at 3:12 PM, Myklebust, Trond <Trond.Myklebust@netapp.com> wrote:

> 
> On Nov 16, 2013, at 14:15, Weston Andros Adamson <dros@netapp.com> wrote:
> 
>> I'm going to rethink this, as the server may return arbitrarily long bitmaps.
>> 
>> I'll probably just always allocate an extra page...
> 
> Perhaps we should do the same thing for NFSv4 that we do in nfs3_proc_getacl and just have xdr_partial_copy_from_skb allocate the pages for us?

This sounds good - the nice part about this is that we’ll be able to cache the ACL the first time we ask for it instead of having the first getacl fail due to not having enough buffer space (this is currently just used to get the ACL length for the next call).

So I have a patch almost ready, but I still have the same problem as before: what value should we use for the ‘len’ argument of xdr_inline_pages?

nfs3_xdr_enc_getacl3args uses "NFSACL_MAXPAGES << PAGE_SHIFT”, which is fine because the inline pages only hold the ACL data, but for nfs4 we must have enough room for:
 - a bitmap4 type
 - the length of the ACL data
 - the ACL data

The ACL length is known beforehand (we know the max is NFS4ACL_MAXPAGES << PAGE_SHIFT), but from the spec it looks like there can be 2^32-1 words in a bitmap4 since it’s defined as "typedef uint32_t bitmap4<>” (no maximum size specified in the <>). We certainly cannot allocate this many pages, so maybe can we agree on a reasonable number and add it as a #define?

I still don’t understand why we need to support more than three bitmaps here, it sounds like supporting a buggy server and that’s usually a no-no.  Also, nfs4xdr.h has #defines:

#define nfs4_fattr_bitmap_maxsz 4
...
#define decode_getacl_maxsz     (op_decode_hdr_maxsz + \
                                 nfs4_fattr_bitmap_maxsz + 1)

so that looks like we only expect 3 bitmap words...

-dros

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Myklebust, Trond]

On Nov 16, 2013, at 14:15, Weston Andros Adamson <dros@netapp.com> wrote:

> I'm going to rethink this, as the server may return arbitrarily long bitmaps.
> 
> I'll probably just always allocate an extra page...

Perhaps we should do the same thing for NFSv4 that we do in nfs3_proc_getacl and just have xdr_partial_copy_from_skb allocate the pages for us?

Cheers
  Trond

Re: [PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

I'm going to rethink this, as the server may return arbitrarily long bitmaps.

I'll probably just always allocate an extra page...

-dros

> On Nov 15, 2013, at 10:02 AM, "Weston Andros Adamson" <dros@netapp.com> wrote:
> 
> 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 bug results in getxattr() returning 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 <dros@netapp.com>
> ---
> fs/nfs/nfs4proc.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 5ab33c0..006cba1 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -4453,7 +4453,12 @@ 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);
> +    /*
> +     * extra space needed for attr bitmap and length in getacl decoder.
> +     * 1 word for bitmap len, 3 words for bitmaps and 1 word for attr len.
> +     */
> +    unsigned int preamble_len = 20;
> +    unsigned int npages = DIV_ROUND_UP(preamble_len + buflen, PAGE_SIZE);
>    int ret = -ENOMEM, i;
> 
>    /* As long as we're doing a round trip to the server anyway,
> -- 
> 1.8.3.1 (Apple Git-46)
> 

[PATCH] NFSv4: fix getacl ERANGE for some ACL buffer sizes

[Weston Andros Adamson]

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 bug results in getxattr() returning 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 <dros@netapp.com>
---
 fs/nfs/nfs4proc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 5ab33c0..006cba1 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -4453,7 +4453,12 @@ 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);
+	/*
+	 * extra space needed for attr bitmap and length in getacl decoder.
+	 * 1 word for bitmap len, 3 words for bitmaps and 1 word for attr len.
+	 */
+	unsigned int preamble_len = 20;
+	unsigned int npages = DIV_ROUND_UP(preamble_len + buflen, PAGE_SIZE);
 	int ret = -ENOMEM, i;
 
 	/* As long as we're doing a round trip to the server anyway,
-- 
1.8.3.1 (Apple Git-46)