[PATCH 0/2] Fix bugs and clean code in __nfs4_get_acl_uncached

[PATCH 0/2] Fix bugs and clean code in __nfs4_get_acl_uncached

[Sachin Prabhu]

Split the bug fixes and code cleanup into separate patches asi
requested in
http://www.spinics.net/lists/linux-nfs/msg31677.html

Fix bugs in function __nfs4_get_acl_uncached and clean up code to make
it easier to understand.

Sachin Prabhu (2):
  Fix array overflow in __nfs4_get_acl_uncached
  Simplify check for size of ACL returned in __nfs4_get_acl_uncached

 fs/nfs/nfs4proc.c       | 18 +++++++++---------
 fs/nfs/nfs4xdr.c        |  4 +++-
 include/linux/nfs_xdr.h |  2 +-
 3 files changed, 13 insertions(+), 11 deletions(-)

-- 
1.7.11.2

[PATCH 1/2] Fix array overflow in __nfs4_get_acl_uncached

[Sachin Prabhu]

This fixes a bug introduced by commit
5a00689930ab975fdd1b37b034475017e460cf2a

Bruce Fields pointed out that the changes introduced by the patch will
cause the array npages to overflow if a size requested is >=
XATTR_SIZE_MAX.

The patch also fixes the check for the size of acl returned in the same
function and updates the comment associated with this function.

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
---
 fs/nfs/nfs4proc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index c157b20..be03b95 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3709,16 +3709,16 @@ 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.  On a NULL buf, we allocate 2 pages guessing that the
+ * ACL request can be serviced by those pages. If so, we cache the ACL data,
+ * and the 2nd call to getxattr is serviced by the cache. If not so, we throw
+ * away the pages, 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.
  */
 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,
@@ -3770,7 +3770,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		goto out_free;
 
 	acl_len = res.acl_len - res.acl_data_offset;
