cfg80211: race problem between suspend and disconnect event

cfg80211: race problem between suspend and disconnect event

[Amitkumar Karwar]

Hi Johannes,

We came across one race issue while running suspend tests with Marvell device.

Mwifiex driver rejects del_key() requests from cfg80211 during suspend. They came very late when driver's cfg80211_suspend handler is already executed and driver is in the middle of SDIO's suspend handler.

In wowlan not configured case, below code ensures cfg80211 terminates connection before calling ops->suspend. However when driver reports disconnection through cfg80211_disconnected() API, cfg80211 handles EVENT_DISCONNECTED in separate work function where ops->del_key() is called in loop to flush keys.

------wiphy_suspend()-----------
if (rdev->wiphy.registered) {
         if (!rdev->wiphy.wowlan_config)
                cfg80211_leave_all(rdev);
          if (rdev->ops->suspend)
           ret = rdev_suspend(rdev, rdev->wiphy.wowlan_config);
--------------------------------

Please let us know if you have any suggestions to resolves this with cfg80211/driver change.

Regards,
Amitkumar

Re: cfg80211: race problem between suspend and disconnect event

[Johannes Berg]

Hi,

> Mwifiex driver rejects del_key() requests from cfg80211 during
> suspend. They came very late when driver's cfg80211_suspend handler
> is already executed and driver is in the middle of SDIO's suspend
> handler.

Interesting. Rejecting those calls is probably perfectly reasonable,
and in fact it's not clear to me why we even try to delete the keys
after we've disconnected - any driver implementation should have
removed them already anyway? You probably don't actually care about the
key removal either?

That said though, there's also the critical protocol stop and the
set_qos_map(NULL) call, which removes the QoS mapping. It doesn't look
like you support this right now in your driver, but in any case it'd be
pretty strange to have that happen after or during suspend.

> Please let us know if you have any suggestions to resolves this with
> cfg80211/driver change.

For cfg80211 we could do something like this:

--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -104,13 +104,16 @@ static int wiphy_suspend(struct device *dev)
 
 	rtnl_lock();
 	if (rdev->wiphy.registered) {
-		if (!rdev->wiphy.wowlan_config)
+		if (!rdev->wiphy.wowlan_config) {
 			cfg80211_leave_all(rdev);
+			cfg80211_process_rdev_events(rdev);
+		}
 		if (rdev->ops->suspend)
 			ret = rdev_suspend(rdev, rdev->wiphy.wowlan_config);
 		if (ret == 1) {
 			/* Driver refuse to configure wowlan */
 			cfg80211_leave_all(rdev);
+			cfg80211_process_rdev_events(rdev);
 			ret = rdev_suspend(rdev, NULL);
 		}
 	}


However, that assumes that you actually cfg80211_disconnected()
synchronously while being asked to disconnect, which doesn't seem to be
true from looking at mwifiex, if HostCmd_CMD_802_11_DEAUTHENTICATE
command can be sent to the firmware then you wait for EVENT_LINK_LOST,
EVENT_DEAUTHENTICATED or EVENT_DISASSOCIATED to come back from the
firmware, so this cfg80211 change won't help.


So somehow you'd have to synchronize with the firmware as well, to
process all those things before suspend, I guess?

We could then export cfg80211_process_rdev_events() as
cfg80211_process_wiphy_events() or so, so that you can call that at an
appropriate place from your suspend handler, after having synchronized
with the firmware?

johannes

RE: cfg80211: race problem between suspend and disconnect event

[Amitkumar Karwar]

SGkgSm9oYW5uZXMsDQoNCj4gRnJvbTogSm9oYW5uZXMgQmVyZyBbbWFpbHRvOmpvaGFubmVzQHNp
cHNvbHV0aW9ucy5uZXRdDQo+IFNlbnQ6IEZyaWRheSwgT2N0b2JlciAyMSwgMjAxNiAxMjoxNiBB
TQ0KPiBUbzogQW1pdGt1bWFyIEthcndhcg0KPiBDYzogS2FsbGUgVmFsbzsgQnJpYW4gTm9ycmlz
OyBOaXNoYW50IFNhcm11a2FkYW07IENhdGh5IEx1bzsgbGludXgtDQo+IHdpcmVsZXNzQHZnZXIu
a2VybmVsLm9yZzsgR2FuYXBhdGhpIEJoYXQNCj4gU3ViamVjdDogUmU6IGNmZzgwMjExOiByYWNl
IHByb2JsZW0gYmV0d2VlbiBzdXNwZW5kIGFuZCBkaXNjb25uZWN0IGV2ZW50DQo+IA0KPiBIaSwN
Cj4gDQo+ID4gTXdpZmlleCBkcml2ZXIgcmVqZWN0cyBkZWxfa2V5KCkgcmVxdWVzdHMgZnJvbSBj
Zmc4MDIxMSBkdXJpbmcNCj4gPiBzdXNwZW5kLiBUaGV5IGNhbWUgdmVyeSBsYXRlIHdoZW4gZHJp
dmVyJ3MgY2ZnODAyMTFfc3VzcGVuZCBoYW5kbGVyIGlzDQo+ID4gYWxyZWFkeSBleGVjdXRlZCBh
bmQgZHJpdmVyIGlzIGluIHRoZSBtaWRkbGUgb2YgU0RJTydzIHN1c3BlbmQNCj4gPiBoYW5kbGVy
Lg0KPiANCj4gSW50ZXJlc3RpbmcuIFJlamVjdGluZyB0aG9zZSBjYWxscyBpcyBwcm9iYWJseSBw
ZXJmZWN0bHkgcmVhc29uYWJsZSwgYW5kDQo+IGluIGZhY3QgaXQncyBub3QgY2xlYXIgdG8gbWUg
d2h5IHdlIGV2ZW4gdHJ5IHRvIGRlbGV0ZSB0aGUga2V5cyBhZnRlcg0KPiB3ZSd2ZSBkaXNjb25u
ZWN0ZWQgLSBhbnkgZHJpdmVyIGltcGxlbWVudGF0aW9uIHNob3VsZCBoYXZlIHJlbW92ZWQgdGhl
bQ0KPiBhbHJlYWR5IGFueXdheT8gWW91IHByb2JhYmx5IGRvbid0IGFjdHVhbGx5IGNhcmUgYWJv
dXQgdGhlIGtleSByZW1vdmFsDQo+IGVpdGhlcj8NCg0KVGhhbmtzIGZvciB5b3VyIHJlcGx5Lg0K
DQpZZXMuIEluIG91ciBjYXNlLCAqODAyXzExX0RFQVVUSEVOVElDQVRFIGNvbW1hbmQgZG93bmxv
YWRlZCB0byBmaXJtd2FyZSB0YWtlcyBjYXJlIG9mIGZsdXNoaW5nIHRoZSBrZXlzLg0KDQpJIGNh
biBzZWUgYmVsb3cgY29kZSBpbiBjZmc4MDIxMSdzIGRpc2Nvbm5lY3QgaGFuZGxpbmcuIEl0IHNl
ZW1zIHRvIGJlIHRoZXJlIGZvciBsb25nIHRpbWUuDQoNCi0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0K
LyoNCiAqIERlbGV0ZSBhbGwgdGhlIGtleXMgLi4uIHBhaXJ3aXNlIGtleXMgY2FuJ3QgcmVhbGx5
DQogKiBleGlzdCBhbnkgbW9yZSBhbnl3YXksIGJ1dCBkZWZhdWx0IGtleXMgbWlnaHQuDQogKi8N
CiBpZiAocmRldi0+b3BzLT5kZWxfa2V5KQ0KICAgIGZvciAoaSA9IDA7IGkgPCA2OyBpKyspDQog
ICAgICAgcmRldl9kZWxfa2V5KHJkZXYsIGRldiwgaSwgZmFsc2UsIE5VTEwpOw0KLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0NCg0KPiANCj4gVGhhdCBzYWlkIHRob3VnaCwgdGhlcmUncyBhbHNvIHRo
ZSBjcml0aWNhbCBwcm90b2NvbCBzdG9wIGFuZCB0aGUNCj4gc2V0X3Fvc19tYXAoTlVMTCkgY2Fs
bCwgd2hpY2ggcmVtb3ZlcyB0aGUgUW9TIG1hcHBpbmcuIEl0IGRvZXNuJ3QgbG9vaw0KPiBsaWtl
IHlvdSBzdXBwb3J0IHRoaXMgcmlnaHQgbm93IGluIHlvdXIgZHJpdmVyLCBidXQgaW4gYW55IGNh
c2UgaXQnZCBiZQ0KPiBwcmV0dHkgc3RyYW5nZSB0byBoYXZlIHRoYXQgaGFwcGVuIGFmdGVyIG9y
IGR1cmluZyBzdXNwZW5kLg0KPiANCj4gPiBQbGVhc2UgbGV0IHVzIGtub3cgaWYgeW91IGhhdmUg
YW55IHN1Z2dlc3Rpb25zIHRvIHJlc29sdmVzIHRoaXMgd2l0aA0KPiA+IGNmZzgwMjExL2RyaXZl
ciBjaGFuZ2UuDQo+IA0KPiBGb3IgY2ZnODAyMTEgd2UgY291bGQgZG8gc29tZXRoaW5nIGxpa2Ug
dGhpczoNCj4gDQo+IC0tLSBhL25ldC93aXJlbGVzcy9zeXNmcy5jDQo+ICsrKyBiL25ldC93aXJl
bGVzcy9zeXNmcy5jDQo+IEBAIC0xMDQsMTMgKzEwNCwxNiBAQCBzdGF0aWMgaW50IHdpcGh5X3N1
c3BlbmQoc3RydWN0IGRldmljZSAqZGV2KQ0KPiANCj4gIAlydG5sX2xvY2soKTsNCj4gIAlpZiAo
cmRldi0+d2lwaHkucmVnaXN0ZXJlZCkgew0KPiAtCQlpZiAoIXJkZXYtPndpcGh5Lndvd2xhbl9j
b25maWcpDQo+ICsJCWlmICghcmRldi0+d2lwaHkud293bGFuX2NvbmZpZykgew0KPiAgCQkJY2Zn
ODAyMTFfbGVhdmVfYWxsKHJkZXYpOw0KPiArCQkJY2ZnODAyMTFfcHJvY2Vzc19yZGV2X2V2ZW50
cyhyZGV2KTsNCj4gKwkJfQ0KPiAgCQlpZiAocmRldi0+b3BzLT5zdXNwZW5kKQ0KPiAgCQkJcmV0
ID0gcmRldl9zdXNwZW5kKHJkZXYsIHJkZXYtPndpcGh5Lndvd2xhbl9jb25maWcpOw0KPiAgCQlp
ZiAocmV0ID09IDEpIHsNCj4gIAkJCS8qIERyaXZlciByZWZ1c2UgdG8gY29uZmlndXJlIHdvd2xh
biAqLw0KPiAgCQkJY2ZnODAyMTFfbGVhdmVfYWxsKHJkZXYpOw0KPiArCQkJY2ZnODAyMTFfcHJv
Y2Vzc19yZGV2X2V2ZW50cyhyZGV2KTsNCj4gIAkJCXJldCA9IHJkZXZfc3VzcGVuZChyZGV2LCBO
VUxMKTsNCj4gIAkJfQ0KPiAgCX0NCj4gDQo+IA0KPiBIb3dldmVyLCB0aGF0IGFzc3VtZXMgdGhh
dCB5b3UgYWN0dWFsbHnCoGNmZzgwMjExX2Rpc2Nvbm5lY3RlZCgpDQo+IHN5bmNocm9ub3VzbHkg
d2hpbGUgYmVpbmcgYXNrZWQgdG8gZGlzY29ubmVjdCwgd2hpY2ggZG9lc24ndCBzZWVtIHRvIGJl
DQo+IHRydWUgZnJvbSBsb29raW5nIGF0IG13aWZpZXgsIGlmIEhvc3RDbWRfQ01EXzgwMl8xMV9E
RUFVVEhFTlRJQ0FURQ0KPiBjb21tYW5kIGNhbiBiZSBzZW50IHRvIHRoZSBmaXJtd2FyZSB0aGVu
IHlvdSB3YWl0IGZvciBFVkVOVF9MSU5LX0xPU1QsDQo+IEVWRU5UX0RFQVVUSEVOVElDQVRFRCBv
csKgRVZFTlRfRElTQVNTT0NJQVRFRCB0byBjb21lIGJhY2sgZnJvbSB0aGUNCj4gZmlybXdhcmUs
IHNvIHRoaXMgY2ZnODAyMTEgY2hhbmdlIHdvbid0IGhlbHAuDQo+IA0KDQpJIHRoaW5rLCB5b3Vy
IGNmZzgwMjExIGNoYW5nZSB3aWxsIGhlbHAuIFdlIGRvIGVuc3VyZSB0aGF0IGNmZzgwMjExX2Rp
c2Nvbm5lY3RlZCgpIGlzIGNhbGxlZCBiZWZvcmUgZXhpdGluZyBtd2lmaWV4X2NmZzgwMjExX2Rp
c2Nvbm5lY3QoKS4NClNlbmRpbmcgSG9zdENtZF9DTURfODAyXzExX0RFQVVUSEVOVElDQVRFIGNv
bW1hbmQgdG8gZmlybXdhcmUgaXMgYSBibG9ja2luZyBjYWxsLiBjZmc4MDIxMV9kaXNjb25uZWN0
ZWQoKSBpcyBjYWxsZWQgd2hpbGUgaGFuZGxpbmcgdGhhdCBjb21tYW5kJ3MgcmVzcG9uc2UuIA0K
DQptd2lmaWV4X3JldF84MDJfMTFfZGVhdXRoZW50aWNhdGUoKS0+bXdpZmlleF9yZXNldF9jb25u
ZWN0X3N0YXRlKCktPmNmZzgwMjExX2Rpc2Nvbm5lY3RlZCgpDQoNCj4gDQo+IFNvIHNvbWVob3cg
eW91J2QgaGF2ZSB0byBzeW5jaHJvbml6ZSB3aXRoIHRoZSBmaXJtd2FyZSBhcyB3ZWxsLCB0bw0K
PiBwcm9jZXNzIGFsbCB0aG9zZSB0aGluZ3MgYmVmb3JlIHN1c3BlbmQsIEkgZ3Vlc3M/DQo+IA0K
PiBXZSBjb3VsZCB0aGVuIGV4cG9ydCBjZmc4MDIxMV9wcm9jZXNzX3JkZXZfZXZlbnRzKCkgYXMN
Cj4gY2ZnODAyMTFfcHJvY2Vzc193aXBoeV9ldmVudHMoKSBvciBzbywgc28gdGhhdCB5b3UgY2Fu
IGNhbGwgdGhhdCBhdCBhbg0KPiBhcHByb3ByaWF0ZSBwbGFjZSBmcm9tIHlvdXIgc3VzcGVuZCBo
YW5kbGVyLCBhZnRlciBoYXZpbmcgc3luY2hyb25pemVkDQo+IHdpdGggdGhlIGZpcm13YXJlPw0K
DQpUaGlzIHdvdWxkIG5vdCBiZSBuZWVkZWQuIA0KDQpSZWdhcmRzLA0KQW1pdGt1bWFyDQo=

Re: cfg80211: race problem between suspend and disconnect event

[Johannes Berg]

> Yes. In our case, *802_11_DEAUTHENTICATE command downloaded to
> firmware takes care of flushing the keys.
> 
> I can see below code in cfg80211's disconnect handling. It seems to
> be there for long time.

Yeah, I saw it, but it's not clear to me that there's much point in it.
In any case, that's an unrelated question in a way, because there
definitely are things happening here that should be "more synchronous",
regardless of whether or not the key deletion makes sense.

> I think, your cfg80211 change will help. We do ensure that
> cfg80211_disconnected() is called before exiting
> mwifiex_cfg80211_disconnect().
> Sending HostCmd_CMD_802_11_DEAUTHENTICATE command to firmware is a
> blocking call. cfg80211_disconnected() is called while handling that
> command's response. 

Ah ok, I missed that - I thought it was asynchronous.

> > So somehow you'd have to synchronize with the firmware as well, to
> > process all those things before suspend, I guess?
> > 
[...]
> This would not be needed. 

Right. Care to test my patch before I properly submit it?

johannes

RE: cfg80211: race problem between suspend and disconnect event

[Amitkumar Karwar]

SGkgSm9oYW5uZXMsDQoNCj4gRnJvbTogSm9oYW5uZXMgQmVyZyBbbWFpbHRvOmpvaGFubmVzQHNp
cHNvbHV0aW9ucy5uZXRdDQo+IFNlbnQ6IFNhdHVyZGF5LCBPY3RvYmVyIDIyLCAyMDE2IDI6NTQg
QU0NCj4gVG86IEFtaXRrdW1hciBLYXJ3YXINCj4gQ2M6IEthbGxlIFZhbG87IEJyaWFuIE5vcnJp
czsgTmlzaGFudCBTYXJtdWthZGFtOyBDYXRoeSBMdW87IGxpbnV4LQ0KPiB3aXJlbGVzc0B2Z2Vy
Lmtlcm5lbC5vcmc7IEdhbmFwYXRoaSBCaGF0DQo+IFN1YmplY3Q6IFJlOiBjZmc4MDIxMTogcmFj
ZSBwcm9ibGVtIGJldHdlZW4gc3VzcGVuZCBhbmQgZGlzY29ubmVjdCBldmVudA0KPiANCj4gDQo+
ID4gWWVzLiBJbiBvdXIgY2FzZSwgKjgwMl8xMV9ERUFVVEhFTlRJQ0FURSBjb21tYW5kIGRvd25s
b2FkZWQgdG8NCj4gPiBmaXJtd2FyZSB0YWtlcyBjYXJlIG9mIGZsdXNoaW5nIHRoZSBrZXlzLg0K
PiA+DQo+ID4gSSBjYW4gc2VlIGJlbG93IGNvZGUgaW4gY2ZnODAyMTEncyBkaXNjb25uZWN0IGhh
bmRsaW5nLiBJdCBzZWVtcyB0byBiZQ0KPiA+IHRoZXJlIGZvciBsb25nIHRpbWUuDQo+IA0KPiBZ
ZWFoLCBJIHNhdyBpdCwgYnV0IGl0J3Mgbm90IGNsZWFyIHRvIG1lIHRoYXQgdGhlcmUncyBtdWNo
IHBvaW50IGluIGl0Lg0KPiBJbiBhbnkgY2FzZSwgdGhhdCdzIGFuIHVucmVsYXRlZCBxdWVzdGlv
biBpbiBhIHdheSwgYmVjYXVzZSB0aGVyZQ0KPiBkZWZpbml0ZWx5IGFyZSB0aGluZ3MgaGFwcGVu
aW5nIGhlcmUgdGhhdCBzaG91bGQgYmUgIm1vcmUgc3luY2hyb25vdXMiLA0KPiByZWdhcmRsZXNz
IG9mIHdoZXRoZXIgb3Igbm90IHRoZSBrZXkgZGVsZXRpb24gbWFrZXMgc2Vuc2UuDQo+IA0KPiA+
IEkgdGhpbmssIHlvdXIgY2ZnODAyMTEgY2hhbmdlIHdpbGwgaGVscC4gV2UgZG8gZW5zdXJlIHRo
YXQNCj4gPiBjZmc4MDIxMV9kaXNjb25uZWN0ZWQoKSBpcyBjYWxsZWQgYmVmb3JlIGV4aXRpbmcN
Cj4gPiBtd2lmaWV4X2NmZzgwMjExX2Rpc2Nvbm5lY3QoKS4NCj4gPiBTZW5kaW5nIEhvc3RDbWRf
Q01EXzgwMl8xMV9ERUFVVEhFTlRJQ0FURSBjb21tYW5kIHRvIGZpcm13YXJlIGlzIGENCj4gPiBi
bG9ja2luZyBjYWxsLiBjZmc4MDIxMV9kaXNjb25uZWN0ZWQoKSBpcyBjYWxsZWQgd2hpbGUgaGFu
ZGxpbmcgdGhhdA0KPiA+IGNvbW1hbmQncyByZXNwb25zZS4NCj4gDQo+IEFoIG9rLCBJIG1pc3Nl
ZCB0aGF0IC0gSSB0aG91Z2h0IGl0IHdhcyBhc3luY2hyb25vdXMuDQo+IA0KPiA+ID4gU28gc29t
ZWhvdyB5b3UnZCBoYXZlIHRvIHN5bmNocm9uaXplIHdpdGggdGhlIGZpcm13YXJlIGFzIHdlbGws
IHRvDQo+ID4gPiBwcm9jZXNzIGFsbCB0aG9zZSB0aGluZ3MgYmVmb3JlIHN1c3BlbmQsIEkgZ3Vl
c3M/DQo+ID4gPg0KPiBbLi4uXQ0KPiA+IFRoaXMgd291bGQgbm90IGJlIG5lZWRlZC4NCj4gDQo+
IFJpZ2h0LiBDYXJlIHRvIHRlc3QgbXkgcGF0Y2ggYmVmb3JlIEkgcHJvcGVybHkgc3VibWl0IGl0
Pw0KDQpXZSByYW4gdGhlIHRlc3RzIGFuZCBjb25maXJtZWQgdGhhdCB5b3VyIHBhdGNoIGRvZXMg
c29sdmUgdGhlIHByb2JsZW0uIFlvdSBjYW4gc3VibWl0IGl0Lg0KDQpSZWdhcmRzLA0KQW1pdGt1
bWFyDQo=