[PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[Trond Myklebust]

Technically, the Linux client is allowed by the NFSv4 spec to send
3 word bitmaps as part of an OPEN request. However, this causes the
current FreeBSD server to return NFS4ERR_ATTRNOTSUPP errors.

Fix the regression by making the Linux client use a 2 word bitmap unless
doing NFSv4.2 with labeled NFS.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4xdr.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 0abfb846..c74d616 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -999,6 +999,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 	__be32 *p;
 	__be32 *q;
 	int len;
+	uint32_t bmval_len = 2;
 	uint32_t bmval0 = 0;
 	uint32_t bmval1 = 0;
 	uint32_t bmval2 = 0;
@@ -1010,7 +1011,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 	 * = 40 bytes, plus any contribution from variable-length fields
 	 *            such as owner/group.
 	 */
-	len = 20;
+	len = 8;
 
 	/* Sigh */
 	if (iap->ia_valid & ATTR_SIZE)
@@ -1040,8 +1041,6 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 		}
 		len += 4 + (XDR_QUADLEN(owner_grouplen) << 2);
 	}
-	if (label)
-		len += 4 + 4 + 4 + (XDR_QUADLEN(label->len) << 2);
 	if (iap->ia_valid & ATTR_ATIME_SET)
 		len += 16;
 	else if (iap->ia_valid & ATTR_ATIME)
@@ -1050,15 +1049,22 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 		len += 16;
 	else if (iap->ia_valid & ATTR_MTIME)
 		len += 4;
+	if (label) {
+		len += 4 + 4 + 4 + (XDR_QUADLEN(label->len) << 2);
+		bmval_len = 3;
+	}
+
+	len += bmval_len << 2;
 	p = reserve_space(xdr, len);
 
 	/*
 	 * We write the bitmap length now, but leave the bitmap and the attribute
 	 * buffer length to be backfilled at the end of this routine.
 	 */
-	*p++ = cpu_to_be32(3);
+	*p++ = cpu_to_be32(bmval_len);
 	q = p;
-	p += 4;
+	/* Skip bitmap entries + attrlen */
+	p += bmval_len + 1;
 
 	if (iap->ia_valid & ATTR_SIZE) {
 		bmval0 |= FATTR4_WORD0_SIZE;
@@ -1112,10 +1118,11 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 				len, ((char *)p - (char *)q) + 4);
 		BUG();
 	}
-	len = (char *)p - (char *)q - 16;
+	len = (char *)p - (char *)q - (bmval_len << 2);
 	*q++ = htonl(bmval0);
 	*q++ = htonl(bmval1);
-	*q++ = htonl(bmval2);
+	if (bmval_len == 3)
+		*q++ = htonl(bmval2);
 	*q = htonl(len);
 
 /* out: */
-- 
1.8.3.1

[PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Trond Myklebust]

The attribute length is already calculated in advance. There is no
reason why we cannot calculate the bitmap in advance too so that
we don't have to play pointer games.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4xdr.c | 121 ++++++++++++++++++++++++-------------------------------
 1 file changed, 53 insertions(+), 68 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c74d616..98f937b 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -997,12 +997,10 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 	int owner_namelen = 0;
 	int owner_grouplen = 0;
 	__be32 *p;
-	__be32 *q;
-	int len;
-	uint32_t bmval_len = 2;
-	uint32_t bmval0 = 0;
-	uint32_t bmval1 = 0;
-	uint32_t bmval2 = 0;
+	unsigned i;
+	uint32_t len = 0;
+	uint32_t bmval_len;
+	uint32_t bmval[3] = { 0 };
 
 	/*
 	 * We reserve enough space to write the entire attribute buffer at once.
@@ -1011,13 +1009,14 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 	 * = 40 bytes, plus any contribution from variable-length fields
 	 *            such as owner/group.
 	 */
-	len = 8;
-
-	/* Sigh */
-	if (iap->ia_valid & ATTR_SIZE)
+	if (iap->ia_valid & ATTR_SIZE) {
+		bmval[0] |= FATTR4_WORD0_SIZE;
 		len += 8;
-	if (iap->ia_valid & ATTR_MODE)
+	}
+	if (iap->ia_valid & ATTR_MODE) {
+		bmval[1] |= FATTR4_WORD1_MODE;
 		len += 4;
+	}
 	if (iap->ia_valid & ATTR_UID) {
 		owner_namelen = nfs_map_uid_to_name(server, iap->ia_uid, owner_name, IDMAP_NAMESZ);
 		if (owner_namelen < 0) {
@@ -1028,6 +1027,7 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 			owner_namelen = sizeof("nobody") - 1;
 			/* goto out; */
 		}
+		bmval[1] |= FATTR4_WORD1_OWNER;
 		len += 4 + (XDR_QUADLEN(owner_namelen) << 2);
 	}
 	if (iap->ia_valid & ATTR_GID) {
@@ -1039,92 +1039,77 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 			owner_grouplen = sizeof("nobody") - 1;
 			/* goto out; */
 		}
