RE: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Gustavo,

> -----Original Message-----
> From: Gustavo Padovan [mailto:gustavo@xxxxxxxxxxx]
> Sent: Thursday, January 10, 2013 4:35 AM
> To: Liu, Chuansheng
> Cc: marcel@xxxxxxxxxxxx; johan.hedberg@xxxxxxxxx;
> linux-bluetooth@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in
> shutdown case
> 
> Hi Liu,
> 
> * Liu, Chuansheng <chuansheng.liu@xxxxxxxxx> [2013-01-04 00:55:26 +0000]:
> 
> >
> >
> > > -----Original Message-----
> > > From: Gustavo Padovan [mailto:gustavo@xxxxxxxxxxx]
> > > Sent: Friday, January 04, 2013 6:03 AM
> > > To: Liu, Chuansheng
> > > Cc: marcel@xxxxxxxxxxxx; johan.hedberg@xxxxxxxxx;
> > > linux-bluetooth@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in
> > > shutdown case
> > >
> > > Hi Chuansheng,
> > >
> > > * Chuansheng Liu <chuansheng.liu@xxxxxxxxx> [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@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxxxxxx>
> > > 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@xxxxxxxxx>
> > >     Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> > >
> > > 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.
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 pushing it, thanks.
And I will try it to reproduce if possible:(
> 
> 	Gustavo
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux