[PATCH] NFS: Clear key construction data if the idmap upcall fails

[PATCH] NFS: Clear key construction data if the idmap upcall fails

[bjschuma]

From: Bryan Schumaker <bjschuma@netapp.com>

idmap_pipe_downcall already clears this field if the upcall succeeds,
but if it fails (rpc.idmapd isn't running) the field will still be set
on the next call triggering a BUG_ON().

Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
---
 fs/nfs/idmap.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
index b701358..645cfe7 100644
--- a/fs/nfs/idmap.c
+++ b/fs/nfs/idmap.c
@@ -683,10 +683,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
 
 	ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
 	if (ret < 0)
-		goto out2;
+		goto out3;
 
 	return ret;
 
+out3:
+	idmap->idmap_key_cons = NULL;
 out2:
 	kfree(im);
 out1:
-- 
1.7.11.4

Re: [PATCH] NFS: Clear key construction data if the idmap upcall fails

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTA4LTA3IGF0IDExOjMwIC0wNDAwLCBianNjaHVtYUBuZXRhcHAuY29tIHdy
b3RlOg0KPiBGcm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+IA0K
PiBpZG1hcF9waXBlX2Rvd25jYWxsIGFscmVhZHkgY2xlYXJzIHRoaXMgZmllbGQgaWYgdGhlIHVw
Y2FsbCBzdWNjZWVkcywNCj4gYnV0IGlmIGl0IGZhaWxzIChycGMuaWRtYXBkIGlzbid0IHJ1bm5p
bmcpIHRoZSBmaWVsZCB3aWxsIHN0aWxsIGJlIHNldA0KPiBvbiB0aGUgbmV4dCBjYWxsIHRyaWdn
ZXJpbmcgYSBCVUdfT04oKS4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IEJyeWFuIFNjaHVtYWtlciA8
YmpzY2h1bWFAbmV0YXBwLmNvbT4NCj4gLS0tDQo+ICBmcy9uZnMvaWRtYXAuYyB8IDE2ICsrKysr
KysrKy0tLS0tLS0NCj4gIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMoKyksIDcgZGVsZXRp
b25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL2lkbWFwLmMgYi9mcy9uZnMvaWRtYXAu
Yw0KPiBpbmRleCBiNzAxMzU4Li42NDVjZmU3IDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvaWRtYXAu
Yw0KPiArKysgYi9mcy9uZnMvaWRtYXAuYw0KPiBAQCAtNjgzLDEwICs2ODMsMTIgQEAgc3RhdGlj
IGludCBuZnNfaWRtYXBfbGVnYWN5X3VwY2FsbChzdHJ1Y3Qga2V5X2NvbnN0cnVjdGlvbiAqY29u
cywNCj4gIA0KPiAgCXJldCA9IHJwY19xdWV1ZV91cGNhbGwoaWRtYXAtPmlkbWFwX3BpcGUsIG1z
Zyk7DQo+ICAJaWYgKHJldCA8IDApDQo+IC0JCWdvdG8gb3V0MjsNCj4gKwkJZ290byBvdXQzOw0K
PiAgDQo+ICAJcmV0dXJuIHJldDsNCj4gIA0KPiArb3V0MzoNCj4gKwlpZG1hcC0+aWRtYXBfa2V5
X2NvbnMgPSBOVUxMOw0KPiAgb3V0MjoNCj4gIAlrZnJlZShpbSk7DQo+ICBvdXQxOg0KDQpUaGF0
IGZpeGVzIHJwY19xdWV1ZV91cGNhbGwgZmFpbHVyZSBwYXRoLCBidXQgd2Ugc3RpbGwgbmVlZCB0
byBkZWFsIHdpdGgNCnRpbWVvdXRzIGFuZCBjbG9zZS4NCg0KSG93IGFib3V0IHRoZSBmb2xsb3dp
bmcgYWx0ZXJuYXRpdmU6DQoNClNldCBpZG1hcC0+aWRtYXBfa2V5X2NvbnMgYWZ0ZXIgZ3JhYmJp
bmcgdGhlIG11dGV4IGluDQpuZnNfaWRtYXBfZ2V0X2tleSgpLCB0aGVuIGNsZWFyIGl0IGJlZm9y
ZSB5b3UgcmVsZWFzZSB0aGF0IG11dGV4LiBUaGF0DQpsZWF2ZXMgb25lIHBvc3NpYmxlIHJhY2Ug
aW4gd2hpY2ggdGhlIGlkbWFwLT5pZG1hcF9rZXlfY29ucyBpcyBjbGVhcmVkDQp3aGlsZSB3ZSdy
ZSBpbiBpZG1hcF9waXBlX2Rvd25jYWxsLiBPbmUgd2F5IHRvIHNvbHZlIHRoYXQgcmFjZSBpcyB0
byB1c2UNCmEgc2Vjb25kIG11dGV4IChzZWUgdGhlIG9yaWdpbmFsIGlkbWFwcGVyIGRlc2lnbiBm
cm9tIGJlZm9yZSB5b3VyDQpjaGFuZ2VzKSB0byBlbnN1cmUgdGhhdCBub3RoaW5nIGNsZWFycyBp
ZG1hcF9rZXlfY29ucyB3aGlsZSB3ZSdyZSBpbg0KaWRtYXBfcGlwZV9kb3duY2FsbC4NCg0KLS0g
DQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHAN
ClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=

Re: [PATCH] NFS: Clear key construction data if the idmap upcall fails

[Bryan Schumaker]

On 08/07/2012 11:44 AM, Myklebust, Trond wrote:
> On Tue, 2012-08-07 at 11:30 -0400, bjschuma@netapp.com wrote:
>> From: Bryan Schumaker <bjschuma@netapp.com>
>>
>> idmap_pipe_downcall already clears this field if the upcall succeeds,
>> but if it fails (rpc.idmapd isn't running) the field will still be set
>> on the next call triggering a BUG_ON().
>>
>> Signed-off-by: Bryan Schumaker <bjschuma@netapp.com>
>> ---
>>  fs/nfs/idmap.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/nfs/idmap.c b/fs/nfs/idmap.c
>> index b701358..645cfe7 100644
>> --- a/fs/nfs/idmap.c
>> +++ b/fs/nfs/idmap.c
>> @@ -683,10 +683,12 @@ static int nfs_idmap_legacy_upcall(struct key_construction *cons,
>>  
>>  	ret = rpc_queue_upcall(idmap->idmap_pipe, msg);
>>  	if (ret < 0)
>> -		goto out2;
>> +		goto out3;
>>  
>>  	return ret;
>>  
>> +out3:
>> +	idmap->idmap_key_cons = NULL;
>>  out2:
>>  	kfree(im);
>>  out1:
> 
> That fixes rpc_queue_upcall failure path, but we still need to deal with
> timeouts and close.
> 
> How about the following alternative:
> 
> Set idmap->idmap_key_cons after grabbing the mutex in
> nfs_idmap_get_key(), then clear it before you release that mutex. That

The key construction data doesn't exist during nfs_idmap_get_key().  request_key() sets it up to pass to our functions as it works through the keyrings code.

- Bryan

> leaves one possible race in which the idmap->idmap_key_cons is cleared
> while we're in idmap_pipe_downcall. One way to solve that race is to use
> a second mutex (see the original idmapper design from before your
> changes) to ensure that nothing clears idmap_key_cons while we're in
> idmap_pipe_downcall.
> 

Re: [PATCH] NFS: Clear key construction data if the idmap upcall fails

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTA4LTA3IGF0IDExOjUyIC0wNDAwLCBCcnlhbiBTY2h1bWFrZXIgd3JvdGU6
DQo+IE9uIDA4LzA3LzIwMTIgMTE6NDQgQU0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+ID4g
T24gVHVlLCAyMDEyLTA4LTA3IGF0IDExOjMwIC0wNDAwLCBianNjaHVtYUBuZXRhcHAuY29tIHdy
b3RlOg0KPiA+PiBGcm9tOiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+
ID4+DQo+ID4+IGlkbWFwX3BpcGVfZG93bmNhbGwgYWxyZWFkeSBjbGVhcnMgdGhpcyBmaWVsZCBp
ZiB0aGUgdXBjYWxsIHN1Y2NlZWRzLA0KPiA+PiBidXQgaWYgaXQgZmFpbHMgKHJwYy5pZG1hcGQg
aXNuJ3QgcnVubmluZykgdGhlIGZpZWxkIHdpbGwgc3RpbGwgYmUgc2V0DQo+ID4+IG9uIHRoZSBu
ZXh0IGNhbGwgdHJpZ2dlcmluZyBhIEJVR19PTigpLg0KPiA+Pg0KPiA+PiBTaWduZWQtb2ZmLWJ5
OiBCcnlhbiBTY2h1bWFrZXIgPGJqc2NodW1hQG5ldGFwcC5jb20+DQo+ID4+IC0tLQ0KPiA+PiAg
ZnMvbmZzL2lkbWFwLmMgfCAxNiArKysrKysrKystLS0tLS0tDQo+ID4+ICAxIGZpbGUgY2hhbmdl
ZCwgOSBpbnNlcnRpb25zKCspLCA3IGRlbGV0aW9ucygtKQ0KPiA+Pg0KPiA+PiBkaWZmIC0tZ2l0
IGEvZnMvbmZzL2lkbWFwLmMgYi9mcy9uZnMvaWRtYXAuYw0KPiA+PiBpbmRleCBiNzAxMzU4Li42
NDVjZmU3IDEwMDY0NA0KPiA+PiAtLS0gYS9mcy9uZnMvaWRtYXAuYw0KPiA+PiArKysgYi9mcy9u
ZnMvaWRtYXAuYw0KPiA+PiBAQCAtNjgzLDEwICs2ODMsMTIgQEAgc3RhdGljIGludCBuZnNfaWRt
YXBfbGVnYWN5X3VwY2FsbChzdHJ1Y3Qga2V5X2NvbnN0cnVjdGlvbiAqY29ucywNCj4gPj4gIA0K
PiA+PiAgCXJldCA9IHJwY19xdWV1ZV91cGNhbGwoaWRtYXAtPmlkbWFwX3BpcGUsIG1zZyk7DQo+
ID4+ICAJaWYgKHJldCA8IDApDQo+ID4+IC0JCWdvdG8gb3V0MjsNCj4gPj4gKwkJZ290byBvdXQz
Ow0KPiA+PiAgDQo+ID4+ICAJcmV0dXJuIHJldDsNCj4gPj4gIA0KPiA+PiArb3V0MzoNCj4gPj4g
KwlpZG1hcC0+aWRtYXBfa2V5X2NvbnMgPSBOVUxMOw0KPiA+PiAgb3V0MjoNCj4gPj4gIAlrZnJl
ZShpbSk7DQo+ID4+ICBvdXQxOg0KPiA+IA0KPiA+IFRoYXQgZml4ZXMgcnBjX3F1ZXVlX3VwY2Fs
bCBmYWlsdXJlIHBhdGgsIGJ1dCB3ZSBzdGlsbCBuZWVkIHRvIGRlYWwgd2l0aA0KPiA+IHRpbWVv
dXRzIGFuZCBjbG9zZS4NCj4gPiANCj4gPiBIb3cgYWJvdXQgdGhlIGZvbGxvd2luZyBhbHRlcm5h
dGl2ZToNCj4gPiANCj4gPiBTZXQgaWRtYXAtPmlkbWFwX2tleV9jb25zIGFmdGVyIGdyYWJiaW5n
IHRoZSBtdXRleCBpbg0KPiA+IG5mc19pZG1hcF9nZXRfa2V5KCksIHRoZW4gY2xlYXIgaXQgYmVm
b3JlIHlvdSByZWxlYXNlIHRoYXQgbXV0ZXguIFRoYXQNCj4gDQo+IFRoZSBrZXkgY29uc3RydWN0
aW9uIGRhdGEgZG9lc24ndCBleGlzdCBkdXJpbmcgbmZzX2lkbWFwX2dldF9rZXkoKS4gIHJlcXVl
c3Rfa2V5KCkgc2V0cyBpdCB1cCB0byBwYXNzIHRvIG91ciBmdW5jdGlvbnMgYXMgaXQgd29ya3Mg
dGhyb3VnaCB0aGUga2V5cmluZ3MgY29kZS4NCg0KT0ssIGhvd2V2ZXIgeW91IGNhbiBzdGlsbCBj
bGVhciBpZG1hcF9rZXlfY29ucyB0aGVyZS4uLg0KDQo+ID4gbGVhdmVzIG9uZSBwb3NzaWJsZSBy
YWNlIGluIHdoaWNoIHRoZSBpZG1hcC0+aWRtYXBfa2V5X2NvbnMgaXMgY2xlYXJlZA0KPiA+IHdo
aWxlIHdlJ3JlIGluIGlkbWFwX3BpcGVfZG93bmNhbGwuIE9uZSB3YXkgdG8gc29sdmUgdGhhdCBy
YWNlIGlzIHRvIHVzZQ0KPiA+IGEgc2Vjb25kIG11dGV4IChzZWUgdGhlIG9yaWdpbmFsIGlkbWFw
cGVyIGRlc2lnbiBmcm9tIGJlZm9yZSB5b3VyDQo+ID4gY2hhbmdlcykgdG8gZW5zdXJlIHRoYXQg
bm90aGluZyBjbGVhcnMgaWRtYXBfa2V5X2NvbnMgd2hpbGUgd2UncmUgaW4NCj4gPiBpZG1hcF9w
aXBlX2Rvd25jYWxsLg0KDQpUaGlzIHdvdWxkIHN0aWxsIGFwcGx5Lg0KDQotLSANClRyb25kIE15
a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlr
bGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==