+		bmval[1] |= FATTR4_WORD1_OWNER_GROUP;
 		len += 4 + (XDR_QUADLEN(owner_grouplen) << 2);
 	}
-	if (iap->ia_valid & ATTR_ATIME_SET)
+	if (iap->ia_valid & ATTR_ATIME_SET) {
+		bmval[1] |= FATTR4_WORD1_TIME_ACCESS_SET;
 		len += 16;
-	else if (iap->ia_valid & ATTR_ATIME)
+	} else if (iap->ia_valid & ATTR_ATIME) {
+		bmval[1] |= FATTR4_WORD1_TIME_ACCESS_SET;
 		len += 4;
-	if (iap->ia_valid & ATTR_MTIME_SET)
+	}
+	if (iap->ia_valid & ATTR_MTIME_SET) {
+		bmval[1] |= FATTR4_WORD1_TIME_MODIFY_SET;
 		len += 16;
-	else if (iap->ia_valid & ATTR_MTIME)
+	} else if (iap->ia_valid & ATTR_MTIME) {
+		bmval[1] |= FATTR4_WORD1_TIME_MODIFY_SET;
 		len += 4;
+	}
 	if (label) {
 		len += 4 + 4 + 4 + (XDR_QUADLEN(label->len) << 2);
-		bmval_len = 3;
+		bmval[2] |= FATTR4_WORD2_SECURITY_LABEL;
 	}
 
-	len += bmval_len << 2;
-	p = reserve_space(xdr, len);
+	if (bmval[2] != 0)
+		bmval_len = 3;
+	else if (bmval[1] != 0)
+		bmval_len = 2;
+	else
+		bmval_len = 1;
+
+	p = reserve_space(xdr, 4 + (bmval_len << 2) + 4 + len);
 
 	/*
 	 * We write the bitmap length now, but leave the bitmap and the attribute
 	 * buffer length to be backfilled at the end of this routine.
 	 */
 	*p++ = cpu_to_be32(bmval_len);
-	q = p;
-	/* Skip bitmap entries + attrlen */
-	p += bmval_len + 1;
+	for (i = 0; i < bmval_len; i++)
+		*p++ = cpu_to_be32(bmval[i]);
+	*p++ = cpu_to_be32(len);
 
-	if (iap->ia_valid & ATTR_SIZE) {
-		bmval0 |= FATTR4_WORD0_SIZE;
+	if (bmval[0] & FATTR4_WORD0_SIZE)
 		p = xdr_encode_hyper(p, iap->ia_size);
-	}
-	if (iap->ia_valid & ATTR_MODE) {
-		bmval1 |= FATTR4_WORD1_MODE;
+	if (bmval[1] & FATTR4_WORD1_MODE)
 		*p++ = cpu_to_be32(iap->ia_mode & S_IALLUGO);
-	}
-	if (iap->ia_valid & ATTR_UID) {
-		bmval1 |= FATTR4_WORD1_OWNER;
+	if (bmval[1] & FATTR4_WORD1_OWNER)
 		p = xdr_encode_opaque(p, owner_name, owner_namelen);
-	}
-	if (iap->ia_valid & ATTR_GID) {
-		bmval1 |= FATTR4_WORD1_OWNER_GROUP;
+	if (bmval[1] & FATTR4_WORD1_OWNER_GROUP)
 		p = xdr_encode_opaque(p, owner_group, owner_grouplen);
+	if (bmval[1] & FATTR4_WORD1_TIME_ACCESS_SET) {
+		if (iap->ia_valid & ATTR_ATIME_SET) {
+			*p++ = cpu_to_be32(NFS4_SET_TO_CLIENT_TIME);
+			p = xdr_encode_hyper(p, (s64)iap->ia_atime.tv_sec);
+			*p++ = cpu_to_be32(iap->ia_atime.tv_nsec);
+		} else
+			*p++ = cpu_to_be32(NFS4_SET_TO_SERVER_TIME);
 	}
-	if (iap->ia_valid & ATTR_ATIME_SET) {
-		bmval1 |= FATTR4_WORD1_TIME_ACCESS_SET;
-		*p++ = cpu_to_be32(NFS4_SET_TO_CLIENT_TIME);
-		p = xdr_encode_hyper(p, (s64)iap->ia_atime.tv_sec);
-		*p++ = cpu_to_be32(iap->ia_atime.tv_nsec);
-	}
-	else if (iap->ia_valid & ATTR_ATIME) {
-		bmval1 |= FATTR4_WORD1_TIME_ACCESS_SET;
-		*p++ = cpu_to_be32(NFS4_SET_TO_SERVER_TIME);
-	}
-	if (iap->ia_valid & ATTR_MTIME_SET) {
-		bmval1 |= FATTR4_WORD1_TIME_MODIFY_SET;
-		*p++ = cpu_to_be32(NFS4_SET_TO_CLIENT_TIME);
-		p = xdr_encode_hyper(p, (s64)iap->ia_mtime.tv_sec);
-		*p++ = cpu_to_be32(iap->ia_mtime.tv_nsec);
-	}
-	else if (iap->ia_valid & ATTR_MTIME) {
-		bmval1 |= FATTR4_WORD1_TIME_MODIFY_SET;
-		*p++ = cpu_to_be32(NFS4_SET_TO_SERVER_TIME);
+	if (bmval[1] & FATTR4_WORD1_TIME_MODIFY_SET) {
+		if (iap->ia_valid & ATTR_MTIME_SET) {
+			*p++ = cpu_to_be32(NFS4_SET_TO_CLIENT_TIME);
+			p = xdr_encode_hyper(p, (s64)iap->ia_mtime.tv_sec);
+			*p++ = cpu_to_be32(iap->ia_mtime.tv_nsec);
+		} else
+			*p++ = cpu_to_be32(NFS4_SET_TO_SERVER_TIME);
 	}
-	if (label) {
-		bmval2 |= FATTR4_WORD2_SECURITY_LABEL;
+	if (bmval[2] & FATTR4_WORD2_SECURITY_LABEL) {
 		*p++ = cpu_to_be32(label->lfs);
 		*p++ = cpu_to_be32(label->pi);
 		*p++ = cpu_to_be32(label->len);
 		p = xdr_encode_opaque_fixed(p, label->label, label->len);
 	}
 
-	/*
-	 * Now we backfill the bitmap and the attribute buffer length.
-	 */
-	if (len != ((char *)p - (char *)q) + 4) {
-		printk(KERN_ERR "NFS: Attr length error, %u != %Zu\n",
-				len, ((char *)p - (char *)q) + 4);
-		BUG();
-	}
-	len = (char *)p - (char *)q - (bmval_len << 2);
-	*q++ = htonl(bmval0);
-	*q++ = htonl(bmval1);
-	if (bmval_len == 3)
-		*q++ = htonl(bmval2);
-	*q = htonl(len);
-
 /* out: */
 }
 
-- 
1.8.3.1

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Andre Heider]

On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
> The attribute length is already calculated in advance. There is no
> reason why we cannot calculate the bitmap in advance too so that
> we don't have to play pointer games.
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

Tested-by: Andre Heider <a.heider@gmail.com>

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Andre Heider]

