* [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).