* [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case
@ 2012-12-25 10:04 Chuansheng Liu
2013-01-03 22:02 ` Gustavo Padovan
0 siblings, 1 reply; 5+ messages in thread
From: Chuansheng Liu @ 2012-12-25 10:04 UTC (permalink / raw)
To: gustavo, marcel, johan.hedberg
Cc: linux-bluetooth, linux-kernel, chuansheng.liu
Meet one panic issue as below stack:
<1>[11340.226404] BUG: unable to handle kernel NULL pointer dereference at 00000008
<4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0
<4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX: 00000000
<4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP: e0fdff1c
<0>[11340.226674] Stack:
<4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 c1754aef dec83e00 00000000 e0fdff5c
<4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c c1751852 d7813800 62262f10
<4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 0000000d e0fdffac c175425c 00000041
<0>[11340.226793] Call Trace:
<4>[11340.226813] [<c184db87>] ? sco_sock_clear_timer+0x27/0x60
<4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0
<4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60
<4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0
<4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80
<4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60
<4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0
<4>[11340.226929] [<c149ba78>] ? trace_hardirqs_on_thunk+0xc/0x10
<4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb
<4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7
<0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b 19 01 74 2f b8 0a 00 00
Disassemble the code:
base address of __sco_sock_close is 0xc184f410
0xc184f4f8 <+232>: lock decl 0x8(%ebx) < == crash here, ebx is 0x0,
the related source code is:
(gdb) l *0xc184f4f8
0xc184f4f8 is in __sco_sock_close (arch/x86/include/asm/atomic.h:123)
119 static inline int atomic_dec_and_test(atomic_t *v)
123 asm volatile(LOCK_PREFIX "decl %0; sete %1"
The whole call stack is:
sys_shutdown()
sco_sock_shutdown()
__sco_sock_close()
hci_conn_put()
atomic_dec_and_test()
Due to the conn->hcon is NULL, and the member hcon->refcnt is at offset 0x8,
so "BUG: unable to handle kernel NULL pointer dereference at 00000008"
appears.
Here fix it that adding the condition if conn->hcon is NULL, just like
in sco_chan_del().
Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
net/bluetooth/sco.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 531a93d..190f70c 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -355,8 +355,10 @@ static void __sco_sock_close(struct sock *sk)
if (sco_pi(sk)->conn) {
sk->sk_state = BT_DISCONN;
sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
- hci_conn_put(sco_pi(sk)->conn->hcon);
- sco_pi(sk)->conn->hcon = NULL;
+ if (sco_pi(sk)->conn->hcon) {
+ hci_conn_put(sco_pi(sk)->conn->hcon);
+ sco_pi(sk)->conn->hcon = NULL;
+ }
} else
sco_chan_del(sk, ECONNRESET);
break;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case 2012-12-25 10:04 [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case Chuansheng Liu @ 2013-01-03 22:02 ` Gustavo Padovan 2013-01-04 0:55 ` Liu, Chuansheng 0 siblings, 1 reply; 5+ messages in thread From: Gustavo Padovan @ 2013-01-03 22:02 UTC (permalink / raw) To: Chuansheng Liu; +Cc: marcel, johan.hedberg, linux-bluetooth, linux-kernel Hi Chuansheng, * Chuansheng Liu <chuansheng.liu@intel.com> [2012-12-25 18:04:17 +0800]: > > Meet one panic issue as below stack: > <1>[11340.226404] BUG: unable to handle kernel NULL pointer dereference at 00000008 > <4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0 > <4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX: 00000000 > <4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP: e0fdff1c > <0>[11340.226674] Stack: > <4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 c1754aef dec83e00 00000000 e0fdff5c > <4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c c1751852 d7813800 62262f10 > <4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 0000000d e0fdffac c175425c 00000041 > <0>[11340.226793] Call Trace: > <4>[11340.226813] [<c184db87>] ? sco_sock_clear_timer+0x27/0x60 > <4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0 > <4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60 > <4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0 > <4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80 > <4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60 > <4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0 > <4>[11340.226929] [<c149ba78>] ? trace_hardirqs_on_thunk+0xc/0x10 > <4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb > <4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7 > <0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b 19 01 74 2f b8 0a 00 00 > > Disassemble the code: > base address of __sco_sock_close is 0xc184f410 > 0xc184f4f8 <+232>: lock decl 0x8(%ebx) < == crash here, ebx is 0x0, > > the related source code is: > (gdb) l *0xc184f4f8 > 0xc184f4f8 is in __sco_sock_close (arch/x86/include/asm/atomic.h:123) > 119 static inline int atomic_dec_and_test(atomic_t *v) > 123 asm volatile(LOCK_PREFIX "decl %0; sete %1" > > The whole call stack is: > sys_shutdown() > sco_sock_shutdown() > __sco_sock_close() > hci_conn_put() > atomic_dec_and_test() > > Due to the conn->hcon is NULL, and the member hcon->refcnt is at offset 0x8, > so "BUG: unable to handle kernel NULL pointer dereference at 00000008" > appears. > > Here fix it that adding the condition if conn->hcon is NULL, just like > in sco_chan_del(). > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > --- > net/bluetooth/sco.c | 6 ++++-- > 1 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index 531a93d..190f70c 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -355,8 +355,10 @@ static void __sco_sock_close(struct sock *sk) > if (sco_pi(sk)->conn) { > sk->sk_state = BT_DISCONN; > sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); > - hci_conn_put(sco_pi(sk)->conn->hcon); > - sco_pi(sk)->conn->hcon = NULL; > + if (sco_pi(sk)->conn->hcon) { > + hci_conn_put(sco_pi(sk)->conn->hcon); > + sco_pi(sk)->conn->hcon = NULL; > + } > } else > sco_chan_del(sk, ECONNRESET); > break; Please check if the following patch fixes the issue for you: commit ae5668c1fc155d3034d0eedcdb52798390975a39 (HEAD, master) Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Date: Thu Jan 3 19:59:28 2013 -0200 Bluetooth: Check if the hci connection exists in SCO shutdown Checking only for sco_conn seems to not be enough and lead to NULL dereferences in the code, check for hcon instead. <1>[11340.226404] BUG: unable to handle kernel NULL pointer dereference at 0000000 8 <4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0 <4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX: 00000000 <4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP: e0fdff1c <0>[11340.226674] Stack: <4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 c1754aef dec83e00 00000000 e0fdff5c <4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c c1751852 d7813800 62262f10 <4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 0000000d e0fdffac c175425c 00000041 <0>[11340.226793] Call Trace: <4>[11340.226813] [<c184db87>] ? sco_sock_clear_timer+0x27/0x60 <4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0 <4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60 <4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0 <4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80 <4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60 <4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0 <4>[11340.226929] [<c149ba78>] ? trace_hardirqs_on_thunk+0xc/0x10 <4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb <4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7 <0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b 19 01 74 2f b8 0a 00 00 Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c index 531a93d..57f250c 100644 --- a/net/bluetooth/sco.c +++ b/net/bluetooth/sco.c @@ -352,7 +352,7 @@ static void __sco_sock_close(struct sock *sk) case BT_CONNECTED: case BT_CONFIG: - if (sco_pi(sk)->conn) { + if (sco_pi(sk)->conn->hcon) { sk->sk_state = BT_DISCONN; sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); hci_conn_put(sco_pi(sk)->conn->hcon); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case 2013-01-03 22:02 ` Gustavo Padovan @ 2013-01-04 0:55 ` Liu, Chuansheng 2013-01-09 20:34 ` Gustavo Padovan 0 siblings, 1 reply; 5+ messages in thread From: Liu, Chuansheng @ 2013-01-04 0:55 UTC (permalink / raw) To: Gustavo Padovan Cc: marcel@holtmann.org, johan.hedberg@gmail.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org, Liu, Chuansheng > -----Original Message----- > From: Gustavo Padovan [mailto:gustavo@padovan.org] > Sent: Friday, January 04, 2013 6:03 AM > To: Liu, Chuansheng > Cc: marcel@holtmann.org; johan.hedberg@gmail.com; > linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon =3D=3D NUL= L in > shutdown case >=20 > Hi Chuansheng, >=20 > * Chuansheng Liu <chuansheng.liu@intel.com> [2012-12-25 18:04:17 +0800]: >=20 > > > > Meet one panic issue as below stack: > > Disassemble the code: > > base address of __sco_sock_close is 0xc184f410 > > 0xc184f4f8 <+232>: lock decl 0x8(%ebx) < =3D=3D crash here, ebx is 0x= 0, > > > > the related source code is: > > (gdb) l *0xc184f4f8 > > 0xc184f4f8 is in __sco_sock_close (arch/x86/include/asm/atomic.h:123) > > 119 static inline int atomic_dec_and_test(atomic_t *v) > > 123 asm volatile(LOCK_PREFIX "decl %0; sete %1" > > > > The whole call stack is: > > sys_shutdown() > > sco_sock_shutdown() > > __sco_sock_close() > > hci_conn_put() > > atomic_dec_and_test() > > > > Due to the conn->hcon is NULL, and the member hcon->refcnt is at offset= 0x8, > > so "BUG: unable to handle kernel NULL pointer dereference at 00000008" > > appears. Could you add the above crash info to indicate where crashed? Thanks. > > > > Here fix it that adding the condition if conn->hcon is NULL, just like > > in sco_chan_del(). > > > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > > --- > > net/bluetooth/sco.c | 6 ++++-- > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > index 531a93d..190f70c 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -355,8 +355,10 @@ static void __sco_sock_close(struct sock *sk) > > if (sco_pi(sk)->conn) { > > sk->sk_state =3D BT_DISCONN; > > sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); > > - hci_conn_put(sco_pi(sk)->conn->hcon); > > - sco_pi(sk)->conn->hcon =3D NULL; > > + if (sco_pi(sk)->conn->hcon) { > > + hci_conn_put(sco_pi(sk)->conn->hcon); > > + sco_pi(sk)->conn->hcon =3D NULL; > > + } > > } else > > sco_chan_del(sk, ECONNRESET); > > break; >=20 > Please check if the following patch fixes the issue for you: >=20 > commit ae5668c1fc155d3034d0eedcdb52798390975a39 (HEAD, master) > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Date: Thu Jan 3 19:59:28 2013 -0200 >=20 > Bluetooth: Check if the hci connection exists in SCO shutdown >=20 > Checking only for sco_conn seems to not be enough and lead to NULL > dereferences in the code, check for hcon instead. >=20 > <1>[11340.226404] BUG: unable to handle kernel NULL pointer > dereference at > 0000000 > 8 > <4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0 > <4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX: > 00000000 > <4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP: > e0fdff1c > <0>[11340.226674] Stack: > <4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 c1754aef > dec83e00 > 00000000 > e0fdff5c > <4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c > c1751852 > d7813800 > 62262f10 > <4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 0000000d > e0fdffac > c175425c > 00000041 > <0>[11340.226793] Call Trace: > <4>[11340.226813] [<c184db87>] ? sco_sock_clear_timer+0x27/0x60 > <4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0 > <4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60 > <4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0 > <4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80 > <4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60 > <4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0 > <4>[11340.226929] [<c149ba78>] ? trace_hardirqs_on_thunk+0xc/0x10 > <4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb > <4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7 > <0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b 19 0= 1 74 > 2f b8 0a 00 00 >=20 > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> >=20 > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > index 531a93d..57f250c 100644 > --- a/net/bluetooth/sco.c > +++ b/net/bluetooth/sco.c > @@ -352,7 +352,7 @@ static void __sco_sock_close(struct sock *sk) >=20 > case BT_CONNECTED: > case BT_CONFIG: > - if (sco_pi(sk)->conn) { > + if (sco_pi(sk)->conn->hcon) { Your fix is incomplete, at least it should be: if ( (sco_pi(sk)->conn) && (sco_pi(sk)->conn->hcon)) { Otherwise, it will bring another crash case. So could you add signed-off-by= me also? Although it is not easy to reproduce, thanks. Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > sk->sk_state =3D BT_DISCONN; > sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); > hci_conn_put(sco_pi(sk)->conn->hcon); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case 2013-01-04 0:55 ` Liu, Chuansheng @ 2013-01-09 20:34 ` Gustavo Padovan 2013-01-10 0:26 ` Liu, Chuansheng 0 siblings, 1 reply; 5+ messages in thread From: Gustavo Padovan @ 2013-01-09 20:34 UTC (permalink / raw) To: Liu, Chuansheng Cc: marcel@holtmann.org, johan.hedberg@gmail.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org Hi Liu, * Liu, Chuansheng <chuansheng.liu@intel.com> [2013-01-04 00:55:26 +0000]: > > > > -----Original Message----- > > From: Gustavo Padovan [mailto:gustavo@padovan.org] > > Sent: Friday, January 04, 2013 6:03 AM > > To: Liu, Chuansheng > > Cc: marcel@holtmann.org; johan.hedberg@gmail.com; > > linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in > > shutdown case > > > > Hi Chuansheng, > > > > * Chuansheng Liu <chuansheng.liu@intel.com> [2012-12-25 18:04:17 +0800]: > > > > > > > > Meet one panic issue as below stack: > > > > > Disassemble the code: > > > base address of __sco_sock_close is 0xc184f410 > > > 0xc184f4f8 <+232>: lock decl 0x8(%ebx) < == crash here, ebx is 0x0, > > > > > > the related source code is: > > > (gdb) l *0xc184f4f8 > > > 0xc184f4f8 is in __sco_sock_close (arch/x86/include/asm/atomic.h:123) > > > 119 static inline int atomic_dec_and_test(atomic_t *v) > > > 123 asm volatile(LOCK_PREFIX "decl %0; sete %1" > > > > > > The whole call stack is: > > > sys_shutdown() > > > sco_sock_shutdown() > > > __sco_sock_close() > > > hci_conn_put() > > > atomic_dec_and_test() > > > > > > Due to the conn->hcon is NULL, and the member hcon->refcnt is at offset 0x8, > > > so "BUG: unable to handle kernel NULL pointer dereference at 00000008" > > > appears. > Could you add the above crash info to indicate where crashed? Thanks. > > > > > > > Here fix it that adding the condition if conn->hcon is NULL, just like > > > in sco_chan_del(). > > > > > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > > > --- > > > net/bluetooth/sco.c | 6 ++++-- > > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > index 531a93d..190f70c 100644 > > > --- a/net/bluetooth/sco.c > > > +++ b/net/bluetooth/sco.c > > > @@ -355,8 +355,10 @@ static void __sco_sock_close(struct sock *sk) > > > if (sco_pi(sk)->conn) { > > > sk->sk_state = BT_DISCONN; > > > sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); > > > - hci_conn_put(sco_pi(sk)->conn->hcon); > > > - sco_pi(sk)->conn->hcon = NULL; > > > + if (sco_pi(sk)->conn->hcon) { > > > + hci_conn_put(sco_pi(sk)->conn->hcon); > > > + sco_pi(sk)->conn->hcon = NULL; > > > + } > > > } else > > > sco_chan_del(sk, ECONNRESET); > > > break; > > > > Please check if the following patch fixes the issue for you: > > > > commit ae5668c1fc155d3034d0eedcdb52798390975a39 (HEAD, master) > > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > Date: Thu Jan 3 19:59:28 2013 -0200 > > > > Bluetooth: Check if the hci connection exists in SCO shutdown > > > > Checking only for sco_conn seems to not be enough and lead to NULL > > dereferences in the code, check for hcon instead. > > > > <1>[11340.226404] BUG: unable to handle kernel NULL pointer > > dereference at > > 0000000 > > 8 > > <4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0 > > <4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX: > > 00000000 > > <4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP: > > e0fdff1c > > <0>[11340.226674] Stack: > > <4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 c1754aef > > dec83e00 > > 00000000 > > e0fdff5c > > <4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c > > c1751852 > > d7813800 > > 62262f10 > > <4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 0000000d > > e0fdffac > > c175425c > > 00000041 > > <0>[11340.226793] Call Trace: > > <4>[11340.226813] [<c184db87>] ? sco_sock_clear_timer+0x27/0x60 > > <4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0 > > <4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60 > > <4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0 > > <4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80 > > <4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60 > > <4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0 > > <4>[11340.226929] [<c149ba78>] ? trace_hardirqs_on_thunk+0xc/0x10 > > <4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb > > <4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7 > > <0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b 19 01 74 > > 2f b8 0a 00 00 > > > > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > index 531a93d..57f250c 100644 > > --- a/net/bluetooth/sco.c > > +++ b/net/bluetooth/sco.c > > @@ -352,7 +352,7 @@ static void __sco_sock_close(struct sock *sk) > > > > case BT_CONNECTED: > > case BT_CONFIG: > > - if (sco_pi(sk)->conn) { > > + if (sco_pi(sk)->conn->hcon) { > Your fix is incomplete, at least it should be: > if ( (sco_pi(sk)->conn) && (sco_pi(sk)->conn->hcon)) { > Otherwise, it will bring another crash case. So could you add signed-off-by me also? Can you point any code flow that can crash with my patch? Otherwise I'm just pushing this patch. I don't think we need to check for sco_pi(sk)->conn here. Gustavo ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case 2013-01-09 20:34 ` Gustavo Padovan @ 2013-01-10 0:26 ` Liu, Chuansheng 0 siblings, 0 replies; 5+ messages in thread From: Liu, Chuansheng @ 2013-01-10 0:26 UTC (permalink / raw) To: Gustavo Padovan Cc: marcel@holtmann.org, johan.hedberg@gmail.com, linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org Hi Gustavo, > -----Original Message----- > From: Gustavo Padovan [mailto:gustavo@padovan.org] > Sent: Thursday, January 10, 2013 4:35 AM > To: Liu, Chuansheng > Cc: marcel@holtmann.org; johan.hedberg@gmail.com; > linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon =3D=3D NUL= L in > shutdown case >=20 > Hi Liu, >=20 > * Liu, Chuansheng <chuansheng.liu@intel.com> [2013-01-04 00:55:26 +0000]: >=20 > > > > > > > -----Original Message----- > > > From: Gustavo Padovan [mailto:gustavo@padovan.org] > > > Sent: Friday, January 04, 2013 6:03 AM > > > To: Liu, Chuansheng > > > Cc: marcel@holtmann.org; johan.hedberg@gmail.com; > > > linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org > > > Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon =3D=3D= NULL in > > > shutdown case > > > > > > Hi Chuansheng, > > > > > > * Chuansheng Liu <chuansheng.liu@intel.com> [2012-12-25 18:04:17 > +0800]: > > > > > > > > > > > Meet one panic issue as below stack: > > > > > > > > Disassemble the code: > > > > base address of __sco_sock_close is 0xc184f410 > > > > 0xc184f4f8 <+232>: lock decl 0x8(%ebx) < =3D=3D crash here, ebx i= s 0x0, > > > > > > > > the related source code is: > > > > (gdb) l *0xc184f4f8 > > > > 0xc184f4f8 is in __sco_sock_close (arch/x86/include/asm/atomic.h:12= 3) > > > > 119 static inline int atomic_dec_and_test(atomic_t *v) > > > > 123 asm volatile(LOCK_PREFIX "decl %0; sete %1" > > > > > > > > The whole call stack is: > > > > sys_shutdown() > > > > sco_sock_shutdown() > > > > __sco_sock_close() > > > > hci_conn_put() > > > > atomic_dec_and_test() > > > > > > > > Due to the conn->hcon is NULL, and the member hcon->refcnt is at of= fset > 0x8, > > > > so "BUG: unable to handle kernel NULL pointer dereference at 000000= 08" > > > > appears. > > Could you add the above crash info to indicate where crashed? Thanks. > > > > > > > > > > Here fix it that adding the condition if conn->hcon is NULL, just l= ike > > > > in sco_chan_del(). > > > > > > > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com> > > > > --- > > > > net/bluetooth/sco.c | 6 ++++-- > > > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > > index 531a93d..190f70c 100644 > > > > --- a/net/bluetooth/sco.c > > > > +++ b/net/bluetooth/sco.c > > > > @@ -355,8 +355,10 @@ static void __sco_sock_close(struct sock *sk) > > > > if (sco_pi(sk)->conn) { > > > > sk->sk_state =3D BT_DISCONN; > > > > sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT); > > > > - hci_conn_put(sco_pi(sk)->conn->hcon); > > > > - sco_pi(sk)->conn->hcon =3D NULL; > > > > + if (sco_pi(sk)->conn->hcon) { > > > > + hci_conn_put(sco_pi(sk)->conn->hcon); > > > > + sco_pi(sk)->conn->hcon =3D NULL; > > > > + } > > > > } else > > > > sco_chan_del(sk, ECONNRESET); > > > > break; > > > > > > Please check if the following patch fixes the issue for you: > > > > > > commit ae5668c1fc155d3034d0eedcdb52798390975a39 (HEAD, master) > > > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > Date: Thu Jan 3 19:59:28 2013 -0200 > > > > > > Bluetooth: Check if the hci connection exists in SCO shutdown > > > > > > Checking only for sco_conn seems to not be enough and lead to NUL= L > > > dereferences in the code, check for hcon instead. > > > > > > <1>[11340.226404] BUG: unable to handle kernel NULL pointer > > > dereference at > > > 0000000 > > > 8 > > > <4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0 > > > <4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX: > > > 00000000 > > > <4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP: > > > e0fdff1c > > > <0>[11340.226674] Stack: > > > <4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 > c1754aef > > > dec83e00 > > > 00000000 > > > e0fdff5c > > > <4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c > > > c1751852 > > > d7813800 > > > 62262f10 > > > <4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 > 0000000d > > > e0fdffac > > > c175425c > > > 00000041 > > > <0>[11340.226793] Call Trace: > > > <4>[11340.226813] [<c184db87>] ? > sco_sock_clear_timer+0x27/0x60 > > > <4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0 > > > <4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60 > > > <4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0 > > > <4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80 > > > <4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60 > > > <4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0 > > > <4>[11340.226929] [<c149ba78>] ? > trace_hardirqs_on_thunk+0xc/0x10 > > > <4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb > > > <4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7 > > > <0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b = 19 > 01 74 > > > 2f b8 0a 00 00 > > > > > > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > > > > > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c > > > index 531a93d..57f250c 100644 > > > --- a/net/bluetooth/sco.c > > > +++ b/net/bluetooth/sco.c > > > @@ -352,7 +352,7 @@ static void __sco_sock_close(struct sock *sk) > > > > > > case BT_CONNECTED: > > > case BT_CONFIG: > > > - if (sco_pi(sk)->conn) { > > > + if (sco_pi(sk)->conn->hcon) { > > Your fix is incomplete, at least it should be: > > if ( (sco_pi(sk)->conn) && (sco_pi(sk)->conn->hcon)) { > > Otherwise, it will bring another crash case. So could you add signed-of= f-by me > also? >=20 > Can you point any code flow that can crash with my patch? Otherwise I'm j= ust > pushing this patch. I don't think we need to check for sco_pi(sk)->conn h= ere. My theory is the old code if(sco_pi(sk)->conn) is already there, unless you= think it is useless and impossible. Just a code review for me, if you think your patch is still OK, please push= ing it, thanks. And I will try it to reproduce if possible:( >=20 > Gustavo ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-10 0:26 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-12-25 10:04 [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case Chuansheng Liu 2013-01-03 22:02 ` Gustavo Padovan 2013-01-04 0:55 ` Liu, Chuansheng 2013-01-09 20:34 ` Gustavo Padovan 2013-01-10 0:26 ` Liu, Chuansheng
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).