Trond,

On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
> The attribute length is already calculated in advance. There is no
> reason why we cannot calculate the bitmap in advance too so that
> we don't have to play pointer games.

I'm sorry to report that this patch seems to be more than just a cleanup.

I just tested 3.11-rc2 against my FreeBSD server, and with just patch
1/2 (as in -rc2) I still get the failure upon `touch`. It fails with
or without Rick's server patch.

Applying this one on top of -rc2 fixes it.

Regards,
Andre

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Myklebust, Trond]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote:
> Trond,
> 
> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> > The attribute length is already calculated in advance. There is no
> > reason why we cannot calculate the bitmap in advance too so that
> > we don't have to play pointer games.
> 
> I'm sorry to report that this patch seems to be more than just a cleanup.
> 
> I just tested 3.11-rc2 against my FreeBSD server, and with just patch
> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with
> or without Rick's server patch.
> 
> Applying this one on top of -rc2 fixes it.

How about the attached instead of the cleanup?

-- 
Trond Myklebust
Linux NFS client maintainer

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-NFSv4-Fix-brainfart-in-attribute-length-calculation.patch --]
[-- Type: text/x-patch; name="0001-NFSv4-Fix-brainfart-in-attribute-length-calculation.patch", Size: 963 bytes --]

From bd00a23bba91f54ab45e8788a8306733d8ca3062 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@netapp.com>
Date: Tue, 23 Jul 2013 12:53:39 -0400
Subject: [PATCH] NFSv4: Fix brainfart in attribute length calculation

The calculation of the attribute length was 4 bytes off.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/nfs4xdr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c74d616..3850b01 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1118,11 +1118,11 @@ static void encode_attrs(struct xdr_stream *xdr, const struct iattr *iap,
 				len, ((char *)p - (char *)q) + 4);
 		BUG();
 	}
-	len = (char *)p - (char *)q - (bmval_len << 2);
 	*q++ = htonl(bmval0);
 	*q++ = htonl(bmval1);
 	if (bmval_len == 3)
 		*q++ = htonl(bmval2);
+	len = (char *)p - (char *)(q + 1);
 	*q = htonl(len);
 
 /* out: */
-- 
1.8.3.1

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Andre Heider]

On Tue, Jul 23, 2013 at 7:00 PM, Myklebust, Trond
<Trond.Myklebust@netapp.com> wrote:
> On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote:
>> Trond,
>>
>> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
>> <Trond.Myklebust@netapp.com> wrote:
>> > The attribute length is already calculated in advance. There is no
>> > reason why we cannot calculate the bitmap in advance too so that
>> > we don't have to play pointer games.
>>
>> I'm sorry to report that this patch seems to be more than just a cleanup.
>>
>> I just tested 3.11-rc2 against my FreeBSD server, and with just patch
>> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with
>> or without Rick's server patch.
>>
>> Applying this one on top of -rc2 fixes it.
>
> How about the attached instead of the cleanup?

