From: "J. Bruce Fields" <[email protected]>
The calculation of attribute length fields is too high by four because
it incorrectly includes the length field itself.
This regression was introduced by
b4a2cf76ab7c08628c62b2062dacefa496b59dfd "NFSv4: Fix a regression
against the FreeBSD server" and causes OPENs to the Linux NFS server to
fail with BADXDR errors (translated by the client into EIO).
Signed-off-by: J. Bruce Fields <[email protected]>
---
fs/nfs/nfs4xdr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
I thought you guys had automated testing against the Linux server? How
did this slip through into upstream?
--b.
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c74d616..d6d6754 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -1118,7 +1118,7 @@ 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);
+ len = (char *)p - (char *)q - (bmval_len + 1 << 2);
*q++ = htonl(bmval0);
*q++ = htonl(bmval1);
if (bmval_len == 3)
--
1.7.9.5
T24gV2VkLCAyMDEzLTA3LTI0IGF0IDEwOjE0IC0wNDAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IEZyb206ICJKLiBCcnVjZSBGaWVsZHMiIDxiZmllbGRzQHJlZGhhdC5jb20+DQo+IA0KPiBU
aGUgY2FsY3VsYXRpb24gb2YgYXR0cmlidXRlIGxlbmd0aCBmaWVsZHMgaXMgdG9vIGhpZ2ggYnkg
Zm91ciBiZWNhdXNlDQo+IGl0IGluY29ycmVjdGx5IGluY2x1ZGVzIHRoZSBsZW5ndGggZmllbGQg
aXRzZWxmLg0KPiANCj4gVGhpcyByZWdyZXNzaW9uIHdhcyBpbnRyb2R1Y2VkIGJ5DQo+IGI0YTJj
Zjc2YWI3YzA4NjI4YzYyYjIwNjJkYWNlZmE0OTZiNTlkZmQgIk5GU3Y0OiBGaXggYSByZWdyZXNz
aW9uDQo+IGFnYWluc3QgdGhlIEZyZWVCU0Qgc2VydmVyIiBhbmQgY2F1c2VzIE9QRU5zIHRvIHRo
ZSBMaW51eCBORlMgc2VydmVyIHRvDQo+IGZhaWwgd2l0aCBCQURYRFIgZXJyb3JzICh0cmFuc2xh
dGVkIGJ5IHRoZSBjbGllbnQgaW50byBFSU8pLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogSi4gQnJ1
Y2UgRmllbGRzIDxiZmllbGRzQHJlZGhhdC5jb20+DQo+IC0tLQ0KPiAgZnMvbmZzL25mczR4ZHIu
YyB8ICAgIDIgKy0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlv
bigtKQ0KPiANCj4gSSB0aG91Z2h0IHlvdSBndXlzIGhhZCBhdXRvbWF0ZWQgdGVzdGluZyBhZ2Fp
bnN0IHRoZSBMaW51eCBzZXJ2ZXI/ICBIb3cNCj4gZGlkIHRoaXMgc2xpcCB0aHJvdWdoIGludG8g
dXBzdHJlYW0/DQo+IA0KPiAtLWIuDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczR4ZHIu
YyBiL2ZzL25mcy9uZnM0eGRyLmMNCj4gaW5kZXggYzc0ZDYxNi4uZDZkNjc1NCAxMDA2NDQNCj4g
LS0tIGEvZnMvbmZzL25mczR4ZHIuYw0KPiArKysgYi9mcy9uZnMvbmZzNHhkci5jDQo+IEBAIC0x
MTE4LDcgKzExMTgsNyBAQCBzdGF0aWMgdm9pZCBlbmNvZGVfYXR0cnMoc3RydWN0IHhkcl9zdHJl
YW0gKnhkciwgY29uc3Qgc3RydWN0IGlhdHRyICppYXAsDQo+ICAJCQkJbGVuLCAoKGNoYXIgKilw
IC0gKGNoYXIgKilxKSArIDQpOw0KPiAgCQlCVUcoKTsNCj4gIAl9DQo+IC0JbGVuID0gKGNoYXIg
KilwIC0gKGNoYXIgKilxIC0gKGJtdmFsX2xlbiA8PCAyKTsNCj4gKwlsZW4gPSAoY2hhciAqKXAg
LSAoY2hhciAqKXEgLSAoYm12YWxfbGVuICsgMSA8PCAyKTsNCj4gIAkqcSsrID0gaHRvbmwoYm12
YWwwKTsNCj4gIAkqcSsrID0gaHRvbmwoYm12YWwxKTsNCj4gIAlpZiAoYm12YWxfbGVuID09IDMp
DQoNClBsZWFzZSBzZWUgY29tbWl0IDRmM2NjNDgwOWE5OGExNjVhOTcwOGI3MmI0N2RlNzE2NDM3
OTdiYmQgKE5GU3Y0OiBGaXgNCmJyYWluZmFydCBpbiBhdHRyaWJ1dGUgbGVuZ3RoIGNhbGN1bGF0
aW9uKSB1cHN0cmVhbS4NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQg
bWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0
YXBwLmNvbQ0K
On Wed, Jul 24, 2013 at 02:41:54PM +0000, Myklebust, Trond wrote:
> On Wed, 2013-07-24 at 10:38 -0400, J. Bruce Fields wrote:
> > On Wed, Jul 24, 2013 at 02:30:58PM +0000, Myklebust, Trond wrote:
> > > On Wed, 2013-07-24 at 10:14 -0400, J. Bruce Fields wrote:
> > > > From: "J. Bruce Fields" <[email protected]>
> > > >
> > > > The calculation of attribute length fields is too high by four because
> > > > it incorrectly includes the length field itself.
> > > >
> > > > This regression was introduced by
> > > > b4a2cf76ab7c08628c62b2062dacefa496b59dfd "NFSv4: Fix a regression
> > > > against the FreeBSD server" and causes OPENs to the Linux NFS server to
> > > > fail with BADXDR errors (translated by the client into EIO).
> > > >
> > > > Signed-off-by: J. Bruce Fields <[email protected]>
> > > > ---
> > > > fs/nfs/nfs4xdr.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > I thought you guys had automated testing against the Linux server? How
> > > > did this slip through into upstream?
> > > >
> > > > --b.
> > > >
> > > > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > > > index c74d616..d6d6754 100644
> > > > --- a/fs/nfs/nfs4xdr.c
> > > > +++ b/fs/nfs/nfs4xdr.c
> > > > @@ -1118,7 +1118,7 @@ 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);
> > > > + len = (char *)p - (char *)q - (bmval_len + 1 << 2);
> > > > *q++ = htonl(bmval0);
> > > > *q++ = htonl(bmval1);
> > > > if (bmval_len == 3)
> > >
> > > Please see commit 4f3cc4809a98a165a9708b72b47de71643797bbd (NFSv4: Fix
> > > brainfart in attribute length calculation) upstream.
> >
> > Oh, good, thanks!
> >
> > But I still don't understand how this made it into upstream in the first
> > place.
> >
> > Any NFSv4 testing at all against the Linux server would catch this, and
> > I thought you had automated testing set up that you could run before
> > submission.
>
> Yes, however it was truly a brainfart: the patch was never tested
> independently of the cleanup which removes the backfilling altogether.
Would it be possible to fix the testing process so that it takes exactly
the commit that you're sending in the pull request?
--b.
T24gV2VkLCAyMDEzLTA3LTI0IGF0IDEwOjM4IC0wNDAwLCBKLiBCcnVjZSBGaWVsZHMgd3JvdGU6
DQo+IE9uIFdlZCwgSnVsIDI0LCAyMDEzIGF0IDAyOjMwOjU4UE0gKzAwMDAsIE15a2xlYnVzdCwg
VHJvbmQgd3JvdGU6DQo+ID4gT24gV2VkLCAyMDEzLTA3LTI0IGF0IDEwOjE0IC0wNDAwLCBKLiBC
cnVjZSBGaWVsZHMgd3JvdGU6DQo+ID4gPiBGcm9tOiAiSi4gQnJ1Y2UgRmllbGRzIiA8YmZpZWxk
c0ByZWRoYXQuY29tPg0KPiA+ID4gDQo+ID4gPiBUaGUgY2FsY3VsYXRpb24gb2YgYXR0cmlidXRl
IGxlbmd0aCBmaWVsZHMgaXMgdG9vIGhpZ2ggYnkgZm91ciBiZWNhdXNlDQo+ID4gPiBpdCBpbmNv
cnJlY3RseSBpbmNsdWRlcyB0aGUgbGVuZ3RoIGZpZWxkIGl0c2VsZi4NCj4gPiA+IA0KPiA+ID4g
VGhpcyByZWdyZXNzaW9uIHdhcyBpbnRyb2R1Y2VkIGJ5DQo+ID4gPiBiNGEyY2Y3NmFiN2MwODYy
OGM2MmIyMDYyZGFjZWZhNDk2YjU5ZGZkICJORlN2NDogRml4IGEgcmVncmVzc2lvbg0KPiA+ID4g
YWdhaW5zdCB0aGUgRnJlZUJTRCBzZXJ2ZXIiIGFuZCBjYXVzZXMgT1BFTnMgdG8gdGhlIExpbnV4
IE5GUyBzZXJ2ZXIgdG8NCj4gPiA+IGZhaWwgd2l0aCBCQURYRFIgZXJyb3JzICh0cmFuc2xhdGVk
IGJ5IHRoZSBjbGllbnQgaW50byBFSU8pLg0KPiA+ID4gDQo+ID4gPiBTaWduZWQtb2ZmLWJ5OiBK
LiBCcnVjZSBGaWVsZHMgPGJmaWVsZHNAcmVkaGF0LmNvbT4NCj4gPiA+IC0tLQ0KPiA+ID4gIGZz
L25mcy9uZnM0eGRyLmMgfCAgICAyICstDQo+ID4gPiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0
aW9uKCspLCAxIGRlbGV0aW9uKC0pDQo+ID4gPiANCj4gPiA+IEkgdGhvdWdodCB5b3UgZ3V5cyBo
YWQgYXV0b21hdGVkIHRlc3RpbmcgYWdhaW5zdCB0aGUgTGludXggc2VydmVyPyAgSG93DQo+ID4g
PiBkaWQgdGhpcyBzbGlwIHRocm91Z2ggaW50byB1cHN0cmVhbT8NCj4gPiA+IA0KPiA+ID4gLS1i
Lg0KPiA+ID4gDQo+ID4gPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczR4ZHIuYyBiL2ZzL25mcy9u
ZnM0eGRyLmMNCj4gPiA+IGluZGV4IGM3NGQ2MTYuLmQ2ZDY3NTQgMTAwNjQ0DQo+ID4gPiAtLS0g
YS9mcy9uZnMvbmZzNHhkci5jDQo+ID4gPiArKysgYi9mcy9uZnMvbmZzNHhkci5jDQo+ID4gPiBA
QCAtMTExOCw3ICsxMTE4LDcgQEAgc3RhdGljIHZvaWQgZW5jb2RlX2F0dHJzKHN0cnVjdCB4ZHJf
c3RyZWFtICp4ZHIsIGNvbnN0IHN0cnVjdCBpYXR0ciAqaWFwLA0KPiA+ID4gIAkJCQlsZW4sICgo
Y2hhciAqKXAgLSAoY2hhciAqKXEpICsgNCk7DQo+ID4gPiAgCQlCVUcoKTsNCj4gPiA+ICAJfQ0K
PiA+ID4gLQlsZW4gPSAoY2hhciAqKXAgLSAoY2hhciAqKXEgLSAoYm12YWxfbGVuIDw8IDIpOw0K
PiA+ID4gKwlsZW4gPSAoY2hhciAqKXAgLSAoY2hhciAqKXEgLSAoYm12YWxfbGVuICsgMSA8PCAy
KTsNCj4gPiA+ICAJKnErKyA9IGh0b25sKGJtdmFsMCk7DQo+ID4gPiAgCSpxKysgPSBodG9ubChi
bXZhbDEpOw0KPiA+ID4gIAlpZiAoYm12YWxfbGVuID09IDMpDQo+ID4gDQo+ID4gUGxlYXNlIHNl
ZSBjb21taXQgNGYzY2M0ODA5YTk4YTE2NWE5NzA4YjcyYjQ3ZGU3MTY0Mzc5N2JiZCAoTkZTdjQ6
IEZpeA0KPiA+IGJyYWluZmFydCBpbiBhdHRyaWJ1dGUgbGVuZ3RoIGNhbGN1bGF0aW9uKSB1cHN0
cmVhbS4NCj4gDQo+IE9oLCBnb29kLCB0aGFua3MhDQo+IA0KPiBCdXQgSSBzdGlsbCBkb24ndCB1
bmRlcnN0YW5kIGhvdyB0aGlzIG1hZGUgaXQgaW50byB1cHN0cmVhbSBpbiB0aGUgZmlyc3QNCj4g
cGxhY2UuDQo+IA0KPiBBbnkgTkZTdjQgdGVzdGluZyBhdCBhbGwgYWdhaW5zdCB0aGUgTGludXgg
c2VydmVyIHdvdWxkIGNhdGNoIHRoaXMsIGFuZA0KPiBJIHRob3VnaHQgeW91IGhhZCBhdXRvbWF0
ZWQgdGVzdGluZyBzZXQgdXAgdGhhdCB5b3UgY291bGQgcnVuIGJlZm9yZQ0KPiBzdWJtaXNzaW9u
Lg0KDQpZZXMsIGhvd2V2ZXIgaXQgd2FzIHRydWx5IGEgYnJhaW5mYXJ0OiB0aGUgcGF0Y2ggd2Fz
IG5ldmVyIHRlc3RlZA0KaW5kZXBlbmRlbnRseSBvZiB0aGUgY2xlYW51cCB3aGljaCByZW1vdmVz
IHRoZSBiYWNrZmlsbGluZyBhbHRvZ2V0aGVyLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGlu
dXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFw
cC5jb20NCnd3dy5uZXRhcHAuY29tDQo=
On Wed, Jul 24, 2013 at 02:30:58PM +0000, Myklebust, Trond wrote:
> On Wed, 2013-07-24 at 10:14 -0400, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <[email protected]>
> >
> > The calculation of attribute length fields is too high by four because
> > it incorrectly includes the length field itself.
> >
> > This regression was introduced by
> > b4a2cf76ab7c08628c62b2062dacefa496b59dfd "NFSv4: Fix a regression
> > against the FreeBSD server" and causes OPENs to the Linux NFS server to
> > fail with BADXDR errors (translated by the client into EIO).
> >
> > Signed-off-by: J. Bruce Fields <[email protected]>
> > ---
> > fs/nfs/nfs4xdr.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > I thought you guys had automated testing against the Linux server? How
> > did this slip through into upstream?
> >
> > --b.
> >
> > diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> > index c74d616..d6d6754 100644
> > --- a/fs/nfs/nfs4xdr.c
> > +++ b/fs/nfs/nfs4xdr.c
> > @@ -1118,7 +1118,7 @@ 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);
> > + len = (char *)p - (char *)q - (bmval_len + 1 << 2);
> > *q++ = htonl(bmval0);
> > *q++ = htonl(bmval1);
> > if (bmval_len == 3)
>
> Please see commit 4f3cc4809a98a165a9708b72b47de71643797bbd (NFSv4: Fix
> brainfart in attribute length calculation) upstream.
Oh, good, thanks!
But I still don't understand how this made it into upstream in the first
place.
Any NFSv4 testing at all against the Linux server would catch this, and
I thought you had automated testing set up that you could run before
submission.
--b.