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