Much better ;) Thanks again for the quick fix!

Feel free to add:
Tested-by: Andre Heider <a.heider@gmail.com>

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Chuck Lever]

On Jul 23, 2013, at 1:00 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote:
>> Trond,
>> 
>> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
>> <Trond.Myklebust@netapp.com> wrote:
>>> The attribute length is already calculated in advance. There is no
>>> reason why we cannot calculate the bitmap in advance too so that
>>> we don't have to play pointer games.
>> 
>> I'm sorry to report that this patch seems to be more than just a cleanup.
>> 
>> I just tested 3.11-rc2 against my FreeBSD server, and with just patch
>> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with
>> or without Rick's server patch.
>> 
>> Applying this one on top of -rc2 fixes it.
> 
> How about the attached instead of the cleanup?

Even with this patch applied, cthon basic tests fail immediately for me.  The mkdir to create the test directory fails with EIO.

The wire trace shows that the CREATE operation is malformed: the client sends a length of 8 for the 4-octet mode attribute value at the end of the operation.

This causes the server to skip over the following GETFH operation -- it thinks the client has sent 3 operations, when the client told it to expect 4 in this compound.

Here:

1064         *p++ = cpu_to_be32(bmval_len);
1065         q = p;
1066         /* Skip bitmap entries + attrlen */
1067         p += bmval_len + 1;

You've skipped over the 4-octet attrlen field, but here:

1125         *q++ = htonl(bmval0);
1126         *q++ = htonl(bmval1);
1127         if (bmval_len == 3)
1128                 *q++ = htonl(bmval2);
1129         len = (char *)p - (char *)q;
1130         *q = htonl(len);

have you failed to take the attrlen field into account when computing the length of the attribute values?

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Myklebust, Trond]

