From: Trond Myklebust <trondmy@primarydata.com>
To: "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"bfields@fieldses.org" <bfields@fieldses.org>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"schumaker.anna@gmail.com" <schumaker.anna@gmail.com>,
"davej@codemonkey.org.uk" <davej@codemonkey.org.uk>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13
Date: Tue, 1 Aug 2017 17:30:30 +0000 [thread overview]
Message-ID: <1501608628.70813.1.camel@primarydata.com> (raw)
In-Reply-To: <CA+55aFyQdHt0qYQ0uBSK+9wfPOyFfx8mJE4r9FfWoxa3Gyei9g@mail.gmail.com>
T24gVHVlLCAyMDE3LTA4LTAxIGF0IDEwOjIwIC0wNzAwLCBMaW51cyBUb3J2YWxkcyB3cm90ZToN
Cj4gT24gVHVlLCBBdWcgMSwgMjAxNyBhdCA4OjUxIEFNLCBkYXZlakBjb2RlbW9ua2V5Lm9yZy51
aw0KPiA8ZGF2ZWpAY29kZW1vbmtleS5vcmcudWs+IHdyb3RlOg0KPiA+IE9uIE1vbiwgSnVsIDMx
LCAyMDE3IGF0IDEwOjM1OjQ1UE0gLTA3MDAsIExpbnVzIFRvcnZhbGRzIHdyb3RlOg0KPiA+ICA+
IEFueSBjaGFuY2Ugb2YgZ2V0dGluZyB0aGUgb3V0cHV0IGZyb20NCj4gPiAgPg0KPiA+ICA+ICAg
IC4vc2NyaXB0cy9mYWRkcjJsaW5lIHZtbGludXgNCj4gPiBuZnM0X2V4Y2hhbmdlX2lkX2RvbmUr
MHgzZDcvMHg4ZTANCj4gPiANCj4gPiANCj4gPiBIbSwgdGhhdCBwb2ludHMgdG8gdGhpcy4uDQo+
ID4gDQo+ID4gNzQ2MyAgICAgICAgICAgICAgICAgLyogU2F2ZSB0aGUgRVhDSEFOR0VfSUQgdmVy
aWZpZXIgc2Vzc2lvbiB0cnVuaw0KPiA+IHRlc3RzICovDQo+ID4gNzQ2NCAgICAgICAgICAgICAg
ICAgbWVtY3B5KGNscC0+Y2xfY29uZmlybS5kYXRhLCBjZGF0YS0NCj4gPiA+YXJncy52ZXJpZmll
ci0+ZGF0YSwNCj4gPiA3NDY1ICAgICAgICAgICAgICAgICAgICAgICAgc2l6ZW9mKGNscC0+Y2xf
Y29uZmlybS5kYXRhKSk7DQo+IA0KPiBPaywgdGhhdCBjZXJ0YWlubHkgbWFkZSBubyBzZW5zZSB0
byBtZSwgYmVjYXVzZSB0aGUgS0FTQU4gcmVwb3J0IG1hZGUNCj4gaXQgbG9vayBsaWtlIGEgc3Rh
bGUgcGF0aG5hbWUgYWNjZXNzIChhbGxvY2F0ZWQgaW4gZ2V0bmFtZSwgZnJlZWQgaW4NCj4gcHV0
bmFtZSksIGJ1dCBJIHRoaW5rIHRoZSBpc3N1ZSBpcyBtb3JlIGZ1bmRhbWVudGFsIHRoYW4gdGhh
dC4NCj4gDQo+IFRoYXQgY2RhdGEtPmFyZ3MudmVyaWZpZXIgc2VlbXMgdG8gYmUgZW50aXJlbHkg
YnJva2VuLiBBVCBsZWFzdCBmb3INCj4gdGhlICJ4cHJ0ID09IE5VTEwiIGNhc2UsIGl0IGRvZXMg
dGhlIGZvbGxvd2luZzoNCj4gDQo+ICAtIHVzZSB0aGUgYWRkcmVzcyBvZiBhIGxvY2FsIHZhcmlh
YmxlICgiJnZlcmlmaWVyIikNCj4gDQo+ICAtIHdhaXQgZm9yIHRoZSBycGMgY29tcGxldGlvbiB1
c2luZyBycGNfd2FpdF9mb3JfY29tcGxldGlvbl90YXNrKCkuDQo+IA0KPiBUaGF0J3MgdW5hY2Nl
cHRhYmx5IGJ1Z2d5IGNyYXAuIHJwY193YWl0X2Zvcl9jb21wbGV0aW9uX3Rhc2soKSB3aWxsDQo+
IGhhcHBpbHkgZXhpdCBvbiBhIGRlYWRseSBzaWduYWwgZXZlbiBpZiB0aGUgcnBjIGhhc24ndCBi
ZWVuDQo+IGNvbXBsZXRlZCwNCj4gc28gbm93IHlvdSdsbCBoYXZlIGEgc3RhbGUgcG9pbnRlciB0
byBhIHN0YWNrIHRoYXQgaGFzIGJlZW4gZnJlZWQuDQo+IA0KPiBTbyBJIHRoaW5rIHRoZSAncGF0
aG5hbWUnIHBhcnQgbWF5IGFjdHVhbGx5IGJlIGVudGlyZWx5IGEgcmVkDQo+IGhlcnJpbmcsDQo+
IGFuZCBpdCdzIHRoZSB1bmRlcmx5aW5nIGFjY2VzcyBpdHNlbGYgdGhhdCBqdXN0IHBpY2tzIHVw
IGEgcmFuZG9tDQo+IHBvaW50ZXIgZnJvbSBhIHN0YWNrIHRoYXQgbm93IGNvbnRhaW5zIHNvbWV0
aGluZyBkaWZmZXJlbnQuIEFuZCBLQVNBTg0KPiBkaWRuJ3Qgbm90aWNlIHRoZSBzdGFsZSBzdGFj
ayBhY2Nlc3MgaXRzZWxmLCBiZWNhdXNlIHRoZSBzdGFjayBzbG90DQo+IGlzDQo+IHN0aWxsIHZh
bGlkIC0gaXQncyBqdXN0IG5vIGxvbmdlciB0aGUgb3JpZ2luYWwgJ3ZlcmlmaWVyJyBhbGxvY2F0
aW9uLg0KPiANCj4gT3IgKnNvbWV0aGluZyogbGlrZSB0aGF0Lg0KPiANCj4gTm9uZSBvZiB0aGlz
IGxvb2tzIGV2ZW4gcmVtb3RlbHkgbmV3LCB0aG91Z2ggLSB0aGUgY29kZSBzZWVtcyB0byBnbw0K
PiBiYWNrIHRvIDIwMDkuIEhhdmUgeW91IGp1c3QgY2hhbmdlZCB3aGF0IHlvdSdyZSB0ZXN0aW5n
IHRvIHRyaWdnZXINCj4gdGhlc2UgdGhpbmdzPw0KPiANCj4gSSdtIG5vdCBldmVuIHN1cmUgd2h5
IGl0IGRvZXMgdGhhdCBzdHVwaWQgc3RhY2sgYWxsb2NhdGlvbi4gSXQgZG9lcyBhDQo+ICpyZWFs
KiBhbGxvY2F0aW9uIGp1c3QgYSBmZXcgbGluZXMgbGF0ZXI6DQo+IA0KPiAgICAgICAgIHN0cnVj
dCBuZnM0MV9leGNoYW5nZV9pZF9kYXRhICpjYWxsZGF0YQ0KPiAgICAgICAgIC4uLg0KPiAgICAg
ICAgIGNhbGxkYXRhID0ga3phbGxvYyhzaXplb2YoKmNhbGxkYXRhKSwgR0ZQX05PRlMpOw0KPiAN
Cj4gYW5kIHRoZSB3aG9sZSB2ZXJpZmllciBzdHJ1Y3R1cmUgY291bGQgZWFzaWx5IGhhdmUgYmVl
biBwYXJ0IG9mIHRoYXQNCj4gc2FtZSBhbGxvY2F0aW9uIGFzIGZhciBhcyBJIGNhbiB0ZWxsLg0K
PiANCj4gQW5kIHRoYXQgcmVhbGx5IG1pZ2h0IHNlZW0gdG8gYmUgdGhlIHJpZ2h0IHRoaW5nIHRv
IGRvLg0KPiANCj4gVE9UQUxMWSBVTlRFU1RFRCBQUk9CQUJMWSBDT01QTEVURSBDUkFQIHBhdGNo
IGF0dGF0Y2hlZC4NCj4gDQo+IFRoYXQgcGF0Y2ggY29tcGlsZXMgZm9yIG1lLiBJdCAqbWlnaHQq
IGV2ZW4gd29yay4gT3IgaXQgbWlnaHQganVzdCBiZQ0KPiB0aGUgcmFtYmxpbmdzIG9mIGEgZGlz
ZWFzZWQgbWluZC4NCj4gDQo+IEFubmE/IFRyb25kPw0KPiANCg0KSSBjYW1lIHRvIHRoZSBzYW1l
IGNvbmNsdXNpb24geWVzdGVyZGF5LCBhbmQgaGF2ZSBhIHN0YWJsZSBwYXRjaCB0aGF0DQpkb2Vz
IHNvbWV0aGluZyBzaW1pbGFyLiBJIGp1c3QgZ290IGRpc3RyYWN0ZWQgd2l0aCB0aGUgb3RoZXIg
YnVncyB0aGF0DQp3ZXJlIGludHJvZHVjZWQgYnkgdGhlIGV4Y2hhbmdlaWQgcGF0Y2ggc2VyaWVz
IGluIExpbnV4LTQuOSAoaW5jbHVkaW5nDQp3aGF0IGxvb2tzIGxpa2UgYSBkdXBsaWNhdGUgZnJl
ZSBpc3N1ZSBpbiBuZnM0X3Rlc3Rfc2Vzc2lvbl90cnVuaygpKS4NCg0KSSBjYW4gcGFzcyBhIGZl
dyBvZiB0aGUgbW9yZSBjcml0aWNhbCBwYXRjaGVzIG9uIHRvIEFubmEgZm9yIG1lcmdpbmcgaW4N
CnRoaXMgY3ljbGUsIHRoZW4gSSd2ZSBnb3Qgc29tZSBjbGVhbiB1cHMgcmVhZHkgZm9yIHRoZSA0
LjE0IG1lcmdlDQp3aW5kb3cuDQoNCkNoZWVycw0KICBUcm9uZA0KDQotLSANClRyb25kIE15a2xl
YnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlr
bGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K
WARNING: multiple messages have this Message-ID (diff)
From: Trond Myklebust <trondmy@primarydata.com>
To: "torvalds@linux-foundation.org" <torvalds@linux-foundation.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"bfields@fieldses.org" <bfields@fieldses.org>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"schumaker.anna@gmail.com" <schumaker.anna@gmail.com>,
"davej@codemonkey.org.uk" <davej@codemonkey.org.uk>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13
Date: Tue, 1 Aug 2017 17:30:30 +0000 [thread overview]
Message-ID: <1501608628.70813.1.camel@primarydata.com> (raw)
In-Reply-To: <CA+55aFyQdHt0qYQ0uBSK+9wfPOyFfx8mJE4r9FfWoxa3Gyei9g@mail.gmail.com>
On Tue, 2017-08-01 at 10:20 -0700, Linus Torvalds wrote:
> On Tue, Aug 1, 2017 at 8:51 AM, davej@codemonkey.org.uk
> <davej@codemonkey.org.uk> wrote:
> > On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote:
> > > Any chance of getting the output from
> > >
> > > ./scripts/faddr2line vmlinux
> > nfs4_exchange_id_done+0x3d7/0x8e0
> >
> >
> > Hm, that points to this..
> >
> > 7463 /* Save the EXCHANGE_ID verifier session trunk
> > tests */
> > 7464 memcpy(clp->cl_confirm.data, cdata-
> > >args.verifier->data,
> > 7465 sizeof(clp->cl_confirm.data));
>
> Ok, that certainly made no sense to me, because the KASAN report made
> it look like a stale pathname access (allocated in getname, freed in
> putname), but I think the issue is more fundamental than that.
>
> That cdata->args.verifier seems to be entirely broken. AT least for
> the "xprt == NULL" case, it does the following:
>
> - use the address of a local variable ("&verifier")
>
> - wait for the rpc completion using rpc_wait_for_completion_task().
>
> That's unacceptably buggy crap. rpc_wait_for_completion_task() will
> happily exit on a deadly signal even if the rpc hasn't been
> completed,
> so now you'll have a stale pointer to a stack that has been freed.
>
> So I think the 'pathname' part may actually be entirely a red
> herring,
> and it's the underlying access itself that just picks up a random
> pointer from a stack that now contains something different. And KASAN
> didn't notice the stale stack access itself, because the stack slot
> is
> still valid - it's just no longer the original 'verifier' allocation.
>
> Or *something* like that.
>
> None of this looks even remotely new, though - the code seems to go
> back to 2009. Have you just changed what you're testing to trigger
> these things?
>
> I'm not even sure why it does that stupid stack allocation. It does a
> *real* allocation just a few lines later:
>
> struct nfs41_exchange_id_data *calldata
> ...
> calldata = kzalloc(sizeof(*calldata), GFP_NOFS);
>
> and the whole verifier structure could easily have been part of that
> same allocation as far as I can tell.
>
> And that really might seem to be the right thing to do.
>
> TOTALLY UNTESTED PROBABLY COMPLETE CRAP patch attatched.
>
> That patch compiles for me. It *might* even work. Or it might just be
> the ramblings of a diseased mind.
>
> Anna? Trond?
>
I came to the same conclusion yesterday, and have a stable patch that
does something similar. I just got distracted with the other bugs that
were introduced by the exchangeid patch series in Linux-4.9 (including
what looks like a duplicate free issue in nfs4_test_session_trunk()).
I can pass a few of the more critical patches on to Anna for merging in
this cycle, then I've got some clean ups ready for the 4.14 merge
window.
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
next prev parent reply other threads:[~2017-08-01 17:30 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-13 21:16 [GIT PULL] Please pull NFS client changes for Linux 4.13 Anna Schumaker
2017-07-13 21:43 ` Linus Torvalds
2017-07-14 7:09 ` Christoph Hellwig
2017-07-14 11:33 ` Anna Schumaker
2017-07-14 14:25 ` Dave Jones
2017-07-14 16:36 ` J. Bruce Fields
2017-07-14 19:05 ` Linus Torvalds
2017-07-14 19:43 ` Andrey Ryabinin
2017-07-14 19:58 ` Linus Torvalds
2017-07-14 20:26 ` Andrey Rybainin
2017-07-14 20:38 ` Daniel Micay
2017-07-14 20:50 ` Linus Torvalds
2017-07-14 21:01 ` Daniel Micay
2017-07-14 21:05 ` Daniel Micay
2017-07-14 20:50 ` Daniel Micay
2017-07-14 23:59 ` Daniel Micay
2017-07-14 19:48 ` Dave Jones
2017-07-16 21:15 ` Dave Jones
2017-07-16 22:57 ` Trond Myklebust
2017-07-16 22:57 ` Trond Myklebust
2017-07-16 22:57 ` Trond Myklebust
2017-07-17 3:05 ` davej
2017-07-17 19:02 ` Linus Torvalds
2017-07-18 14:20 ` [GIT PULL] Please pull an nfsd bugfix for 4.13 bfields
2017-07-31 15:43 ` [GIT PULL] Please pull NFS client changes for Linux 4.13 davej
2017-08-01 5:35 ` Linus Torvalds
2017-08-01 15:51 ` davej
2017-08-01 17:20 ` Linus Torvalds
2017-08-01 17:30 ` Trond Myklebust [this message]
2017-08-01 17:30 ` Trond Myklebust
2017-08-01 17:50 ` davej
2017-08-01 17:58 ` Trond Myklebust
2017-08-01 17:58 ` Trond Myklebust
2017-08-01 17:53 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1501608628.70813.1.camel@primarydata.com \
--to=trondmy@primarydata.com \
--cc=bfields@fieldses.org \
--cc=davej@codemonkey.org.uk \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=schumaker.anna@gmail.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.