nfs4_update_server unconditionally releases the nfs_client for the
source server. If migration fails, this can cause the source server's
nfs_client struct to be left with a low reference count, resulting in
use-after-free.
NFS: state manager: migration failed on NFSv4 server nfsvmu10 with error 6
WARNING: CPU: 16 PID: 17960 at fs/nfs/client.c:281
nfs_put_client+0xfa/0x110 [nfs]()
nfs_put_client+0xfa/0x110 [nfs]
nfs4_run_state_manager+0x30/0x40 [nfsv4]
kthread+0xd8/0xf0
BUG: unable to handle kernel NULL pointer dereference at 00000000000002a8
nfs4_xdr_enc_write+0x6b/0x160 [nfsv4]
rpcauth_wrap_req+0xac/0xf0 [sunrpc]
call_transmit+0x18c/0x2c0 [sunrpc]
__rpc_execute+0xa6/0x490 [sunrpc]
rpc_async_schedule+0x15/0x20 [sunrpc]
process_one_work+0x160/0x470
worker_thread+0x112/0x540
kthread+0xd8/0xf0
Reported-by: Helen Chao <[email protected]>
Fixes: 32e62b7c3ef0 ("NFS: Add nfs4_update_server")
Signed-off-by: Bill Baker <[email protected]>
Reviewed-by: Chuck Lever <[email protected]>
---
fs/nfs/nfs4client.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 65a7e5d..c818cdd 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -1226,11 +1226,11 @@ int nfs4_update_server(struct nfs_server
*server, const char *hostname,
clp->cl_proto, clnt->cl_timeout,
clp->cl_minorversion, net);
clear_bit(NFS_MIG_TSM_POSSIBLE, &server->mig_status);
- nfs_put_client(clp);
if (error != 0) {
nfs_server_insert_lists(server);
return error;
}
+ nfs_put_client(clp);
if (server->nfs_client->cl_hostname == NULL)
server->nfs_client->cl_hostname = kstrdup(hostname,
GFP_KERNEL);
--
1.8.3.1
--
Bill Baker - Oracle Linux NFS
SGkgQmlsbCwNCg0KT24gV2VkLCAyMDE4LTAyLTA3IGF0IDE0OjUzIC0wNjAwLCBCaWxsIEJha2Vy
IHdyb3RlOg0KPiBuZnM0X3VwZGF0ZV9zZXJ2ZXIgdW5jb25kaXRpb25hbGx5IHJlbGVhc2VzIHRo
ZSBuZnNfY2xpZW50IGZvciB0aGUNCj4gc291cmNlIHNlcnZlci4gSWYgbWlncmF0aW9uIGZhaWxz
LCB0aGlzIGNhbiBjYXVzZSB0aGUgc291cmNlIHNlcnZlcidzDQo+IG5mc19jbGllbnQgc3RydWN0
IHRvIGJlIGxlZnQgd2l0aCBhIGxvdyByZWZlcmVuY2UgY291bnQsIHJlc3VsdGluZyBpbg0KPiB1
c2UtYWZ0ZXItZnJlZS4NCj4gDQo+IE5GUzogc3RhdGUgbWFuYWdlcjogbWlncmF0aW9uIGZhaWxl
ZCBvbiBORlN2NCBzZXJ2ZXIgbmZzdm11MTAgd2l0aA0KPiBlcnJvciA2DQo+IFdBUk5JTkc6IENQ
VTogMTYgUElEOiAxNzk2MCBhdCBmcy9uZnMvY2xpZW50LmM6MjgxIA0KPiBuZnNfcHV0X2NsaWVu
dCsweGZhLzB4MTEwIFtuZnNdKCkNCj4gICAgICAgICAgbmZzX3B1dF9jbGllbnQrMHhmYS8weDEx
MCBbbmZzXQ0KPiAgICAgICAgICBuZnM0X3J1bl9zdGF0ZV9tYW5hZ2VyKzB4MzAvMHg0MCBbbmZz
djRdDQo+ICAgICAgICAgIGt0aHJlYWQrMHhkOC8weGYwDQo+IA0KPiBCVUc6IHVuYWJsZSB0byBo
YW5kbGUga2VybmVsIE5VTEwgcG9pbnRlciBkZXJlZmVyZW5jZSBhdA0KPiAwMDAwMDAwMDAwMDAw
MmE4DQo+ICAgICAgICAgIG5mczRfeGRyX2VuY193cml0ZSsweDZiLzB4MTYwIFtuZnN2NF0NCj4g
ICAgICAgICAgcnBjYXV0aF93cmFwX3JlcSsweGFjLzB4ZjAgW3N1bnJwY10NCj4gICAgICAgICAg
Y2FsbF90cmFuc21pdCsweDE4Yy8weDJjMCBbc3VucnBjXQ0KPiAgICAgICAgICBfX3JwY19leGVj
dXRlKzB4YTYvMHg0OTAgW3N1bnJwY10NCj4gICAgICAgICAgcnBjX2FzeW5jX3NjaGVkdWxlKzB4
MTUvMHgyMCBbc3VucnBjXQ0KPiAgICAgICAgICBwcm9jZXNzX29uZV93b3JrKzB4MTYwLzB4NDcw
DQo+ICAgICAgICAgIHdvcmtlcl90aHJlYWQrMHgxMTIvMHg1NDANCj4gICAgICAgICAga3RocmVh
ZCsweGQ4LzB4ZjANCj4gDQo+IFJlcG9ydGVkLWJ5OiBIZWxlbiBDaGFvIDxoZWxlbi5jaGFvQG9y
YWNsZS5jb20+DQo+IEZpeGVzOiAzMmU2MmI3YzNlZjAgKCJORlM6IEFkZCBuZnM0X3VwZGF0ZV9z
ZXJ2ZXIiKQ0KPiBTaWduZWQtb2ZmLWJ5OiBCaWxsIEJha2VyIDxiaWxsLmJha2VyQG9yYWNsZS5j
b20+DQo+IFJldmlld2VkLWJ5OiBDaHVjayBMZXZlciA8Y2h1Y2subGV2ZXJAb3JhY2xlLmNvbT4N
Cj4gLS0tDQo+ICAgZnMvbmZzL25mczRjbGllbnQuYyB8IDIgKy0NCj4gICAxIGZpbGUgY2hhbmdl
ZCwgMSBpbnNlcnRpb24oKyksIDEgZGVsZXRpb24oLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9u
ZnMvbmZzNGNsaWVudC5jIGIvZnMvbmZzL25mczRjbGllbnQuYw0KPiBpbmRleCA2NWE3ZTVkLi5j
ODE4Y2RkIDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvbmZzNGNsaWVudC5jDQo+ICsrKyBiL2ZzL25m
cy9uZnM0Y2xpZW50LmMNCj4gQEAgLTEyMjYsMTEgKzEyMjYsMTEgQEAgaW50IG5mczRfdXBkYXRl
X3NlcnZlcihzdHJ1Y3QgbmZzX3NlcnZlciANCj4gKnNlcnZlciwgY29uc3QgY2hhciAqaG9zdG5h
bWUsDQo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNscC0+Y2xfcHJvdG8sIGNs
bnQtPmNsX3RpbWVvdXQsDQo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGNscC0+
Y2xfbWlub3J2ZXJzaW9uLCBuZXQpOw0KPiAgICAgICAgICBjbGVhcl9iaXQoTkZTX01JR19UU01f
UE9TU0lCTEUsICZzZXJ2ZXItPm1pZ19zdGF0dXMpOw0KPiAtICAgICAgIG5mc19wdXRfY2xpZW50
KGNscCk7DQo+ICAgICAgICAgIGlmIChlcnJvciAhPSAwKSB7DQo+ICAgICAgICAgICAgICAgICAg
bmZzX3NlcnZlcl9pbnNlcnRfbGlzdHMoc2VydmVyKTsNCj4gICAgICAgICAgICAgICAgICByZXR1
cm4gZXJyb3I7DQo+ICAgICAgICAgIH0NCj4gKyAgICAgICBuZnNfcHV0X2NsaWVudChjbHApOw0K
PiANCj4gICAgICAgICAgaWYgKHNlcnZlci0+bmZzX2NsaWVudC0+Y2xfaG9zdG5hbWUgPT0gTlVM
TCkNCj4gICAgICAgICAgICAgICAgICBzZXJ2ZXItPm5mc19jbGllbnQtPmNsX2hvc3RuYW1lID0g
a3N0cmR1cChob3N0bmFtZSwgDQo+IEdGUF9LRVJORUwpOw0KPiANCg0KVGhhdCBsb29rcyBhbG1v
c3QgcmlnaHQuIElzbid0IHRoZXJlIG5vdyBhIHJlZmVyZW5jZSBsZWFrIGlmDQpuZnM0X3NldF9j
bGllbnQoKSByZXR1cm5zIC1FTE9PUD8gSSB0aGluayB0aGF0IGlzIHJlYWxseSBkb3duIHRvIGFu
DQpleGlzdGluZyBidWcgaW4gbmZzNF9zZXRfY2xpZW50KCkgcmF0aGVyIHRoYW4gaW4geW91ciBm
aXgsIGhvd2V2ZXIgdGhlDQpmaXggZG9lcyByZS1leHBvc2UgdGhhdCBidWcuDQoNCkNoZWVycw0K
ICBUcm9uZA0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFp
bmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K
T24gV2VkLCAyMDE4LTAyLTA3IGF0IDE2OjA3IC0wNTAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+IEhpIEJpbGwsDQo+IA0KPiBPbiBXZWQsIDIwMTgtMDItMDcgYXQgMTQ6NTMgLTA2MDAsIEJp
bGwgQmFrZXIgd3JvdGU6DQo+ID4gbmZzNF91cGRhdGVfc2VydmVyIHVuY29uZGl0aW9uYWxseSBy
ZWxlYXNlcyB0aGUgbmZzX2NsaWVudCBmb3IgdGhlDQo+ID4gc291cmNlIHNlcnZlci4gSWYgbWln
cmF0aW9uIGZhaWxzLCB0aGlzIGNhbiBjYXVzZSB0aGUgc291cmNlDQo+ID4gc2VydmVyJ3MNCj4g
PiBuZnNfY2xpZW50IHN0cnVjdCB0byBiZSBsZWZ0IHdpdGggYSBsb3cgcmVmZXJlbmNlIGNvdW50
LCByZXN1bHRpbmcNCj4gPiBpbg0KPiA+IHVzZS1hZnRlci1mcmVlLg0KPiA+IA0KPiA+IE5GUzog
c3RhdGUgbWFuYWdlcjogbWlncmF0aW9uIGZhaWxlZCBvbiBORlN2NCBzZXJ2ZXIgbmZzdm11MTAg
d2l0aA0KPiA+IGVycm9yIDYNCj4gPiBXQVJOSU5HOiBDUFU6IDE2IFBJRDogMTc5NjAgYXQgZnMv
bmZzL2NsaWVudC5jOjI4MSANCj4gPiBuZnNfcHV0X2NsaWVudCsweGZhLzB4MTEwIFtuZnNdKCkN
Cj4gPiAgICAgICAgICBuZnNfcHV0X2NsaWVudCsweGZhLzB4MTEwIFtuZnNdDQo+ID4gICAgICAg
ICAgbmZzNF9ydW5fc3RhdGVfbWFuYWdlcisweDMwLzB4NDAgW25mc3Y0XQ0KPiA+ICAgICAgICAg
IGt0aHJlYWQrMHhkOC8weGYwDQo+ID4gDQo+ID4gQlVHOiB1bmFibGUgdG8gaGFuZGxlIGtlcm5l
bCBOVUxMIHBvaW50ZXIgZGVyZWZlcmVuY2UgYXQNCj4gPiAwMDAwMDAwMDAwMDAwMmE4DQo+ID4g
ICAgICAgICAgbmZzNF94ZHJfZW5jX3dyaXRlKzB4NmIvMHgxNjAgW25mc3Y0XQ0KPiA+ICAgICAg
ICAgIHJwY2F1dGhfd3JhcF9yZXErMHhhYy8weGYwIFtzdW5ycGNdDQo+ID4gICAgICAgICAgY2Fs
bF90cmFuc21pdCsweDE4Yy8weDJjMCBbc3VucnBjXQ0KPiA+ICAgICAgICAgIF9fcnBjX2V4ZWN1
dGUrMHhhNi8weDQ5MCBbc3VucnBjXQ0KPiA+ICAgICAgICAgIHJwY19hc3luY19zY2hlZHVsZSsw
eDE1LzB4MjAgW3N1bnJwY10NCj4gPiAgICAgICAgICBwcm9jZXNzX29uZV93b3JrKzB4MTYwLzB4
NDcwDQo+ID4gICAgICAgICAgd29ya2VyX3RocmVhZCsweDExMi8weDU0MA0KPiA+ICAgICAgICAg
IGt0aHJlYWQrMHhkOC8weGYwDQo+ID4gDQo+ID4gUmVwb3J0ZWQtYnk6IEhlbGVuIENoYW8gPGhl
bGVuLmNoYW9Ab3JhY2xlLmNvbT4NCj4gPiBGaXhlczogMzJlNjJiN2MzZWYwICgiTkZTOiBBZGQg
bmZzNF91cGRhdGVfc2VydmVyIikNCj4gPiBTaWduZWQtb2ZmLWJ5OiBCaWxsIEJha2VyIDxiaWxs
LmJha2VyQG9yYWNsZS5jb20+DQo+ID4gUmV2aWV3ZWQtYnk6IENodWNrIExldmVyIDxjaHVjay5s
ZXZlckBvcmFjbGUuY29tPg0KPiA+IC0tLQ0KPiA+ICAgZnMvbmZzL25mczRjbGllbnQuYyB8IDIg
Ky0NCj4gPiAgIDEgZmlsZSBjaGFuZ2VkLCAxIGluc2VydGlvbigrKSwgMSBkZWxldGlvbigtKQ0K
PiA+IA0KPiA+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNGNsaWVudC5jIGIvZnMvbmZzL25mczRj
bGllbnQuYw0KPiA+IGluZGV4IDY1YTdlNWQuLmM4MThjZGQgMTAwNjQ0DQo+ID4gLS0tIGEvZnMv
bmZzL25mczRjbGllbnQuYw0KPiA+ICsrKyBiL2ZzL25mcy9uZnM0Y2xpZW50LmMNCj4gPiBAQCAt
MTIyNiwxMSArMTIyNiwxMSBAQCBpbnQgbmZzNF91cGRhdGVfc2VydmVyKHN0cnVjdCBuZnNfc2Vy
dmVyIA0KPiA+ICpzZXJ2ZXIsIGNvbnN0IGNoYXIgKmhvc3RuYW1lLA0KPiA+ICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgIGNscC0+Y2xfcHJvdG8sIGNsbnQtPmNsX3RpbWVvdXQsDQo+
ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgY2xwLT5jbF9taW5vcnZlcnNpb24s
IG5ldCk7DQo+ID4gICAgICAgICAgY2xlYXJfYml0KE5GU19NSUdfVFNNX1BPU1NJQkxFLCAmc2Vy
dmVyLT5taWdfc3RhdHVzKTsNCj4gPiAtICAgICAgIG5mc19wdXRfY2xpZW50KGNscCk7DQo+ID4g
ICAgICAgICAgaWYgKGVycm9yICE9IDApIHsNCj4gPiAgICAgICAgICAgICAgICAgIG5mc19zZXJ2
ZXJfaW5zZXJ0X2xpc3RzKHNlcnZlcik7DQo+ID4gICAgICAgICAgICAgICAgICByZXR1cm4gZXJy
b3I7DQo+ID4gICAgICAgICAgfQ0KPiA+ICsgICAgICAgbmZzX3B1dF9jbGllbnQoY2xwKTsNCj4g
PiANCj4gPiAgICAgICAgICBpZiAoc2VydmVyLT5uZnNfY2xpZW50LT5jbF9ob3N0bmFtZSA9PSBO
VUxMKQ0KPiA+ICAgICAgICAgICAgICAgICAgc2VydmVyLT5uZnNfY2xpZW50LT5jbF9ob3N0bmFt
ZSA9DQo+ID4ga3N0cmR1cChob3N0bmFtZSwgDQo+ID4gR0ZQX0tFUk5FTCk7DQo+ID4gDQo+IA0K
PiBUaGF0IGxvb2tzIGFsbW9zdCByaWdodC4gSXNuJ3QgdGhlcmUgbm93IGEgcmVmZXJlbmNlIGxl
YWsgaWYNCj4gbmZzNF9zZXRfY2xpZW50KCkgcmV0dXJucyAtRUxPT1A/IEkgdGhpbmsgdGhhdCBp
cyByZWFsbHkgZG93biB0byBhbg0KPiBleGlzdGluZyBidWcgaW4gbmZzNF9zZXRfY2xpZW50KCkg
cmF0aGVyIHRoYW4gaW4geW91ciBmaXgsIGhvd2V2ZXIgDQoNClNvcnJ5Li4uIEkgc2hvdWxkIHBy
b2JhYmx5IGhhdmUgYmVlbiBtb3JlIGV4cGxpY2l0IGFib3V0IHdoZXJlIHRoZSBidWcNCmlzOiBu
ZnM0X3NldF9jbGllbnQoKSBzaG91bGQgcHJvYmFibHkgYmUgY2FsbGluZyBuZnNfcHV0X2NsaWVu
dCgpIG9uDQppdHMgY29weSBvZiAnY2xwJyBiZWZvcmUgcmV0dXJuaW5nIC1FTE9PUC4NCg0KPiB0
aGUNCj4gZml4IGRvZXMgcmUtZXhwb3NlIHRoYXQgYnVnLg0KDQpCeSB3aGljaCBJIG1lYW4gdGhh
dCB0aGUgdHdvIGJ1Z3MgY3VycmVudGx5IGNhbmNlbCBlYWNoIG90aGVyIG91dCBmb3INCnRoZSBj
YXNlIG9mIEVMT09QLiBCeSBmaXhpbmcgb25lIGJ1ZywgYnV0IG5vdCB0aGUgb3RoZXIsIHdlJ3Jl
IHVuZG9pbmcNCnRoZSBjYW5jZWxsYXRpb24uLi4g4pi6DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0
DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIsIFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1
c3RAcHJpbWFyeWRhdGEuY29tDQo=
On 02/07/2018 03:20 PM, Trond Myklebust wrote:
> On Wed, 2018-02-07 at 16:07 -0500, Trond Myklebust wrote:
>> Hi Bill,
>>
>> On Wed, 2018-02-07 at 14:53 -0600, Bill Baker wrote:
>>> nfs4_update_server unconditionally releases the nfs_client for the
>>> source server. If migration fails, this can cause the source
>>> server's
>>> nfs_client struct to be left with a low reference count, resulting
>>> in
>>> use-after-free.
>>>
<snip>
>> That looks almost right. Isn't there now a reference leak if
>> nfs4_set_client() returns -ELOOP? I think that is really down to an
>> existing bug in nfs4_set_client() rather than in your fix, however
>
> Sorry... I should probably have been more explicit about where the bug
> is: nfs4_set_client() should probably be calling nfs_put_client() on
> its copy of 'clp' before returning -ELOOP.
>
>> the
>> fix does re-expose that bug.
>
> By which I mean that the two bugs currently cancel each other out for
> the case of ELOOP. By fixing one bug, but not the other, we're undoing
> the cancellation... ☺
Hi Trond,
Thank you for your quick feedback, let me go do some more inspection.
--
Bill Baker - Oracle Linux NFS
On 02/07/2018 03:20 PM, Trond Myklebust wrote:
> On Wed, 2018-02-07 at 16:07 -0500, Trond Myklebust wrote:
>> Hi Bill,
>>
>> On Wed, 2018-02-07 at 14:53 -0600, Bill Baker wrote:
>>> nfs4_update_server unconditionally releases the nfs_client for the
>>> source server. If migration fails, this can cause the source
>>> server's
>>> nfs_client struct to be left with a low reference count, resulting
>>> in
>>> use-after-free.
>>>
>>> NFS: state manager: migration failed on NFSv4 server nfsvmu10 with
>>> error 6
>>> WARNING: CPU: 16 PID: 17960 at fs/nfs/client.c:281
>>> nfs_put_client+0xfa/0x110 [nfs]()
>>> nfs_put_client+0xfa/0x110 [nfs]
>>> nfs4_run_state_manager+0x30/0x40 [nfsv4]
>>> kthread+0xd8/0xf0
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at
>>> 00000000000002a8
>>> nfs4_xdr_enc_write+0x6b/0x160 [nfsv4]
>>> rpcauth_wrap_req+0xac/0xf0 [sunrpc]
>>> call_transmit+0x18c/0x2c0 [sunrpc]
>>> __rpc_execute+0xa6/0x490 [sunrpc]
>>> rpc_async_schedule+0x15/0x20 [sunrpc]
>>> process_one_work+0x160/0x470
>>> worker_thread+0x112/0x540
>>> kthread+0xd8/0xf0
>>>
>>> Reported-by: Helen Chao <[email protected]>
>>> Fixes: 32e62b7c3ef0 ("NFS: Add nfs4_update_server")
>>> Signed-off-by: Bill Baker <[email protected]>
>>> Reviewed-by: Chuck Lever <[email protected]>
>>> ---
>>> fs/nfs/nfs4client.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>>> index 65a7e5d..c818cdd 100644
>>> --- a/fs/nfs/nfs4client.c
>>> +++ b/fs/nfs/nfs4client.c
>>> @@ -1226,11 +1226,11 @@ int nfs4_update_server(struct nfs_server
>>> *server, const char *hostname,
>>> clp->cl_proto, clnt->cl_timeout,
>>> clp->cl_minorversion, net);
>>> clear_bit(NFS_MIG_TSM_POSSIBLE, &server->mig_status);
>>> - nfs_put_client(clp);
>>> if (error != 0) {
>>> nfs_server_insert_lists(server);
>>> return error;
>>> }
>>> + nfs_put_client(clp);
>>>
>>> if (server->nfs_client->cl_hostname == NULL)
>>> server->nfs_client->cl_hostname =
>>> kstrdup(hostname,
>>> GFP_KERNEL);
>>>
>>
>> That looks almost right. Isn't there now a reference leak if
>> nfs4_set_client() returns -ELOOP? I think that is really down to an
>> existing bug in nfs4_set_client() rather than in your fix, however
>
> Sorry... I should probably have been more explicit about where the bug
> is: nfs4_set_client() should probably be calling nfs_put_client() on
> its copy of 'clp' before returning -ELOOP.
>
>> the
>> fix does re-expose that bug.
>
> By which I mean that the two bugs currently cancel each other out for
> the case of ELOOP. By fixing one bug, but not the other, we're undoing
> the cancellation... ☺
>
Ah, yes, I see.
if (server->nfs_client == clp) {
nfs_put_client(clp);
return -ELOOP;
}
I'll send an updated patch shortly. Thanks for your feedback.
--
Bill Baker - Oracle Linux NFS