T24gVHVlLCAyMDEzLTA3LTIzIGF0IDE2OjE0IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gSnVsIDIzLCAyMDEzLCBhdCAxOjAwIFBNLCAiTXlrbGVidXN0LCBUcm9uZCIgPFRyb25kLk15
a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90ZToNCj4gDQo+ID4gT24gVHVlLCAyMDEzLTA3LTIzIGF0
IDE3OjU5ICswMjAwLCBBbmRyZSBIZWlkZXIgd3JvdGU6DQo+ID4+IFRyb25kLA0KPiA+PiANCj4g
Pj4gT24gV2VkLCBKdWwgMTcsIDIwMTMgYXQgMTE6NTkgUE0sIFRyb25kIE15a2xlYnVzdA0KPiA+
PiA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+IHdyb3RlOg0KPiA+Pj4gVGhlIGF0dHJpYnV0
ZSBsZW5ndGggaXMgYWxyZWFkeSBjYWxjdWxhdGVkIGluIGFkdmFuY2UuIFRoZXJlIGlzIG5vDQo+
ID4+PiByZWFzb24gd2h5IHdlIGNhbm5vdCBjYWxjdWxhdGUgdGhlIGJpdG1hcCBpbiBhZHZhbmNl
IHRvbyBzbyB0aGF0DQo+ID4+PiB3ZSBkb24ndCBoYXZlIHRvIHBsYXkgcG9pbnRlciBnYW1lcy4N
Cj4gPj4gDQo+ID4+IEknbSBzb3JyeSB0byByZXBvcnQgdGhhdCB0aGlzIHBhdGNoIHNlZW1zIHRv
IGJlIG1vcmUgdGhhbiBqdXN0IGEgY2xlYW51cC4NCj4gPj4gDQo+ID4+IEkganVzdCB0ZXN0ZWQg
My4xMS1yYzIgYWdhaW5zdCBteSBGcmVlQlNEIHNlcnZlciwgYW5kIHdpdGgganVzdCBwYXRjaA0K
PiA+PiAxLzIgKGFzIGluIC1yYzIpIEkgc3RpbGwgZ2V0IHRoZSBmYWlsdXJlIHVwb24gYHRvdWNo
YC4gSXQgZmFpbHMgd2l0aA0KPiA+PiBvciB3aXRob3V0IFJpY2sncyBzZXJ2ZXIgcGF0Y2guDQo+
ID4+IA0KPiA+PiBBcHBseWluZyB0aGlzIG9uZSBvbiB0b3Agb2YgLXJjMiBmaXhlcyBpdC4NCj4g
PiANCj4gPiBIb3cgYWJvdXQgdGhlIGF0dGFjaGVkIGluc3RlYWQgb2YgdGhlIGNsZWFudXA/DQo+
IA0KPiBFdmVuIHdpdGggdGhpcyBwYXRjaCBhcHBsaWVkLCBjdGhvbiBiYXNpYyB0ZXN0cyBmYWls
IGltbWVkaWF0ZWx5IGZvciBtZS4gIFRoZSBta2RpciB0byBjcmVhdGUgdGhlIHRlc3QgZGlyZWN0
b3J5IGZhaWxzIHdpdGggRUlPLg0KPiANCj4gVGhlIHdpcmUgdHJhY2Ugc2hvd3MgdGhhdCB0aGUg
Q1JFQVRFIG9wZXJhdGlvbiBpcyBtYWxmb3JtZWQ6IHRoZSBjbGllbnQgc2VuZHMgYSBsZW5ndGgg
b2YgOCBmb3IgdGhlIDQtb2N0ZXQgbW9kZSBhdHRyaWJ1dGUgdmFsdWUgYXQgdGhlIGVuZCBvZiB0
aGUgb3BlcmF0aW9uLg0KPiANCj4gVGhpcyBjYXVzZXMgdGhlIHNlcnZlciB0byBza2lwIG92ZXIg
dGhlIGZvbGxvd2luZyBHRVRGSCBvcGVyYXRpb24gLS0gaXQgdGhpbmtzIHRoZSBjbGllbnQgaGFz
IHNlbnQgMyBvcGVyYXRpb25zLCB3aGVuIHRoZSBjbGllbnQgdG9sZCBpdCB0byBleHBlY3QgNCBp
biB0aGlzIGNvbXBvdW5kLg0KPiANCj4gSGVyZToNCj4gDQo+IDEwNjQgICAgICAgICAqcCsrID0g
Y3B1X3RvX2JlMzIoYm12YWxfbGVuKTsNCj4gMTA2NSAgICAgICAgIHEgPSBwOw0KPiAxMDY2ICAg
ICAgICAgLyogU2tpcCBiaXRtYXAgZW50cmllcyArIGF0dHJsZW4gKi8NCj4gMTA2NyAgICAgICAg
IHAgKz0gYm12YWxfbGVuICsgMTsNCj4gDQo+IFlvdSd2ZSBza2lwcGVkIG92ZXIgdGhlIDQtb2N0
ZXQgYXR0cmxlbiBmaWVsZCwgYnV0IGhlcmU6DQo+IA0KPiAxMTI1ICAgICAgICAgKnErKyA9IGh0
b25sKGJtdmFsMCk7DQo+IDExMjYgICAgICAgICAqcSsrID0gaHRvbmwoYm12YWwxKTsNCj4gMTEy
NyAgICAgICAgIGlmIChibXZhbF9sZW4gPT0gMykNCj4gMTEyOCAgICAgICAgICAgICAgICAgKnEr
KyA9IGh0b25sKGJtdmFsMik7DQo+IDExMjkgICAgICAgICBsZW4gPSAoY2hhciAqKXAgLSAoY2hh
ciAqKXE7DQoNCkluIHRoZSBwYXRjaCBJIHNlbnQgb3V0LCB0aGUgYWJvdmUgc2hvdWxkIHJlYWQg
KHErMSkuLi4NCg0KPiAxMTMwICAgICAgICAgKnEgPSBodG9ubChsZW4pOw0KPiANCj4gaGF2ZSB5
b3UgZmFpbGVkIHRvIHRha2UgdGhlIGF0dHJsZW4gZmllbGQgaW50byBhY2NvdW50IHdoZW4gY29t
cHV0aW5nIHRoZSBsZW5ndGggb2YgdGhlIGF0dHJpYnV0ZSB2YWx1ZXM/DQo+IA0KDQotLSANClRy
b25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJv
bmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

Re: [PATCH 2/2] NFSv4: encode_attrs should not backfill the bitmap and attribute length

[Chuck Lever]

On Jul 23, 2013, at 5:19 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-07-23 at 16:14 -0400, Chuck Lever wrote:
>> On Jul 23, 2013, at 1:00 PM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
>> 
>>> On Tue, 2013-07-23 at 17:59 +0200, Andre Heider wrote:
>>>> Trond,
>>>> 
>>>> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
>>>> <Trond.Myklebust@netapp.com> wrote:
>>>>> The attribute length is already calculated in advance. There is no
>>>>> reason why we cannot calculate the bitmap in advance too so that
>>>>> we don't have to play pointer games.
>>>> 
>>>> I'm sorry to report that this patch seems to be more than just a cleanup.
>>>> 
>>>> I just tested 3.11-rc2 against my FreeBSD server, and with just patch
>>>> 1/2 (as in -rc2) I still get the failure upon `touch`. It fails with
>>>> or without Rick's server patch.
>>>> 
>>>> Applying this one on top of -rc2 fixes it.
>>> 
>>> How about the attached instead of the cleanup?
>> 
>> Even with this patch applied, cthon basic tests fail immediately for me.  The mkdir to create the test directory fails with EIO.
>> 
>> The wire trace shows that the CREATE operation is malformed: the client sends a length of 8 for the 4-octet mode attribute value at the end of the operation.
>> 
>> This causes the server to skip over the following GETFH operation -- it thinks the client has sent 3 operations, when the client told it to expect 4 in this compound.
>> 
>> Here:
>> 
>> 1064         *p++ = cpu_to_be32(bmval_len);
>> 1065         q = p;
>> 1066         /* Skip bitmap entries + attrlen */
>> 1067         p += bmval_len + 1;
>> 
>> You've skipped over the 4-octet attrlen field, but here:
>> 
>> 1125         *q++ = htonl(bmval0);
>> 1126         *q++ = htonl(bmval1);
>> 1127         if (bmval_len == 3)
>> 1128                 *q++ = htonl(bmval2);
>> 1129         len = (char *)p - (char *)q;
> 
> In the patch I sent out, the above should read (q+1)...

