* [PATCH] NFSv4: Fix attribute length @ 2013-07-24 14:14 J. Bruce Fields 2013-07-24 14:30 ` Myklebust, Trond 0 siblings, 1 reply; 5+ messages in thread From: J. Bruce Fields @ 2013-07-24 14:14 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs From: "J. Bruce Fields" <bfields@redhat.com> 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 <bfields@redhat.com> --- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSv4: Fix attribute length 2013-07-24 14:14 [PATCH] NFSv4: Fix attribute length J. Bruce Fields @ 2013-07-24 14:30 ` Myklebust, Trond 2013-07-24 14:38 ` J. Bruce Fields 0 siblings, 1 reply; 5+ messages in thread From: Myklebust, Trond @ 2013-07-24 14:30 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSv4: Fix attribute length 2013-07-24 14:30 ` Myklebust, Trond @ 2013-07-24 14:38 ` J. Bruce Fields 2013-07-24 14:41 ` Myklebust, Trond 0 siblings, 1 reply; 5+ messages in thread From: J. Bruce Fields @ 2013-07-24 14:38 UTC (permalink / raw) To: Myklebust, Trond; +Cc: linux-nfs@vger.kernel.org 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" <bfields@redhat.com> > > > > 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 <bfields@redhat.com> > > --- > > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSv4: Fix attribute length 2013-07-24 14:38 ` J. Bruce Fields @ 2013-07-24 14:41 ` Myklebust, Trond 2013-07-24 14:44 ` J. Bruce Fields 0 siblings, 1 reply; 5+ messages in thread From: Myklebust, Trond @ 2013-07-24 14:41 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs@vger.kernel.org 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= ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] NFSv4: Fix attribute length 2013-07-24 14:41 ` Myklebust, Trond @ 2013-07-24 14:44 ` J. Bruce Fields 0 siblings, 0 replies; 5+ messages in thread From: J. Bruce Fields @ 2013-07-24 14:44 UTC (permalink / raw) To: Myklebust, Trond; +Cc: linux-nfs@vger.kernel.org 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" <bfields@redhat.com> > > > > > > > > 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 <bfields@redhat.com> > > > > --- > > > > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-24 14:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-24 14:14 [PATCH] NFSv4: Fix attribute length J. Bruce Fields 2013-07-24 14:30 ` Myklebust, Trond 2013-07-24 14:38 ` J. Bruce Fields 2013-07-24 14:41 ` Myklebust, Trond 2013-07-24 14:44 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).