-	if (acl_len > args.acl_len)
+	if (res.acl_len > args.acl_len)
 		nfs4_write_cached_acl(inode, NULL, 0, acl_len);
 	else
 		nfs4_write_cached_acl(inode, pages, res.acl_data_offset,
-- 
1.7.11.2

[PATCH 2/2] Simplify check for size of ACL returned in __nfs4_get_acl_uncached

[Sachin Prabhu]

Rename flag ACL_LEN_REQEST to NFS4_ACL_LEN_ONLY. Apart from using this
flag during a request to indicate that the user is only interested in
the length of the ACL, we now also use it in the response stage to
indicate that the data returned holds the complete ACL or just the
length.

Signed-off-by: Sachin Prabhu <sprabhu@redhat.com>
---
 fs/nfs/nfs4proc.c       | 4 ++--
 fs/nfs/nfs4xdr.c        | 4 +++-
 include/linux/nfs_xdr.h | 2 +-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index be03b95..aaa98f0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3760,7 +3760,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 	/* Let decode_getfacl know not to fail if the ACL data is larger than
 	 * the page we send as a guess */
 	if (buf == NULL)
-		res.acl_flags |= NFS4_ACL_LEN_REQUEST;
+		res.acl_flags |= NFS4_ACL_LEN_ONLY;
 
 	dprintk("%s  buf %p buflen %zu npages %d args.acl_len %zu\n",
 		__func__, buf, buflen, npages, args.acl_len);
@@ -3770,7 +3770,7 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		goto out_free;
 
 	acl_len = res.acl_len - res.acl_data_offset;
-	if (res.acl_len > args.acl_len)
+	if (res.acl_flags & NFS4_ACL_LEN_ONLY)
 		nfs4_write_cached_acl(inode, NULL, 0, acl_len);
 	else
 		nfs4_write_cached_acl(inode, pages, res.acl_data_offset,
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 18fae29..a88fcd0 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5101,7 +5101,7 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 		hdrlen = (u8 *)xdr->p - (u8 *)iov->iov_base;
 		attrlen += res->acl_data_offset;
 		if (attrlen > page_len) {
-			if (res->acl_flags & NFS4_ACL_LEN_REQUEST) {
+			if (res->acl_flags & NFS4_ACL_LEN_ONLY) {
 				/* getxattr interface called with a NULL buf */
 				res->acl_len = attrlen;
 				goto out;
@@ -5110,6 +5110,8 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
 					attrlen, page_len);
 			return -EINVAL;
 		}
+		/* At this stage we have the complete ACL */
+		res->acl_flags &= ~NFS4_ACL_LEN_ONLY;
 		xdr_read_pages(xdr, attrlen);
 		res->acl_len = attrlen;
 	} else
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index d3b7c18..c2d77d5 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -648,7 +648,7 @@ struct nfs_getaclargs {
 };
 
 /* getxattr ACL interface flags */
-#define NFS4_ACL_LEN_REQUEST	0x0001	/* zero length getxattr buffer */
+#define NFS4_ACL_LEN_ONLY	0x0001	/* zero length getxattr buffer */
 struct nfs_getaclres {
 	size_t				acl_len;
 	size_t				acl_data_offset;
-- 
1.7.11.2

Re: [PATCH 2/2] Simplify check for size of ACL returned in __nfs4_get_acl_uncached

[Myklebust, Trond]

T24gV2VkLCAyMDEyLTA4LTAxIGF0IDE0OjI5ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K
PiBSZW5hbWUgZmxhZyBBQ0xfTEVOX1JFUUVTVCB0byBORlM0X0FDTF9MRU5fT05MWS4gQXBhcnQg
ZnJvbSB1c2luZyB0aGlzDQo+IGZsYWcgZHVyaW5nIGEgcmVxdWVzdCB0byBpbmRpY2F0ZSB0aGF0
IHRoZSB1c2VyIGlzIG9ubHkgaW50ZXJlc3RlZCBpbg0KPiB0aGUgbGVuZ3RoIG9mIHRoZSBBQ0ws
IHdlIG5vdyBhbHNvIHVzZSBpdCBpbiB0aGUgcmVzcG9uc2Ugc3RhZ2UgdG8NCj4gaW5kaWNhdGUg
dGhhdCB0aGUgZGF0YSByZXR1cm5lZCBob2xkcyB0aGUgY29tcGxldGUgQUNMIG9yIGp1c3QgdGhl
DQo+IGxlbmd0aC4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IFNhY2hpbiBQcmFiaHUgPHNwcmFiaHVA
cmVkaGF0LmNvbT4NCj4gLS0tDQo+ICBmcy9uZnMvbmZzNHByb2MuYyAgICAgICB8IDQgKystLQ0K
PiAgZnMvbmZzL25mczR4ZHIuYyAgICAgICAgfCA0ICsrKy0NCj4gIGluY2x1ZGUvbGludXgvbmZz
X3hkci5oIHwgMiArLQ0KPiAgMyBmaWxlcyBjaGFuZ2VkLCA2IGluc2VydGlvbnMoKyksIDQgZGVs
ZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRwcm9jLmMgYi9mcy9uZnMv
bmZzNHByb2MuYw0KPiBpbmRleCBiZTAzYjk1Li5hYWE5OGYwIDEwMDY0NA0KPiAtLS0gYS9mcy9u
ZnMvbmZzNHByb2MuYw0KPiArKysgYi9mcy9uZnMvbmZzNHByb2MuYw0KPiBAQCAtMzc2MCw3ICsz
NzYwLDcgQEAgc3RhdGljIHNzaXplX3QgX19uZnM0X2dldF9hY2xfdW5jYWNoZWQoc3RydWN0IGlu
b2RlICppbm9kZSwgdm9pZCAqYnVmLCBzaXplX3QgYnUNCj4gIAkvKiBMZXQgZGVjb2RlX2dldGZh
Y2wga25vdyBub3QgdG8gZmFpbCBpZiB0aGUgQUNMIGRhdGEgaXMgbGFyZ2VyIHRoYW4NCj4gIAkg
KiB0aGUgcGFnZSB3ZSBzZW5kIGFzIGEgZ3Vlc3MgKi8NCj4gIAlpZiAoYnVmID09IE5VTEwpDQo+
IC0JCXJlcy5hY2xfZmxhZ3MgfD0gTkZTNF9BQ0xfTEVOX1JFUVVFU1Q7DQo+ICsJCXJlcy5hY2xf
ZmxhZ3MgfD0gTkZTNF9BQ0xfTEVOX09OTFk7DQo+ICANCj4gIAlkcHJpbnRrKCIlcyAgYnVmICVw
IGJ1ZmxlbiAlenUgbnBhZ2VzICVkIGFyZ3MuYWNsX2xlbiAlenVcbiIsDQo+ICAJCV9fZnVuY19f
LCBidWYsIGJ1ZmxlbiwgbnBhZ2VzLCBhcmdzLmFjbF9sZW4pOw0KPiBAQCAtMzc3MCw3ICszNzcw
LDcgQEAgc3RhdGljIHNzaXplX3QgX19uZnM0X2dldF9hY2xfdW5jYWNoZWQoc3RydWN0IGlub2Rl
ICppbm9kZSwgdm9pZCAqYnVmLCBzaXplX3QgYnUNCj4gIAkJZ290byBvdXRfZnJlZTsNCj4gIA0K
PiAgCWFjbF9sZW4gPSByZXMuYWNsX2xlbiAtIHJlcy5hY2xfZGF0YV9vZmZzZXQ7DQo+IC0JaWYg
KHJlcy5hY2xfbGVuID4gYXJncy5hY2xfbGVuKQ0KPiArCWlmIChyZXMuYWNsX2ZsYWdzICYgTkZT
NF9BQ0xfTEVOX09OTFkpDQoNCklzIHRoaXMgYSBidWdmaXggb3IgYSBjbGVhbnVwPyBJZiB0aGUg
Zm9ybWVyLCB0aGVuIGl0IGJlbG9uZ3MgaW4gdGhlDQpmaXJzdCBwYXRjaC4NCg0KPiAgCQluZnM0
X3dyaXRlX2NhY2hlZF9hY2woaW5vZGUsIE5VTEwsIDAsIGFjbF9sZW4pOw0KPiAgCWVsc2UNCj4g
IAkJbmZzNF93cml0ZV9jYWNoZWRfYWNsKGlub2RlLCBwYWdlcywgcmVzLmFjbF9kYXRhX29mZnNl
dCwNCg0KTm93IGlmIE5GUzRfQUNMX0xFTl9PTkxZIGlzIHNldCwgd2UgYXBwZWFyIHRvIHVuY29u
ZGl0aW9uYWxseSB3cml0ZSB0aGUNCmFjbCwgZXZlbiBpZiB0aGUgYnVmZmVyIG92ZXJmbG93cy4u
Lg0KDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHhkci5jIGIvZnMvbmZzL25mczR4ZHIuYw0K
PiBpbmRleCAxOGZhZTI5Li5hODhmY2QwIDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvbmZzNHhkci5j
DQo+ICsrKyBiL2ZzL25mcy9uZnM0eGRyLmMNCj4gQEAgLTUxMDEsNyArNTEwMSw3IEBAIHN0YXRp
YyBpbnQgZGVjb2RlX2dldGFjbChzdHJ1Y3QgeGRyX3N0cmVhbSAqeGRyLCBzdHJ1Y3QgcnBjX3Jx
c3QgKnJlcSwNCj4gIAkJaGRybGVuID0gKHU4ICopeGRyLT5wIC0gKHU4ICopaW92LT5pb3ZfYmFz
ZTsNCj4gIAkJYXR0cmxlbiArPSByZXMtPmFjbF9kYXRhX29mZnNldDsNCj4gIAkJaWYgKGF0dHJs
ZW4gPiBwYWdlX2xlbikgew0KPiAtCQkJaWYgKHJlcy0+YWNsX2ZsYWdzICYgTkZTNF9BQ0xfTEVO
X1JFUVVFU1QpIHsNCj4gKwkJCWlmIChyZXMtPmFjbF9mbGFncyAmIE5GUzRfQUNMX0xFTl9PTkxZ
KSB7DQoNClRoZSBsb2dpYyBoZXJlIHN0aWxsIGxvb2tzIGluY29ycmVjdC4gSWYgdGhlIHVzZXIg
aXMgb25seSByZXF1ZXN0aW5nIHRoZQ0KYWNsIGxlbmd0aCwgdGhlbiB3ZSBzaG91bGRuJ3QgY2Fy
ZSBhYm91dCB0aGUgJ2F0dHJsZW4gPiBwYWdlX2xlbicgdGVzdC4NCg0KVGhlcmUgYWxzbyBhcHBl
YXJzIHRvIHJlbWFpbiBhIGxvdCBvZiBpbGxlZ2FsIGRlZmVyZW5jaW5nIG9mIHhkci0+cCBldmVu
DQphZnRlciB0aGlzIHBhdGNoLiBXZSBzaG91bGQgYmUgYWJsZSB0byBmaXggdGhhdCBieSByZXBs
YWNpbmcNCnhkcl9yZWFkX3BhZ2VzIHdpdGggYSBjYWxsIHRvIHhkcl9lbnRlcl9wYWdlKCkgYmVm
b3JlIHdlIGNhbGwNCmRlY29kZV9hdHRyX2JpdG1hcCgpLi4uDQoNCj4gIAkJCQkvKiBnZXR4YXR0
ciBpbnRlcmZhY2UgY2FsbGVkIHdpdGggYSBOVUxMIGJ1ZiAqLw0KPiAgCQkJCXJlcy0+YWNsX2xl
biA9IGF0dHJsZW47DQo+ICAJCQkJZ290byBvdXQ7DQo+IEBAIC01MTEwLDYgKzUxMTAsOCBAQCBz
dGF0aWMgaW50IGRlY29kZV9nZXRhY2woc3RydWN0IHhkcl9zdHJlYW0gKnhkciwgc3RydWN0IHJw
Y19ycXN0ICpyZXEsDQo+ICAJCQkJCWF0dHJsZW4sIHBhZ2VfbGVuKTsNCj4gIAkJCXJldHVybiAt
RUlOVkFMOw0KPiAgCQl9DQo+ICsJCS8qIEF0IHRoaXMgc3RhZ2Ugd2UgaGF2ZSB0aGUgY29tcGxl
dGUgQUNMICovDQo+ICsJCXJlcy0+YWNsX2ZsYWdzICY9IH5ORlM0X0FDTF9MRU5fT05MWTsNCj4g
IAkJeGRyX3JlYWRfcGFnZXMoeGRyLCBhdHRybGVuKTsNCg0KV2Ugc2hvdWxkIGp1c3QgcmVwbGFj
ZSB0aGF0IGNoZWNrIG9mICdhdHRybGVuID4gcGFnZV9sZW4nIHdpdGggYSBjaGVjaw0Kb24gdGhl
IHJldHVybiB2YWx1ZSBvZiB4ZHJfcmVhZF9wYWdlcygpLg0KDQo+ICAJCXJlcy0+YWNsX2xlbiA9
IGF0dHJsZW47DQo+ICAJfSBlbHNlDQo+IGRpZmYgLS1naXQgYS9pbmNsdWRlL2xpbnV4L25mc194
ZHIuaCBiL2luY2x1ZGUvbGludXgvbmZzX3hkci5oDQo+IGluZGV4IGQzYjdjMTguLmMyZDc3ZDUg
MTAwNjQ0DQo+IC0tLSBhL2luY2x1ZGUvbGludXgvbmZzX3hkci5oDQo+ICsrKyBiL2luY2x1ZGUv
bGludXgvbmZzX3hkci5oDQo+IEBAIC02NDgsNyArNjQ4LDcgQEAgc3RydWN0IG5mc19nZXRhY2xh
cmdzIHsNCj4gIH07DQo+ICANCj4gIC8qIGdldHhhdHRyIEFDTCBpbnRlcmZhY2UgZmxhZ3MgKi8N
Cj4gLSNkZWZpbmUgTkZTNF9BQ0xfTEVOX1JFUVVFU1QJMHgwMDAxCS8qIHplcm8gbGVuZ3RoIGdl
dHhhdHRyIGJ1ZmZlciAqLw0KPiArI2RlZmluZSBORlM0X0FDTF9MRU5fT05MWQkweDAwMDEJLyog
emVybyBsZW5ndGggZ2V0eGF0dHIgYnVmZmVyICovDQo+ICBzdHJ1Y3QgbmZzX2dldGFjbHJlcyB7
DQo+ICAJc2l6ZV90CQkJCWFjbF9sZW47DQo+ICAJc2l6ZV90CQkJCWFjbF9kYXRhX29mZnNldDsN
Cg0K

Re: [PATCH 2/2] Simplify check for size of ACL returned in __nfs4_get_acl_uncached

[Sachin Prabhu]

On Wed, 2012-08-01 at 17:56 +0000, Myklebust, Trond wrote:
>  
> >  	acl_len = res.acl_len - res.acl_data_offset;
> > -	if (res.acl_len > args.acl_len)
> > +	if (res.acl_flags & NFS4_ACL_LEN_ONLY)
> 
> Is this a bugfix or a cleanup? If the former, then it belongs in the
> first patch.

The earlier patch fixes this bug with the the if condition. This patch
introduces the NFS4_ACL_LEN_ONLY flag which is set when the buffer we
have set aside for the ACLs was not enough for the ACLs returned by the
server.

> 
> >  		nfs4_write_cached_acl(inode, NULL, 0, acl_len);
> >  	else
> >  		nfs4_write_cached_acl(inode, pages, res.acl_data_offset,
> 
> Now if NFS4_ACL_LEN_ONLY is set, we appear to unconditionally write the
> acl, even if the buffer overflows...

If NFS4_ACL_LEN_ONLY is set, we call nfs4_write_cached_acl with the
second argument set to NULL. This results in only the acl length being
cached.


> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > index 18fae29..a88fcd0 100644
> > --- a/fs/nfs/nfs4xdr.c
> > +++ b/fs/nfs/nfs4xdr.c
> > @@ -5101,7 +5101,7 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
> >  		hdrlen = (u8 *)xdr->p - (u8 *)iov->iov_base;
> >  		attrlen += res->acl_data_offset;
> >  		if (attrlen > page_len) {
> > -			if (res->acl_flags & NFS4_ACL_LEN_REQUEST) {
> > +			if (res->acl_flags & NFS4_ACL_LEN_ONLY) {
> 
> The logic here still looks incorrect. If the user is only requesting the
> acl length, then we shouldn't care about the 'attrlen > page_len' test.

This is a result of a feature introduced with an earlier
patch(bf118a342f10dafe44b14451a1392c3254629a1f) and the behaviour is
described in the comment above __nfs4_get_acl_uncached().

***
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.
***

Check to see if the ACL attribute length returned by the server is
larger than the buffer allocated for it.
if (attrlen > page_len) {
	If the length returned is smaller than buffer allocated. 
	Check flags to see if the user had only requested for length.
	if (res->acl_flags & NFS4_ACL_LEN_ONLY) {
		/* getxattr interface called with a NULL buf */
		If yes, just save length and return
		res->acl_len = attrlen;
		goto out;
	}
	Else return -EINVAL.
	dprintk("NFS: acl reply: attrlen %u > page_len %zu\n",
		attrlen, page_len);
	return -EINVAL;
}
At the end of this if condition, we have determined that the buffer
length we allocated is large enough for the length of ACL returned.
So clear NFS4_ACL_LEN_ONLY if set.
/* At this stage we have the complete ACL */
res->acl_flags &= ~NFS4_ACL_LEN_ONLY;


> There also appears to remain a lot of illegal deferencing of xdr->p even
> after this patch. We should be able to fix that by replacing
> xdr_read_pages with a call to xdr_enter_page() before we call
> decode_attr_bitmap()...


Sachin Prabhu