Re: kernel panic happens when disconnecting Bluetooth headset

[Nick Pelly <npelly@google.com>]

On Tue, Dec 22, 2009 at 8:20 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi Marcel,
>
> On Sat, Dec 19, 2009 at 1:02 AM, Marcel Holtmann <marcel@holtmann.org> wr=
ote:
>> Hi Nick,
>>
>>> >> Processing a RFCOMM UA frame when the socket is closed and we were n=
ot
>>> >> the
>>> >> RFCOMM initiator would cause rfcomm_session_put() to be called twice
>>> >> during
>>> >> rfcomm_process_rx(). This would cause a kernel panic in
>>> >> rfcomm_session_close.
>>> >>
>>> >> This could be easily reproduced during disconnect with devices such =
as
>>> >> Motorola H270 that send RFCOMM UA followed quickly by L2CAP disconne=
ct
>>> >> request.
>>> >> This hcidump for this looks like:
>>> >>
>>> >> 2009-09-21 17:22:37.788895 < ACL data: handle 1 flags 0x02 dlen 8
>>> >> =A0 =A0L2CAP(d): cid 0x0041 len 4 [psm 3]
>>> >> =A0 =A0 =A0RFCOMM(s): DISC: cr 0 dlci 20 pf 1 ilen 0 fcs 0x7d
>>> >> 2009-09-21 17:22:37.906204 > HCI Event: Number of Completed Packets
>>> >> (0x13)
>>> >> plen 5
>>> >> =A0 =A0handle 1 packets 1
>>> >> 2009-09-21 17:22:37.933090 > ACL data: handle 1 flags 0x02 dlen 8
>>> >> =A0 =A0L2CAP(d): cid 0x0040 len 4 [psm 3]
>>> >> =A0 =A0 =A0RFCOMM(s): UA: cr 0 dlci 20 pf 1 ilen 0 fcs 0x57
>>> >> 2009-09-21 17:22:38.636764 < ACL data: handle 1 flags 0x02 dlen 8
>>> >> =A0 =A0L2CAP(d): cid 0x0041 len 4 [psm 3]
>>> >> =A0 =A0 =A0RFCOMM(s): DISC: cr 0 dlci 0 pf 1 ilen 0 fcs 0x9c
>>> >> 2009-09-21 17:22:38.744125 > HCI Event: Number of Completed Packets
>>> >> (0x13)
>>> >> plen 5
>>> >> =A0 =A0handle 1 packets 1
>>> >> 2009-09-21 17:22:38.763687 > ACL data: handle 1 flags 0x02 dlen 8
>>> >> =A0 =A0L2CAP(d): cid 0x0040 len 4 [psm 3]
>>> >> =A0 =A0 =A0RFCOMM(s): UA: cr 0 dlci 0 pf 1 ilen 0 fcs 0xb6
>>> >> 2009-09-21 17:22:38.783554 > ACL data: handle 1 flags 0x02 dlen 12
>>> >> =A0 =A0L2CAP(s): Disconn req: dcid 0x0040 scid 0x0041
>>> >>
>>> >> Avoid calling rfcomm_session_put() twice by skipping this call
>>> >> in rfcomm_recv_ua() if the socket is closed.
>>> >>
>>> >> Picked from:
>>> >> http://android.git.kernel.org/?p=3Dkernel/common.git;a=3Dcommit;h=3D=
1048e007842da2d6440679e1ca80f45438a6369d
>>> >>
>>> >> Signed-off-by: Nick Pelly <npelly@google.com>
>>> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>> >> ---
>>> >> =A0net/bluetooth/rfcomm/core.c | =A0 =A03 ++-
>>> >> =A01 files changed, 2 insertions(+), 1 deletions(-)
>>> >>
>>> >> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core=
.c
>>> >> index 0313e88..56ffcb8 100644
>>> >> --- a/net/bluetooth/rfcomm/core.c
>>> >> +++ b/net/bluetooth/rfcomm/core.c
>>> >> @@ -1148,7 +1148,8 @@ static int rfcomm_recv_ua(struct rfcomm_sessio=
n
>>> >> *s, u8 dlci)
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>>> >>
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 case BT_DISCONN:
>>> >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rfcomm_session_put(s);
>>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (s->sock->sk->sk_st=
ate !=3D BT_CLOSED)
>>> >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rfcomm=
_session_put(s);
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>>> >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>>> >> =A0 =A0 =A0 =A0 }
>>> >
>>> > I am not a big fan of conditionally decreasing reference counts. I do
>>> > think it would be better to fix this by holding an extra pair of
>>> > reference counts or actually fixing the imbalance. What about the oth=
er
>>> > patches I proposed?
>>>
>>> Your proposed patch was to add an extra hold() / put() reference count
>>> around the offending put(). I did test this patch, and found it does
>>> not fix the underlying imbalance, it just moves the kernel panic
>>> somewhere else.
>>>
>>> As best I can tell, my patch does address the underlying imbalance. It
>>> is in production on Android phones and seems to work well. As best I
>>> can tell, there is not a cleaner solution that does not involve
>>> significant refactoring of rfcomm refcounting.
>
> We have this patch also in Nokia N900 phone. And this was the best soluti=
on
> for the problem mentioned.
>
>> the RFCOMM reference counting is something nasty and it does need to be
>> re-written. One thing that needs to happen that we stop using the L2CAP
>> sockets directly. We have to put a proper L2CAP in-kernel specific API
>> in between that ensures we are not mixing things. That is the one issues
>> that we always had in this area.
>>
>> Before applying this patch, I like to have additionally a comment in
>> front of this conditional put call that explains a little bit the
>> problem area here. The long explanation with logs etc. should be in the
>> commit message. I have to make sure that we fully understand what is
>> going on here and why we did it.
>
> What do you think about following comment:
>
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -1151,7 +1151,11 @@ static int rfcomm_recv_ua(struct rfcomm_session
> *s, u8 dlci)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0case BT_DISCONN:
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rfcomm_session_put(s);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* When socket is closed an=
d we are not RFCOMM
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* initiator rfcomm_proce=
ss_rx already calls
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* rfcomm_session_put */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (s->sock->sk->sk_state !=
=3D BT_CLOSED)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rfcomm_sess=
ion_put(s);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0}
> --
>

Ping.

Nick