* [PATCH] Bluetooth: bnep: Fix deadlock in session deletion. @ 2011-08-04 1:59 Jaikumar Ganesh 2011-08-04 16:13 ` Peter Hurley 0 siblings, 1 reply; 3+ messages in thread From: Jaikumar Ganesh @ 2011-08-04 1:59 UTC (permalink / raw) To: linux-bluetooth; +Cc: Jaikumar Ganesh Commit f4d7cd4a4c25cb4a5c30a675d4cc0052c93b925a introduced usage of <linux/kthread.h> API. kthread_stop is a blocking function which returns only when the thread exits. In this case, the thread couldn't exit because it was waiting to get a write semaphore. bnep_del_connection function which calls kthread_stop also held the read semaphore. Signed-off-by: Jaikumar Ganesh <jaikumar@google.com> --- net/bluetooth/bnep/core.c | 47 ++++++++++++++++++++++++++------------------ 1 files changed, 28 insertions(+), 19 deletions(-) diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c index eb8486f..f587b81 100644 --- a/net/bluetooth/bnep/core.c +++ b/net/bluetooth/bnep/core.c @@ -470,6 +470,31 @@ send: return len; } +static int cleanup_bnep_session(struct bnep_session *s) +{ + struct net_device *dev = s->dev; + + /* Cleanup session */ + down_write(&bnep_session_sem); + + /* Delete network device */ + unregister_netdev(dev); + + /* Wakeup user-space polling for socket errors */ + s->sock->sk->sk_err = EUNATCH; + + wake_up_interruptible(sk_sleep(s->sock->sk)); + + /* Release the socket */ + fput(s->sock->file); + + __bnep_unlink_session(s); + + up_write(&bnep_session_sem); + free_netdev(dev); + return 0; +} + static int bnep_session(void *arg) { struct bnep_session *s = arg; @@ -511,25 +536,6 @@ static int bnep_session(void *arg) } __set_current_state(TASK_RUNNING); remove_wait_queue(sk_sleep(sk), &wait); - - /* Cleanup session */ - down_write(&bnep_session_sem); - - /* Delete network device */ - unregister_netdev(dev); - - /* Wakeup user-space polling for socket errors */ - s->sock->sk->sk_err = EUNATCH; - - wake_up_interruptible(sk_sleep(s->sock->sk)); - - /* Release the socket */ - fput(s->sock->file); - - __bnep_unlink_session(s); - - up_write(&bnep_session_sem); - free_netdev(dev); return 0; } @@ -651,6 +657,9 @@ int bnep_del_connection(struct bnep_conndel_req *req) err = -ENOENT; up_read(&bnep_session_sem); + + if (!err) + cleanup_bnep_session(s); return err; } -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: bnep: Fix deadlock in session deletion. 2011-08-04 1:59 [PATCH] Bluetooth: bnep: Fix deadlock in session deletion Jaikumar Ganesh @ 2011-08-04 16:13 ` Peter Hurley 2011-08-04 21:37 ` Jaikumar Ganesh 0 siblings, 1 reply; 3+ messages in thread From: Peter Hurley @ 2011-08-04 16:13 UTC (permalink / raw) To: Jaikumar Ganesh; +Cc: linux-bluetooth@vger.kernel.org T24gV2VkLCAyMDExLTA4LTAzIGF0IDIxOjU5IC0wNDAwLCBKYWlrdW1hciBHYW5lc2ggd3JvdGU6 DQo+IENvbW1pdCBmNGQ3Y2Q0YTRjMjVjYjRhNWMzMGE2NzVkNGNjMDA1MmM5M2I5MjVhIGludHJv ZHVjZWQNCj4gdXNhZ2Ugb2YgPGxpbnV4L2t0aHJlYWQuaD4gQVBJLiBrdGhyZWFkX3N0b3AgaXMg YSBibG9ja2luZw0KPiBmdW5jdGlvbiB3aGljaCByZXR1cm5zIG9ubHkgd2hlbiB0aGUgdGhyZWFk IGV4aXRzLiBJbiB0aGlzDQo+IGNhc2UsIHRoZSB0aHJlYWQgY291bGRuJ3QgZXhpdCBiZWNhdXNl IGl0IHdhcyB3YWl0aW5nIHRvIGdldA0KPiBhIHdyaXRlIHNlbWFwaG9yZS4gYm5lcF9kZWxfY29u bmVjdGlvbiBmdW5jdGlvbiB3aGljaCBjYWxscw0KPiBrdGhyZWFkX3N0b3AgYWxzbyBoZWxkIHRo ZSByZWFkIHNlbWFwaG9yZS4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IEphaWt1bWFyIEdhbmVzaCA8 amFpa3VtYXJAZ29vZ2xlLmNvbT4NCj4gLS0tDQo+ICBuZXQvYmx1ZXRvb3RoL2JuZXAvY29yZS5j IHwgICA0NyArKysrKysrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tLQ0KPiAg MSBmaWxlcyBjaGFuZ2VkLCAyOCBpbnNlcnRpb25zKCspLCAxOSBkZWxldGlvbnMoLSkNCj4gDQo+ IGRpZmYgLS1naXQgYS9uZXQvYmx1ZXRvb3RoL2JuZXAvY29yZS5jIGIvbmV0L2JsdWV0b290aC9i bmVwL2NvcmUuYw0KPiBpbmRleCBlYjg0ODZmLi5mNTg3YjgxIDEwMDY0NA0KPiAtLS0gYS9uZXQv Ymx1ZXRvb3RoL2JuZXAvY29yZS5jDQo+ICsrKyBiL25ldC9ibHVldG9vdGgvYm5lcC9jb3JlLmMN Cj4gQEAgLTQ3MCw2ICs0NzAsMzEgQEAgc2VuZDoNCj4gIAlyZXR1cm4gbGVuOw0KPiAgfQ0KPiAg DQo+ICtzdGF0aWMgaW50IGNsZWFudXBfYm5lcF9zZXNzaW9uKHN0cnVjdCBibmVwX3Nlc3Npb24g KnMpDQo+ICt7DQo+ICsJc3RydWN0IG5ldF9kZXZpY2UgKmRldiA9IHMtPmRldjsNCj4gKw0KPiAr CS8qIENsZWFudXAgc2Vzc2lvbiAqLw0KPiArCWRvd25fd3JpdGUoJmJuZXBfc2Vzc2lvbl9zZW0p Ow0KPiArDQo+ICsJLyogRGVsZXRlIG5ldHdvcmsgZGV2aWNlICovDQo+ICsJdW5yZWdpc3Rlcl9u ZXRkZXYoZGV2KTsNCj4gKw0KPiArCS8qIFdha2V1cCB1c2VyLXNwYWNlIHBvbGxpbmcgZm9yIHNv Y2tldCBlcnJvcnMgKi8NCj4gKwlzLT5zb2NrLT5zay0+c2tfZXJyID0gRVVOQVRDSDsNCj4gKw0K PiArCXdha2VfdXBfaW50ZXJydXB0aWJsZShza19zbGVlcChzLT5zb2NrLT5zaykpOw0KPiArDQo+ ICsJLyogUmVsZWFzZSB0aGUgc29ja2V0ICovDQo+ICsJZnB1dChzLT5zb2NrLT5maWxlKTsNCj4g Kw0KPiArCV9fYm5lcF91bmxpbmtfc2Vzc2lvbihzKTsNCj4gKw0KPiArCXVwX3dyaXRlKCZibmVw X3Nlc3Npb25fc2VtKTsNCj4gKwlmcmVlX25ldGRldihkZXYpOw0KPiArCXJldHVybiAwOw0KPiAr fQ0KPiArDQo+ICBzdGF0aWMgaW50IGJuZXBfc2Vzc2lvbih2b2lkICphcmcpDQo+ICB7DQo+ICAJ c3RydWN0IGJuZXBfc2Vzc2lvbiAqcyA9IGFyZzsNCj4gQEAgLTUxMSwyNSArNTM2LDYgQEAgc3Rh dGljIGludCBibmVwX3Nlc3Npb24odm9pZCAqYXJnKQ0KPiAgCX0NCj4gIAlfX3NldF9jdXJyZW50 X3N0YXRlKFRBU0tfUlVOTklORyk7DQo+ICAJcmVtb3ZlX3dhaXRfcXVldWUoc2tfc2xlZXAoc2sp LCAmd2FpdCk7DQo+IC0NCj4gLQkvKiBDbGVhbnVwIHNlc3Npb24gKi8NCj4gLQlkb3duX3dyaXRl KCZibmVwX3Nlc3Npb25fc2VtKTsNCj4gLQ0KPiAtCS8qIERlbGV0ZSBuZXR3b3JrIGRldmljZSAq Lw0KPiAtCXVucmVnaXN0ZXJfbmV0ZGV2KGRldik7DQo+IC0NCj4gLQkvKiBXYWtldXAgdXNlci1z cGFjZSBwb2xsaW5nIGZvciBzb2NrZXQgZXJyb3JzICovDQo+IC0Jcy0+c29jay0+c2stPnNrX2Vy ciA9IEVVTkFUQ0g7DQo+IC0NCj4gLQl3YWtlX3VwX2ludGVycnVwdGlibGUoc2tfc2xlZXAocy0+ c29jay0+c2spKTsNCj4gLQ0KPiAtCS8qIFJlbGVhc2UgdGhlIHNvY2tldCAqLw0KPiAtCWZwdXQo cy0+c29jay0+ZmlsZSk7DQo+IC0NCj4gLQlfX2JuZXBfdW5saW5rX3Nlc3Npb24ocyk7DQo+IC0N Cj4gLQl1cF93cml0ZSgmYm5lcF9zZXNzaW9uX3NlbSk7DQo+IC0JZnJlZV9uZXRkZXYoZGV2KTsN Cj4gIAlyZXR1cm4gMDsNCj4gIH0NCg0KVGhpcyB3b24ndCB3b3JrIGJlY2F1c2UgdGhlIHNlc3Np b24gdGhyZWFkIGNhbiBleGl0IGl0c2VsZiAobGlrZSBpZiBpdA0KZGlzY292ZXJzIHRoYXQgdGhl IHNrX3N0YXRlIGlzIG5vIGxvbmdlciBCVF9DT05ORUNURUQpLg0KIA0KPiBAQCAtNjUxLDYgKzY1 Nyw5IEBAIGludCBibmVwX2RlbF9jb25uZWN0aW9uKHN0cnVjdCBibmVwX2Nvbm5kZWxfcmVxICpy ZXEpDQo+ICAJCWVyciA9IC1FTk9FTlQ7DQo+ICANCj4gIAl1cF9yZWFkKCZibmVwX3Nlc3Npb25f c2VtKTsNCj4gKw0KPiArCWlmICghZXJyKQ0KPiArCQljbGVhbnVwX2JuZXBfc2Vzc2lvbihzKTsN Cg0KU2luY2UgdGhlIHRocmVhZCBjYW4gZXhpdCBpdHNlbGYsIHRoZSBzZXNzaW9uIHMgbWF5IG5v IGxvbmdlciBiZSB2YWxpZA0KYWZ0ZXIgdGhlIHJlYWQgbG9jayBpcyByZWxlYXNlZC4NCg0KPiAg CXJldHVybiBlcnI7DQo+ICB9DQo+ICANCg0KRG9lcyB0aGUgcGF0Y2ggYmVsb3cgd29yayBmb3Ig eW91Pw0KDQotLS0NCiBuZXQvYmx1ZXRvb3RoL2JuZXAvYm5lcC5oIHwgICAgMSArDQogbmV0L2Js dWV0b290aC9ibmVwL2NvcmUuYyB8ICAgIDkgKysrKystLS0tDQogMiBmaWxlcyBjaGFuZ2VkLCA2 IGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9uZXQvYmx1ZXRv b3RoL2JuZXAvYm5lcC5oIGIvbmV0L2JsdWV0b290aC9ibmVwL2JuZXAuaA0KaW5kZXggOGU2YzA2 MS4uZTdlZTUzMSAxMDA2NDQNCi0tLSBhL25ldC9ibHVldG9vdGgvYm5lcC9ibmVwLmgNCisrKyBi L25ldC9ibHVldG9vdGgvYm5lcC9ibmVwLmgNCkBAIC0xNTUsNiArMTU1LDcgQEAgc3RydWN0IGJu ZXBfc2Vzc2lvbiB7DQogCXVuc2lnbmVkIGludCAgcm9sZTsNCiAJdW5zaWduZWQgbG9uZyBzdGF0 ZTsNCiAJdW5zaWduZWQgbG9uZyBmbGFnczsNCisJYXRvbWljX3QgICAgICB0ZXJtaW5hdGU7DQog CXN0cnVjdCB0YXNrX3N0cnVjdCAqdGFzazsNCiANCiAJc3RydWN0IGV0aGhkciBlaDsNCmRpZmYg LS1naXQgYS9uZXQvYmx1ZXRvb3RoL2JuZXAvY29yZS5jIGIvbmV0L2JsdWV0b290aC9ibmVwL2Nv cmUuYw0KaW5kZXggN2U4ZmYzYy4uZDllZGZlOCAxMDA2NDQNCi0tLSBhL25ldC9ibHVldG9vdGgv Ym5lcC9jb3JlLmMNCisrKyBiL25ldC9ibHVldG9vdGgvYm5lcC9jb3JlLmMNCkBAIC00ODcsNyAr NDg3LDcgQEAgc3RhdGljIGludCBibmVwX3Nlc3Npb24odm9pZCAqYXJnKQ0KIAl3aGlsZSAoMSkg ew0KIAkJc2V0X2N1cnJlbnRfc3RhdGUoVEFTS19JTlRFUlJVUFRJQkxFKTsNCiANCi0JCWlmIChr dGhyZWFkX3Nob3VsZF9zdG9wKCkpDQorCQlpZiAoYXRvbWljX3JlYWQoJnMtPnRlcm1pbmF0ZSkp DQogCQkJYnJlYWs7DQogCQkvKiBSWCAqLw0KIAkJd2hpbGUgKChza2IgPSBza2JfZGVxdWV1ZSgm c2stPnNrX3JlY2VpdmVfcXVldWUpKSkgew0KQEAgLTY0Miw5ICs2NDIsMTAgQEAgaW50IGJuZXBf ZGVsX2Nvbm5lY3Rpb24oc3RydWN0IGJuZXBfY29ubmRlbF9yZXENCipyZXEpDQogCWRvd25fcmVh ZCgmYm5lcF9zZXNzaW9uX3NlbSk7DQogDQogCXMgPSBfX2JuZXBfZ2V0X3Nlc3Npb24ocmVxLT5k c3QpOw0KLQlpZiAocykNCi0JCWt0aHJlYWRfc3RvcChzLT50YXNrKTsNCi0JZWxzZQ0KKwlpZiAo cykgew0KKwkJYXRvbWljX2luYygmcy0+dGVybWluYXRlKTsNCisJCXdha2VfdXBfcHJvY2Vzcyhz LT50YXNrKTsNCisJfSBlbHNlDQogCQllcnIgPSAtRU5PRU5UOw0KIA0KIAl1cF9yZWFkKCZibmVw X3Nlc3Npb25fc2VtKTsNCi0tIA0KMS43LjQuMQ0KDQoNCg== ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: bnep: Fix deadlock in session deletion. 2011-08-04 16:13 ` Peter Hurley @ 2011-08-04 21:37 ` Jaikumar Ganesh 0 siblings, 0 replies; 3+ messages in thread From: Jaikumar Ganesh @ 2011-08-04 21:37 UTC (permalink / raw) To: Peter Hurley; +Cc: linux-bluetooth@vger.kernel.org Hi Peter, On Thu, Aug 4, 2011 at 9:13 AM, Peter Hurley <peter@hurleysoftware.com> wrote: > On Wed, 2011-08-03 at 21:59 -0400, Jaikumar Ganesh wrote: >> Commit f4d7cd4a4c25cb4a5c30a675d4cc0052c93b925a introduced >> usage of <linux/kthread.h> API. kthread_stop is a blocking >> function which returns only when the thread exits. In this >> case, the thread couldn't exit because it was waiting to get >> a write semaphore. bnep_del_connection function which calls >> kthread_stop also held the read semaphore. >> >> Signed-off-by: Jaikumar Ganesh <jaikumar@google.com> >> --- >> net/bluetooth/bnep/core.c | 47 ++++++++++++++++++++++++++------------------ >> 1 files changed, 28 insertions(+), 19 deletions(-) >> >> diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c >> index eb8486f..f587b81 100644 >> --- a/net/bluetooth/bnep/core.c >> +++ b/net/bluetooth/bnep/core.c >> @@ -470,6 +470,31 @@ send: >> return len; >> } >> >> +static int cleanup_bnep_session(struct bnep_session *s) >> +{ >> + struct net_device *dev = s->dev; >> + >> + /* Cleanup session */ >> + down_write(&bnep_session_sem); >> + >> + /* Delete network device */ >> + unregister_netdev(dev); >> + >> + /* Wakeup user-space polling for socket errors */ >> + s->sock->sk->sk_err = EUNATCH; >> + >> + wake_up_interruptible(sk_sleep(s->sock->sk)); >> + >> + /* Release the socket */ >> + fput(s->sock->file); >> + >> + __bnep_unlink_session(s); >> + >> + up_write(&bnep_session_sem); >> + free_netdev(dev); >> + return 0; >> +} >> + >> static int bnep_session(void *arg) >> { >> struct bnep_session *s = arg; >> @@ -511,25 +536,6 @@ static int bnep_session(void *arg) >> } >> __set_current_state(TASK_RUNNING); >> remove_wait_queue(sk_sleep(sk), &wait); >> - >> - /* Cleanup session */ >> - down_write(&bnep_session_sem); >> - >> - /* Delete network device */ >> - unregister_netdev(dev); >> - >> - /* Wakeup user-space polling for socket errors */ >> - s->sock->sk->sk_err = EUNATCH; >> - >> - wake_up_interruptible(sk_sleep(s->sock->sk)); >> - >> - /* Release the socket */ >> - fput(s->sock->file); >> - >> - __bnep_unlink_session(s); >> - >> - up_write(&bnep_session_sem); >> - free_netdev(dev); >> return 0; >> } > > This won't work because the session thread can exit itself (like if it > discovers that the sk_state is no longer BT_CONNECTED). > >> @@ -651,6 +657,9 @@ int bnep_del_connection(struct bnep_conndel_req *req) >> err = -ENOENT; >> >> up_read(&bnep_session_sem); >> + >> + if (!err) >> + cleanup_bnep_session(s); > > Since the thread can exit itself, the session s may no longer be valid > after the read lock is released. I agree. > >> return err; >> } >> > > Does the patch below work for you? > > --- > net/bluetooth/bnep/bnep.h | 1 + > net/bluetooth/bnep/core.c | 9 +++++---- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h > index 8e6c061..e7ee531 100644 > --- a/net/bluetooth/bnep/bnep.h > +++ b/net/bluetooth/bnep/bnep.h > @@ -155,6 +155,7 @@ struct bnep_session { > unsigned int role; > unsigned long state; > unsigned long flags; > + atomic_t terminate; > struct task_struct *task; > > struct ethhdr eh; > diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c > index 7e8ff3c..d9edfe8 100644 > --- a/net/bluetooth/bnep/core.c > +++ b/net/bluetooth/bnep/core.c > @@ -487,7 +487,7 @@ static int bnep_session(void *arg) > while (1) { > set_current_state(TASK_INTERRUPTIBLE); > > - if (kthread_should_stop()) > + if (atomic_read(&s->terminate)) > break; > /* RX */ > while ((skb = skb_dequeue(&sk->sk_receive_queue))) { > @@ -642,9 +642,10 @@ int bnep_del_connection(struct bnep_conndel_req > *req) > down_read(&bnep_session_sem); > > s = __bnep_get_session(req->dst); > - if (s) > - kthread_stop(s->task); > - else > + if (s) { > + atomic_inc(&s->terminate); > + wake_up_process(s->task); > + } else > err = -ENOENT; > > up_read(&bnep_session_sem); > -- > 1.7.4.1 > > The patch works fine. Thanks > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-08-04 21:37 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-08-04 1:59 [PATCH] Bluetooth: bnep: Fix deadlock in session deletion Jaikumar Ganesh 2011-08-04 16:13 ` Peter Hurley 2011-08-04 21:37 ` Jaikumar Ganesh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).