diff mbox

[GIT,PULL] Please pull NFS client changes for Linux 4.13

Message ID CA+55aFyQdHt0qYQ0uBSK+9wfPOyFfx8mJE4r9FfWoxa3Gyei9g@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Linus Torvalds Aug. 1, 2017, 5:20 p.m. UTC 
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?

So caveat probatorem,

              Linus
fs/nfs/nfs4proc.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Trond Myklebust Aug. 1, 2017, 5:30 p.m. UTC | #1 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Jones Aug. 1, 2017, 5:50 p.m. UTC | #2 
On Tue, Aug 01, 2017 at 10:20:31AM -0700, Linus Torvalds wrote:

 > 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?

No idea why it only just showed up, but it isn't 100% reproducable
either.  A month or so ago I did disable the V4 code on the server
completely (as I was using v3 everywhere else), so maybe I started hitting
a fallback path somewhere.  *shrug*

	Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds Aug. 1, 2017, 5:53 p.m. UTC | #3 
On Tue, Aug 1, 2017 at 10:20 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

I think the "something like that" is actually just reading the
cdata->args.verifier->data pointer itself, and it *is* the stack
access - but the stack page has been free'd (because of the same fatal
signal that interrupted the rpc_wait_for_completion_task() call), and
then re-allocated (and free'd again) as a pathname page.

Maybe.

Regardless, my patch still looks conceptually correct, even if it
might have bugs due to total lack of testing.

                    Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust Aug. 1, 2017, 5:58 p.m. UTC | #4 
On Tue, 2017-08-01 at 13:50 -0400, davej@codemonkey.org.uk wrote:
> On Tue, Aug 01, 2017 at 10:20:31AM -0700, Linus Torvalds wrote:

> 

>  > 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?

> 

> No idea why it only just showed up, but it isn't 100% reproducable

> either.  A month or so ago I did disable the V4 code on the server

> completely (as I was using v3 everywhere else), so maybe I started

> hitting

> a fallback path somewhere.  *shrug*

> 


I would only expect you too see it if you interrupt the wait on the
asynchronous EXCHANGE_ID call (which would allow the RPC call to
continue while the caller stack is trashed). Prior to commit
8d89bd70bc939, that code path was fully synchronous, so there was no
issue with interrupting the call.

-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 18ca6879d8de..0712af3d38f8 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7490,6 +7490,11 @@  static const struct rpc_call_ops nfs4_exchange_id_call_ops = {
 	.rpc_release = nfs4_exchange_id_release,
 };
 
+struct verifier_and_calldata {
+	struct nfs41_exchange_id_data calldata;
+	nfs4_verifier verifier;
+};
+
 /*
  * _nfs4_proc_exchange_id()
  *
@@ -7498,7 +7503,8 @@  static const struct rpc_call_ops nfs4_exchange_id_call_ops = {
 static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 			u32 sp4_how, struct rpc_xprt *xprt)
 {
-	nfs4_verifier verifier;
+	struct verifier_and_calldata *vna;
+	nfs4_verifier *verifier;
 	struct rpc_message msg = {
 		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_EXCHANGE_ID],
 		.rpc_cred = cred,
@@ -7516,14 +7522,17 @@  static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 	if (!atomic_inc_not_zero(&clp->cl_count))
 		return -EIO;
 
-	calldata = kzalloc(sizeof(*calldata), GFP_NOFS);
-	if (!calldata) {
+	vna = kzalloc(sizeof(*vna), GFP_NOFS);
+	if (!vna) {
 		nfs_put_client(clp);
 		return -ENOMEM;
 	}
+	/* kfree() of calldata will also free the verifier */
+	calldata = &vna->calldata;
+	verifier = &vna->verifier;
 
 	if (!xprt)
-		nfs4_init_boot_verifier(clp, &verifier);
+		nfs4_init_boot_verifier(clp, verifier);
 
 	status = nfs4_init_uniform_client_string(clp);
 	if (status)
@@ -7566,7 +7575,7 @@  static int _nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred,
 				RPC_TASK_SOFT|RPC_TASK_SOFTCONN|RPC_TASK_ASYNC;
 		calldata->args.verifier = &clp->cl_confirm;
 	} else {
-		calldata->args.verifier = &verifier;
+		calldata->args.verifier = verifier;
 	}
 	calldata->args.client = clp;
 #ifdef CONFIG_NFS_V4_1_MIGRATION