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 length of the ACL returned. The
flag ACL_LEN_REQUEST has been renamed to NFS4_ACL_LEN_ONLY and apart
from indicating that the user is just interested in the ACL length when
making a request, it is also used to indicate if the xdr pages buffer
returned in response holds the complete ACL or just the length.
Signed-off-by: Sachin Prabhu <[email protected]>
---
fs/nfs/nfs4proc.c | 18 +++++++++---------
fs/nfs/nfs4xdr.c | 4 +++-
include/linux/nfs_xdr.h | 2 +-
3 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15fc7e4..e19c322 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3726,16 +3726,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,
@@ -3777,7 +3777,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);
@@ -3787,7 +3787,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_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 8aadd90..39c8483 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.10.4
On Mon, 2012-07-30 at 21:54 +0000, Myklebust, Trond wrote:
> On Tue, 2012-07-24 at 12:36 +0100, Sachin Prabhu wrote:
> > 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 length of the ACL returned. The
> > flag ACL_LEN_REQUEST has been renamed to NFS4_ACL_LEN_ONLY and apart
> > from indicating that the user is just interested in the ACL length when
> > making a request, it is also used to indicate if the xdr pages buffer
> > returned in response holds the complete ACL or just the length.
> >
> > Signed-off-by: Sachin Prabhu <[email protected]>
>
> If this fix is going into stable, then the patch needs to be split up.
> The rename of ACL_LEN_REQUEST to NFS4_ACL_LEN_ONLY is a cleanup and
> should not be propagated to stable...
Trond the change there was required to fix a mistake in the if
condition.
static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf,
size_t buflen)
{
..
acl_len = res.acl_len - res.acl_data_offset;
if (acl_len > args.acl_len)
..
}
At this point,
res.acl_data_offset = size of bitmap array + size of length attribute
res.acl_length = res.acl_data_offset + ACL length
These 2 variables are set in decode_getacl(). The right check here would
have been to compare res.acl_len and args.acl_len.
Instead of simply replacing the if condition, I decided to clean up the
code to use NFS4_ACL_LEN_ONLY on advice of Bruce to make the code easier
to understand.
So both parts of this patch are actually bug fixes. If you still think
that they should be separated, I am happy to do that.
Sachin Prabhu
T24gVHVlLCAyMDEyLTA3LTMxIGF0IDIyOjA0ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K
PiBPbiBNb24sIDIwMTItMDctMzAgYXQgMjE6NTQgKzAwMDAsIE15a2xlYnVzdCwgVHJvbmQgd3Jv
dGU6DQo+ID4gT24gVHVlLCAyMDEyLTA3LTI0IGF0IDEyOjM2ICswMTAwLCBTYWNoaW4gUHJhYmh1
IHdyb3RlOg0KPiA+ID4gVGhpcyBmaXhlcyBhIGJ1ZyBpbnRyb2R1Y2VkIGJ5IGNvbW1pdA0KPiA+
ID4gNWEwMDY4OTkzMGFiOTc1ZmRkMWIzN2IwMzQ0NzUwMTdlNDYwY2YyYQ0KPiA+ID4gDQo+ID4g
PiBCcnVjZSBGaWVsZHMgcG9pbnRlZCBvdXQgdGhhdCB0aGUgY2hhbmdlcyBpbnRyb2R1Y2VkIGJ5
IHRoZSBwYXRjaCB3aWxsDQo+ID4gPiBjYXVzZSB0aGUgYXJyYXkgbnBhZ2VzIHRvIG92ZXJmbG93
IGlmIGEgc2l6ZSByZXF1ZXN0ZWQgaXMgPj0NCj4gPiA+IFhBVFRSX1NJWkVfTUFYLg0KPiA+ID4g
DQo+ID4gPiBUaGUgcGF0Y2ggYWxzbyBmaXhlcyB0aGUgY2hlY2sgZm9yIHRoZSBsZW5ndGggb2Yg
dGhlIEFDTCByZXR1cm5lZC4gVGhlDQo+ID4gPiBmbGFnIEFDTF9MRU5fUkVRVUVTVCBoYXMgYmVl
biByZW5hbWVkIHRvIE5GUzRfQUNMX0xFTl9PTkxZIGFuZCBhcGFydA0KPiA+ID4gZnJvbSBpbmRp
Y2F0aW5nIHRoYXQgdGhlIHVzZXIgaXMganVzdCBpbnRlcmVzdGVkIGluIHRoZSBBQ0wgbGVuZ3Ro
IHdoZW4NCj4gPiA+IG1ha2luZyBhIHJlcXVlc3QsIGl0IGlzIGFsc28gdXNlZCB0byBpbmRpY2F0
ZSBpZiB0aGUgeGRyIHBhZ2VzIGJ1ZmZlcg0KPiA+ID4gcmV0dXJuZWQgaW4gcmVzcG9uc2UgaG9s
ZHMgdGhlIGNvbXBsZXRlIEFDTCBvciBqdXN0IHRoZSBsZW5ndGguDQo+ID4gPiANCj4gPiA+IFNp
Z25lZC1vZmYtYnk6IFNhY2hpbiBQcmFiaHUgPHNwcmFiaHVAcmVkaGF0LmNvbT4NCj4gPiANCj4g
PiBJZiB0aGlzIGZpeCBpcyBnb2luZyBpbnRvIHN0YWJsZSwgdGhlbiB0aGUgcGF0Y2ggbmVlZHMg
dG8gYmUgc3BsaXQgdXAuDQo+ID4gVGhlIHJlbmFtZSBvZiBBQ0xfTEVOX1JFUVVFU1QgdG8gTkZT
NF9BQ0xfTEVOX09OTFkgaXMgYSBjbGVhbnVwIGFuZA0KPiA+IHNob3VsZCBub3QgYmUgcHJvcGFn
YXRlZCB0byBzdGFibGUuLi4NCj4gDQo+IFRyb25kIHRoZSBjaGFuZ2UgdGhlcmUgd2FzIHJlcXVp
cmVkIHRvIGZpeCBhIG1pc3Rha2UgaW4gdGhlIGlmDQo+IGNvbmRpdGlvbi4NCj4gDQo+IHN0YXRp
YyBzc2l6ZV90IF9fbmZzNF9nZXRfYWNsX3VuY2FjaGVkKHN0cnVjdCBpbm9kZSAqaW5vZGUsIHZv
aWQgKmJ1ZiwNCj4gc2l6ZV90IGJ1ZmxlbikNCj4gew0KPiAuLg0KPiAgICAgICAgIGFjbF9sZW4g
PSByZXMuYWNsX2xlbiAtIHJlcy5hY2xfZGF0YV9vZmZzZXQ7DQo+ICAgICAgICAgaWYgKGFjbF9s
ZW4gPiBhcmdzLmFjbF9sZW4pDQo+IC4uDQo+IH0NCj4gDQo+IEF0IHRoaXMgcG9pbnQsIA0KPiBy
ZXMuYWNsX2RhdGFfb2Zmc2V0ID0gc2l6ZSBvZiBiaXRtYXAgYXJyYXkgKyBzaXplIG9mIGxlbmd0
aCBhdHRyaWJ1dGUNCj4gcmVzLmFjbF9sZW5ndGggPSByZXMuYWNsX2RhdGFfb2Zmc2V0ICsgQUNM
IGxlbmd0aCANCj4gVGhlc2UgMiB2YXJpYWJsZXMgYXJlIHNldCBpbiBkZWNvZGVfZ2V0YWNsKCku
IFRoZSByaWdodCBjaGVjayBoZXJlIHdvdWxkDQo+IGhhdmUgYmVlbiB0byBjb21wYXJlIHJlcy5h
Y2xfbGVuIGFuZCBhcmdzLmFjbF9sZW4uIA0KPiANCj4gSW5zdGVhZCBvZiBzaW1wbHkgcmVwbGFj
aW5nIHRoZSBpZiBjb25kaXRpb24sIEkgZGVjaWRlZCB0byBjbGVhbiB1cCB0aGUNCj4gY29kZSB0
byB1c2UgTkZTNF9BQ0xfTEVOX09OTFkgb24gYWR2aWNlIG9mIEJydWNlIHRvIG1ha2UgdGhlIGNv
ZGUgZWFzaWVyDQo+IHRvIHVuZGVyc3RhbmQuDQo+IA0KPiBTbyBib3RoIHBhcnRzIG9mIHRoaXMg
cGF0Y2ggYXJlIGFjdHVhbGx5IGJ1ZyBmaXhlcy4gSWYgeW91IHN0aWxsIHRoaW5rDQo+IHRoYXQg
dGhleSBzaG91bGQgYmUgc2VwYXJhdGVkLCBJIGFtIGhhcHB5IHRvIGRvIHRoYXQuDQoNCkkgYWNj
ZXB0IHRoYXQgd2UgbmVlZCBib3RoIGZpeGVzLiBUaGUgcGFydCB0aGF0IEkgd2FudCB0byBzZXBh
cmF0ZSBvdXQNCmlzIF9vbmx5XyB0aGUgcmVuYW1lIG9mIEFDTF9MRU5fUkVRVUVTVCwgc2luY2Ug
dGhhdCBpcyBub3QgYSBmaXguDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xp
ZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3
Lm5ldGFwcC5jb20NCg0K
On Tue, Jul 24, 2012 at 12:36:25PM +0100, Sachin Prabhu wrote:
> 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 length of the ACL returned. The
> flag ACL_LEN_REQUEST has been renamed to NFS4_ACL_LEN_ONLY and apart
> from indicating that the user is just interested in the ACL length when
> making a request, it is also used to indicate if the xdr pages buffer
> returned in response holds the complete ACL or just the length.
Looks right to me.
--b.
>
> Signed-off-by: Sachin Prabhu <[email protected]>
> ---
> fs/nfs/nfs4proc.c | 18 +++++++++---------
> fs/nfs/nfs4xdr.c | 4 +++-
> include/linux/nfs_xdr.h | 2 +-
> 3 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 15fc7e4..e19c322 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3726,16 +3726,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,
> @@ -3777,7 +3777,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);
> @@ -3787,7 +3787,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_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 8aadd90..39c8483 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.10.4
>
> --
> 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
T24gVHVlLCAyMDEyLTA3LTI0IGF0IDEyOjM2ICswMTAwLCBTYWNoaW4gUHJhYmh1IHdyb3RlOg0K
PiBUaGlzIGZpeGVzIGEgYnVnIGludHJvZHVjZWQgYnkgY29tbWl0DQo+IDVhMDA2ODk5MzBhYjk3
NWZkZDFiMzdiMDM0NDc1MDE3ZTQ2MGNmMmENCj4gDQo+IEJydWNlIEZpZWxkcyBwb2ludGVkIG91
dCB0aGF0IHRoZSBjaGFuZ2VzIGludHJvZHVjZWQgYnkgdGhlIHBhdGNoIHdpbGwNCj4gY2F1c2Ug
dGhlIGFycmF5IG5wYWdlcyB0byBvdmVyZmxvdyBpZiBhIHNpemUgcmVxdWVzdGVkIGlzID49DQo+
IFhBVFRSX1NJWkVfTUFYLg0KPiANCj4gVGhlIHBhdGNoIGFsc28gZml4ZXMgdGhlIGNoZWNrIGZv
ciB0aGUgbGVuZ3RoIG9mIHRoZSBBQ0wgcmV0dXJuZWQuIFRoZQ0KPiBmbGFnIEFDTF9MRU5fUkVR
VUVTVCBoYXMgYmVlbiByZW5hbWVkIHRvIE5GUzRfQUNMX0xFTl9PTkxZIGFuZCBhcGFydA0KPiBm
cm9tIGluZGljYXRpbmcgdGhhdCB0aGUgdXNlciBpcyBqdXN0IGludGVyZXN0ZWQgaW4gdGhlIEFD
TCBsZW5ndGggd2hlbg0KPiBtYWtpbmcgYSByZXF1ZXN0LCBpdCBpcyBhbHNvIHVzZWQgdG8gaW5k
aWNhdGUgaWYgdGhlIHhkciBwYWdlcyBidWZmZXINCj4gcmV0dXJuZWQgaW4gcmVzcG9uc2UgaG9s
ZHMgdGhlIGNvbXBsZXRlIEFDTCBvciBqdXN0IHRoZSBsZW5ndGguDQo+IA0KPiBTaWduZWQtb2Zm
LWJ5OiBTYWNoaW4gUHJhYmh1IDxzcHJhYmh1QHJlZGhhdC5jb20+DQoNCklmIHRoaXMgZml4IGlz
IGdvaW5nIGludG8gc3RhYmxlLCB0aGVuIHRoZSBwYXRjaCBuZWVkcyB0byBiZSBzcGxpdCB1cC4N
ClRoZSByZW5hbWUgb2YgQUNMX0xFTl9SRVFVRVNUIHRvIE5GUzRfQUNMX0xFTl9PTkxZIGlzIGEg
Y2xlYW51cCBhbmQNCnNob3VsZCBub3QgYmUgcHJvcGFnYXRlZCB0byBzdGFibGUuLi4NCg0KQ2hl
ZXJzDQogIFRyb25kDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWlu
dGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAu
Y29tDQoNCg==