D'OH!

> 
>> 1130         *q = htonl(len);
>> 
>> have you failed to take the attrlen field into account when computing the length of the attribute values?
>> 

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[Rick Macklem]

Trond Myklebust wrote:
> Technically, the Linux client is allowed by the NFSv4 spec to send
> 3 word bitmaps as part of an OPEN request. However, this causes the
> current FreeBSD server to return NFS4ERR_ATTRNOTSUPP errors.
> 
Yep. The code in the FreeBSD distros is only NFSv4.0, which only had
bits up to 56. I suppose it should just ignore high order bitmap
words. (My current code, which isn't in a distro yet, does handle
3 word bitmaps as a part of the NFSv4.1 support. Maybe I'll change it
to allow any number of bitmap words, so long as the high order words
are 0.)

Thanks for patching this, since it will be some time before the newer
FreeBSD server code gets released, rick

> Fix the regression by making the Linux client use a 2 word bitmap
> unless
> doing NFSv4.2 with labeled NFS.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
>  fs/nfs/nfs4xdr.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index 0abfb846..c74d616 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -999,6 +999,7 @@ static void encode_attrs(struct xdr_stream *xdr,
> const struct iattr *iap,
>  	__be32 *p;
>  	__be32 *q;
>  	int len;
> +	uint32_t bmval_len = 2;
>  	uint32_t bmval0 = 0;
>  	uint32_t bmval1 = 0;
>  	uint32_t bmval2 = 0;
> @@ -1010,7 +1011,7 @@ static void encode_attrs(struct xdr_stream
> *xdr, const struct iattr *iap,
>  	 * = 40 bytes, plus any contribution from variable-length fields
>  	 *            such as owner/group.
>  	 */
> -	len = 20;
> +	len = 8;
>  
>  	/* Sigh */
>  	if (iap->ia_valid & ATTR_SIZE)
> @@ -1040,8 +1041,6 @@ static void encode_attrs(struct xdr_stream
> *xdr, const struct iattr *iap,
>  		}
>  		len += 4 + (XDR_QUADLEN(owner_grouplen) << 2);
>  	}
> -	if (label)
> -		len += 4 + 4 + 4 + (XDR_QUADLEN(label->len) << 2);
>  	if (iap->ia_valid & ATTR_ATIME_SET)
>  		len += 16;
>  	else if (iap->ia_valid & ATTR_ATIME)
> @@ -1050,15 +1049,22 @@ static void encode_attrs(struct xdr_stream
> *xdr, const struct iattr *iap,
>  		len += 16;
>  	else if (iap->ia_valid & ATTR_MTIME)
>  		len += 4;
> +	if (label) {
> +		len += 4 + 4 + 4 + (XDR_QUADLEN(label->len) << 2);
> +		bmval_len = 3;
> +	}
> +
> +	len += bmval_len << 2;
>  	p = reserve_space(xdr, len);
>  
>  	/*
>  	 * We write the bitmap length now, but leave the bitmap and the
>  	 attribute
>  	 * buffer length to be backfilled at the end of this routine.
>  	 */
> -	*p++ = cpu_to_be32(3);
> +	*p++ = cpu_to_be32(bmval_len);
>  	q = p;
> -	p += 4;
> +	/* Skip bitmap entries + attrlen */
> +	p += bmval_len + 1;
>  
>  	if (iap->ia_valid & ATTR_SIZE) {
>  		bmval0 |= FATTR4_WORD0_SIZE;
> @@ -1112,10 +1118,11 @@ static void encode_attrs(struct xdr_stream
> *xdr, const struct iattr *iap,
>  				len, ((char *)p - (char *)q) + 4);
>  		BUG();
>  	}
> -	len = (char *)p - (char *)q - 16;
> +	len = (char *)p - (char *)q - (bmval_len << 2);
>  	*q++ = htonl(bmval0);
>  	*q++ = htonl(bmval1);
> -	*q++ = htonl(bmval2);
> +	if (bmval_len == 3)
> +		*q++ = htonl(bmval2);
>  	*q = htonl(len);
>  
>  /* out: */
> --
> 1.8.3.1
> 
> 

Re: [PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[Andre Heider]

On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
<Trond.Myklebust@netapp.com> wrote:
> Technically, the Linux client is allowed by the NFSv4 spec to send
> 3 word bitmaps as part of an OPEN request. However, this causes the
> current FreeBSD server to return NFS4ERR_ATTRNOTSUPP errors.
>
> Fix the regression by making the Linux client use a 2 word bitmap unless
> doing NFSv4.2 with labeled NFS.
>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>

Tested-by: Andre Heider <a.heider@gmail.com>

Re: [PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[Rick Macklem]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

Andre Heider wrote:
> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
> <Trond.Myklebust@netapp.com> wrote:
> > Technically, the Linux client is allowed by the NFSv4 spec to send
> > 3 word bitmaps as part of an OPEN request. However, this causes the
> > current FreeBSD server to return NFS4ERR_ATTRNOTSUPP errors.
> >
> > Fix the regression by making the Linux client use a 2 word bitmap
> > unless
> > doing NFSv4.2 with labeled NFS.
> >
> > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> Tested-by: Andre Heider <a.heider@gmail.com>
> 
I've attached the patch I plan to commit to FreeBSD's head soon, which
fixes the server so that it checks for the high order bitmaps words
being non-zero before replying with NFS4_ERR_ATTRNOTSUPP.

The patch is pretty straightforward, but if you can apply it to your
server and test it against the unpatched Linux client, that would be
appreciated.

It will take a while for the patch to find its way to a FreeBSD release,
so having the workaround in the Linux client will be very helpful.

Thanks for reporting this, rick


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: big-attrbits.patch --]
[-- Type: text/x-patch; name=big-attrbits.patch, Size: 976 bytes --]

--- sys/fs/nfs/nfs_commonsubs.c.orig	2013-07-18 19:15:25.000000000 -0400
+++ sys/fs/nfs/nfs_commonsubs.c	2013-07-18 19:15:58.000000000 -0400
@@ -761,21 +761,21 @@ nfsrv_getattrbits(struct nfsrv_descript 
 		error = NFSERR_BADXDR;
 		goto nfsmout;
 	}
-	if (cnt > NFSATTRBIT_MAXWORDS) {
+	if (cnt > NFSATTRBIT_MAXWORDS)
 		outcnt = NFSATTRBIT_MAXWORDS;
-		if (retnotsupp)
-			*retnotsupp = NFSERR_ATTRNOTSUPP;
-	} else {
+	else
 		outcnt = cnt;
-	}
 	NFSZERO_ATTRBIT(attrbitp);
 	if (outcnt > 0) {
 		NFSM_DISSECT(tl, u_int32_t *, outcnt * NFSX_UNSIGNED);
 		for (i = 0; i < outcnt; i++)
 			attrbitp->bits[i] = fxdr_unsigned(u_int32_t, *tl++);
 	}
-	if (cnt > outcnt)
-		error = nfsm_advance(nd, (cnt - outcnt) * NFSX_UNSIGNED, -1);
+	for (i = 0; i < (cnt - outcnt); i++) {
+		NFSM_DISSECT(tl, u_int32_t *, NFSX_UNSIGNED);
+		if (retnotsupp != NULL && *tl != 0)
+			*retnotsupp = NFSERR_ATTRNOTSUPP;
+	}
 	if (cntp)
 		*cntp = NFSX_UNSIGNED + (cnt * NFSX_UNSIGNED);
 nfsmout:

Re: [PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[Myklebust, Trond]

T24gVGh1LCAyMDEzLTA3LTE4IGF0IDE5OjMwIC0wNDAwLCBSaWNrIE1hY2tsZW0gd3JvdGU6DQo+
IEFuZHJlIEhlaWRlciB3cm90ZToNCj4gPiBPbiBXZWQsIEp1bCAxNywgMjAxMyBhdCAxMTo1OSBQ
TSwgVHJvbmQgTXlrbGVidXN0DQo+ID4gPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90
ZToNCj4gPiA+IFRlY2huaWNhbGx5LCB0aGUgTGludXggY2xpZW50IGlzIGFsbG93ZWQgYnkgdGhl
IE5GU3Y0IHNwZWMgdG8gc2VuZA0KPiA+ID4gMyB3b3JkIGJpdG1hcHMgYXMgcGFydCBvZiBhbiBP
UEVOIHJlcXVlc3QuIEhvd2V2ZXIsIHRoaXMgY2F1c2VzIHRoZQ0KPiA+ID4gY3VycmVudCBGcmVl
QlNEIHNlcnZlciB0byByZXR1cm4gTkZTNEVSUl9BVFRSTk9UU1VQUCBlcnJvcnMuDQo+ID4gPg0K
PiA+ID4gRml4IHRoZSByZWdyZXNzaW9uIGJ5IG1ha2luZyB0aGUgTGludXggY2xpZW50IHVzZSBh
IDIgd29yZCBiaXRtYXANCj4gPiA+IHVubGVzcw0KPiA+ID4gZG9pbmcgTkZTdjQuMiB3aXRoIGxh
YmVsZWQgTkZTLg0KPiA+ID4NCj4gPiA+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQo+ID4gDQo+ID4gVGVzdGVkLWJ5OiBBbmRyZSBI
ZWlkZXIgPGEuaGVpZGVyQGdtYWlsLmNvbT4NCj4gPiANCj4gSSd2ZSBhdHRhY2hlZCB0aGUgcGF0
Y2ggSSBwbGFuIHRvIGNvbW1pdCB0byBGcmVlQlNEJ3MgaGVhZCBzb29uLCB3aGljaA0KPiBmaXhl
cyB0aGUgc2VydmVyIHNvIHRoYXQgaXQgY2hlY2tzIGZvciB0aGUgaGlnaCBvcmRlciBiaXRtYXBz
IHdvcmRzDQo+IGJlaW5nIG5vbi16ZXJvIGJlZm9yZSByZXBseWluZyB3aXRoIE5GUzRfRVJSX0FU
VFJOT1RTVVBQLg0KPiANCj4gVGhlIHBhdGNoIGlzIHByZXR0eSBzdHJhaWdodGZvcndhcmQsIGJ1
dCBpZiB5b3UgY2FuIGFwcGx5IGl0IHRvIHlvdXINCj4gc2VydmVyIGFuZCB0ZXN0IGl0IGFnYWlu
c3QgdGhlIHVucGF0Y2hlZCBMaW51eCBjbGllbnQsIHRoYXQgd291bGQgYmUNCj4gYXBwcmVjaWF0
ZWQuDQo+IA0KPiBJdCB3aWxsIHRha2UgYSB3aGlsZSBmb3IgdGhlIHBhdGNoIHRvIGZpbmQgaXRz
IHdheSB0byBhIEZyZWVCU0QgcmVsZWFzZSwNCj4gc28gaGF2aW5nIHRoZSB3b3JrYXJvdW5kIGlu
IHRoZSBMaW51eCBjbGllbnQgd2lsbCBiZSB2ZXJ5IGhlbHBmdWwuDQoNClJpZ2h0LiBTbywgbXkg
cGxhbiBpcyB0byBzZW5kIHBhdGNoIDEvMiBhcyBhIGJ1Z2ZpeCB3aXRoaW4gdGhlIG5leHQgZmV3
DQpkYXlzLCB0byBlbnN1cmUgdGhhdCB3ZSBlbmQgdXAgd2l0aCBubyBnYXAgaW4gTkZTdjQgY2xp
ZW50IGZ1bmN0aW9uYWxpdHkNCmJldHdlZW4gdGhlIHJlbGVhc2UgdmVyc2lvbnMgb2YgTGludXgg
My4xMCBhbmQgMy4xMS4NCg0KU2luY2UgMi8yIGlzIG1vcmUgb2YgYSBjbGVhbnVwLCBhbmQgYWN0
dWFsbHkgY2hhbmdlcyB0aGUgZnVuY3Rpb25hbGl0eQ0KdG8gc2VuZCBvbmx5IGEgMS13b3JkIGJp
dG1hcCBmb3IgdHJ1bmNhdGUoKSBjYWxscywgSSdsbCB3YWl0IGZvciB0aGUNCjMuMTIgbWVyZ2Ug
d2luZG93IGJlZm9yZSBjb21taXR0aW5nIHRoYXQuDQoNCkNoZWVycw0KICBUcm9uZA0KDQotLSAN
ClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0K
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQo=

Re: [PATCH 1/2] NFSv4: Fix a regression against the FreeBSD server

[Andre Heider]

On Fri, Jul 19, 2013 at 1:30 AM, Rick Macklem <rmacklem@uoguelph.ca> wrote:
> Andre Heider wrote:
>> On Wed, Jul 17, 2013 at 11:59 PM, Trond Myklebust
>> <Trond.Myklebust@netapp.com> wrote:
>> > Technically, the Linux client is allowed by the NFSv4 spec to send
>> > 3 word bitmaps as part of an OPEN request. However, this causes the
>> > current FreeBSD server to return NFS4ERR_ATTRNOTSUPP errors.
>> >
>> > Fix the regression by making the Linux client use a 2 word bitmap
>> > unless
>> > doing NFSv4.2 with labeled NFS.
>> >
>> > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
>>
>> Tested-by: Andre Heider <a.heider@gmail.com>
>>
> I've attached the patch I plan to commit to FreeBSD's head soon, which
> fixes the server so that it checks for the high order bitmaps words
> being non-zero before replying with NFS4_ERR_ATTRNOTSUPP.
>
> The patch is pretty straightforward, but if you can apply it to your
> server and test it against the unpatched Linux client, that would be
> appreciated.

Confirmed, reverting the client patches and applying this server patch
also works ;)

Thanks,
Andre