diff mbox

[2/6] NFSv4: fix getacl ERANGE for some ACL buffer sizes

Message ID 1487470070-32358-3-git-send-email-bfields@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bruce Fields Feb. 19, 2017, 2:07 a.m. UTC 
From: Weston Andros Adamson <dros@netapp.com>

We're not taking into account that the space needed for the (variable
length) attr bitmap, with the result that we'd sometimes get a spurious
ERANGE when the ACL data got close to the end of a page.

Just add in an extra page to make sure.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfs/nfs4proc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Weston Andros Adamson Feb. 21, 2017, 7:46 p.m. UTC | #1 
> On Feb 18, 2017, at 9:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> 
> From: Weston Andros Adamson <dros@netapp.com>
> 
> We're not taking into account that the space needed for the (variable
> length) attr bitmap, with the result that we'd sometimes get a spurious
> ERANGE when the ACL data got close to the end of a page.
> 
> Just add in an extra page to make sure.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Thanks, you can add:

Signed-off-by: Weston Andros Adamson <dros@primarydata.com>


> ---
> fs/nfs/nfs4proc.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 0a0eaecf9676..60501e10625d 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -5069,7 +5069,7 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
>  */
> static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
> {
> -	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
> +	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
> 	struct nfs_getaclargs args = {
> 		.fh = NFS_FH(inode),
> 		.acl_pages = pages,
> @@ -5083,13 +5083,9 @@ 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);
> +	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE) + 1;
> 	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;
> 
> -- 
> 2.9.3
> 
> --
> 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

--
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
Bruce Fields Feb. 22, 2017, 10:36 p.m. UTC | #2 
On Tue, Feb 21, 2017 at 07:46:58PM +0000, Weston Andros Adamson wrote:
> 
> > On Feb 18, 2017, at 9:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> > 
> > From: Weston Andros Adamson <dros@netapp.com>
> > 
> > We're not taking into account that the space needed for the (variable
> > length) attr bitmap, with the result that we'd sometimes get a spurious
> > ERANGE when the ACL data got close to the end of a page.
> > 
> > Just add in an extra page to make sure.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> Thanks, you can add:
> 
> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>

Thanks.

Anna, could we get this one in now?

The rest of it still needs some work to account for the problem Andreas
notes (where we can return a length successfully even though we'll never
accept an ACL of that length).  But this one is an easy fix for a real
bug.

Let me know if you need it resent.

--b.
--
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
Anna Schumaker Feb. 23, 2017, 2:55 p.m. UTC | #3 
On 02/22/2017 05:36 PM, J. Bruce Fields wrote:
> On Tue, Feb 21, 2017 at 07:46:58PM +0000, Weston Andros Adamson wrote:
>>
>>> On Feb 18, 2017, at 9:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
>>>
>>> From: Weston Andros Adamson <dros@netapp.com>
>>>
>>> We're not taking into account that the space needed for the (variable
>>> length) attr bitmap, with the result that we'd sometimes get a spurious
>>> ERANGE when the ACL data got close to the end of a page.
>>>
>>> Just add in an extra page to make sure.
>>>
>>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
>>
>> Thanks, you can add:
>>
>> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> 
> Thanks.
> 
> Anna, could we get this one in now?
> 
> The rest of it still needs some work to account for the problem Andreas
> notes (where we can return a length successfully even though we'll never
> accept an ACL of that length).  But this one is an easy fix for a real
> bug.
> 
> Let me know if you need it resent.

I don't mind taking just the one patch.  Have you made any changes to it since this posting?  If not, then I can take it from the email.

Thanks,
Anna

> 
> --b.
> 
--
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
Bruce Fields Feb. 23, 2017, 7:43 p.m. UTC | #4 
On Thu, Feb 23, 2017 at 09:55:35AM -0500, Anna Schumaker wrote:
> 
> 
> On 02/22/2017 05:36 PM, J. Bruce Fields wrote:
> > On Tue, Feb 21, 2017 at 07:46:58PM +0000, Weston Andros Adamson wrote:
> >>
> >>> On Feb 18, 2017, at 9:07 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> >>>
> >>> From: Weston Andros Adamson <dros@netapp.com>
> >>>
> >>> We're not taking into account that the space needed for the (variable
> >>> length) attr bitmap, with the result that we'd sometimes get a spurious
> >>> ERANGE when the ACL data got close to the end of a page.
> >>>
> >>> Just add in an extra page to make sure.
> >>>
> >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> >>
> >> Thanks, you can add:
> >>
> >> Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> > 
> > Thanks.
> > 
> > Anna, could we get this one in now?
> > 
> > The rest of it still needs some work to account for the problem Andreas
> > notes (where we can return a length successfully even though we'll never
> > accept an ACL of that length).  But this one is an easy fix for a real
> > bug.
> > 
> > Let me know if you need it resent.
> 
> I don't mind taking just the one patch.  Have you made any changes to it since this posting?  If not, then I can take it from the email.

Just Dros's signoff.  Actually, we should probably take the first two.
I'll resend them just to make sure.

--b.
--
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
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0a0eaecf9676..60501e10625d 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5069,7 +5069,7 @@  static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
  */
 static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t buflen)
 {
-	struct page *pages[NFS4ACL_MAXPAGES] = {NULL, };
+	struct page *pages[NFS4ACL_MAXPAGES + 1] = {NULL, };
 	struct nfs_getaclargs args = {
 		.fh = NFS_FH(inode),
 		.acl_pages = pages,
@@ -5083,13 +5083,9 @@  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);
+	unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE) + 1;
 	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;