Re: [PATCH] Kernel crash when krfcomm is preempted by l2cap tasklet

[Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3564 bytes --]

Hi Gustavo,

Thanks for reviewing the patch. Please check new patches attached and
comments below:

On Sun, May 16, 2010 at 11:31 AM, Gustavo F. Padovan
<gustavo@padovan.org> wrote:
> Hi Andrei,
>
> * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-05-14 18:39:40 +0300]:
>
>> Hi all,
>>
>> We have a bug with race condition between l2cap tasklet and krfcomm process.
>>
>> When sending following sequence:
>>
>> ...
>> No.     Time        Source                Destination           Protocol Info
>>      89 1.951202            RFCOMM   Rcvd DISC DLCI=20
>>      90 1.951324            RFCOMM   Sent UA DLCI=20
>>      91 1.959381            HCI_EVT   Number of Completed Packets
>>      92 1.966461            RFCOMM   Rcvd DISC DLCI=0
>>      93 1.966492            L2CAP    Rcvd Disconnect Request
>>      94 1.972595            L2CAP    Sent Disconnect Response
>>
>> ...
>>
>> krfcommd kernel thread is preempted with l2cap tasklet which remove l2cap_conn
>> (L2CAP connection handler structure). Then rfcomm thread tries to send RFCOMM
>> UA which is reply to RFCOMM DISC and when de-referencing l2cap_conn crash
>> happens.
>>
>> ...
>> [  694.175933] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000000
>> [  694.184936] pgd = c0004000
>> [  694.187683] [00000000] *pgd=00000000
>> [  694.191711] Internal error: Oops: 5 [#1] PREEMPT
>> [  694.196350] last sysfs file:
>> /sys/devices/platform/hci_h4p/firmware/hci_h4p/loading
>> [  694.260375] CPU: 0    Not tainted  (2.6.32.10 #1)
>> [  694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
>> [  694.270721] LR is at 0xd7017303
>> ...
>> [  694.525085] Backtrace:
>> [  694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap])
>> from [<c02f2cc8>] (sock_sendmsg+0xb8
>> [  694.537292] [<c02f2c10>] (sock_sendmsg+0x0/0xd8) from [<c02f3044>]
>> (kernel_sendmsg+0x48/0x80)
>> ...
>>
>> I have a patch which fixes the issue but not sure that there is no
>> better way. Waiting for comments.
>>
>> Currently I am investigating possibility to:
>> - implement l2cap_conn reference counting
>
> sock_owned_by_user() gives the same effect as a ref count. See comments below.
>

Yes. For this cases this is enough. But proper refcounting is always
better than this "effects" ;)

>> - use socket backlog queue to process l2cap packets later when socket is not
>> owned by the process.
>
> You actually don't need a backlog queue here. You can process the signal
> packet, skipping the l2cap_chan_del() call;
>
...
>> +     /* sk is locked in krfcomm process */
>> +     if (sock_owned_by_user(sk)) {
>> +             BT_DBG("sk %p is owned by user", sk);
>> +             bh_unlock_sock(sk);
>> +             return 0;
>> +     }
>
> That's wrong. Use the sock_owned_by_user() only to avoid the
> l2cap_chan_del() call, so you can process all the rest of the function
> and send the Disconnect Response.

I have changed the patch so that it only prevents l2cap_chan_del
-		l2cap_chan_del(sk, ECONNREFUSED);
+		/* delete l2cap channel if sk is not owned by user */
+		if (!sock_owned_by_user(sk))
+			l2cap_chan_del(sk, ECONNREFUSED);

Then still l2cap_sock_kill(sk) removes sk but in the second patch sk is
hold with sock_hold(sk) until rfcomm is still processing packet.

> The same check should be added to l2cap_connect_rsp() and
> l2cap_disconnect_rsp(), since they call l2cap_chan_del() under bh
> context as well. ;)

Added.

Regards,
Andrei Emeltchenko

[-- Attachment #2: 0001-Bluetooth-Check-sk-is-not-owned-before-freeing-l2cap.patch --]
[-- Type: text/x-patch, Size: 2654 bytes --]

From 8531c8add8a378fc24b92f2c2311b01a7b1f63fc Mon Sep 17 00:00:00 2001
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Date: Mon, 17 May 2010 12:10:46 +0300
Subject: [PATCH 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

Check that socket sk is not locked in user process before removing
l2cap connection handler.

krfcommd kernel thread may be preempted with l2cap tasklet which remove
l2cap_conn structure. If krfcommd is in process of sending of RFCOMM reply
(like "RFCOMM UA" reply to "RFCOMM DISC") then kernel crash happens.

...
[  694.175933] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[  694.184936] pgd = c0004000
[  694.187683] [00000000] *pgd=00000000
[  694.191711] Internal error: Oops: 5 [#1] PREEMPT
[  694.196350] last sysfs file: /sys/devices/platform/hci_h4p/firmware/hci_h4p/loading
[  694.260375] CPU: 0    Not tainted  (2.6.32.10 #1)
[  694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
[  694.270721] LR is at 0xd7017303
...
[  694.525085] Backtrace:
[  694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap]) from [<c02f2cc8>] (sock_sendmsg+0xb8/0xd8)
[  694.537292] [<c02f2c10>] (sock_sendmsg+0x0/0xd8) from [<c02f3044>] (kernel_sendmsg+0x48/0x80)
...

Modified version after comments of Gustavo F. Padovan <gustavo@padovan.org>

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 net/bluetooth/l2cap.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index bb00015..794f2b7 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2927,7 +2927,9 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		break;
 
 	default:
-		l2cap_chan_del(sk, ECONNREFUSED);
+		/* delete l2cap channel if sk is not owned by user */
+		if (!sock_owned_by_user(sk))
+			l2cap_chan_del(sk, ECONNREFUSED);
 		break;
 	}
 
@@ -3135,7 +3137,10 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 		del_timer(&l2cap_pi(sk)->ack_timer);
 	}
 
-	l2cap_chan_del(sk, ECONNRESET);
+	/* delete l2cap channel if sk is not owned by user */
+	if (!sock_owned_by_user(sk))
+		l2cap_chan_del(sk, ECONNRESET);
+
 	bh_unlock_sock(sk);
 
 	l2cap_sock_kill(sk);
@@ -3167,7 +3172,10 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 		del_timer(&l2cap_pi(sk)->ack_timer);
 	}
 
-	l2cap_chan_del(sk, 0);
+	/* delete l2cap channel if sk is not owned by user */
+	if (!sock_owned_by_user(sk))
+		l2cap_chan_del(sk, 0);
+
 	bh_unlock_sock(sk);
 
 	l2cap_sock_kill(sk);
-- 
1.7.0.4


[-- Attachment #3: 0002-Bluetooth-Prevent-sk-freeing-in-tasklet-using-refcou.patch --]
[-- Type: text/x-patch, Size: 1066 bytes --]

From 99ffb4e41ff46022cbc52f3c947425791de7fa39 Mon Sep 17 00:00:00 2001
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Date: Mon, 17 May 2010 12:28:06 +0300
Subject: [PATCH 2/2] Bluetooth: Prevent sk freeing in tasklet using refcount

Socket sk may be freed in tasklet while still be in use in krfcommd
process. Use refcount to mark sk as used.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
 net/bluetooth/l2cap.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 794f2b7..bf762d6 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -1724,6 +1724,7 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
 	if (msg->msg_flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
+	sock_hold(sk);
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_CONNECTED) {
@@ -1808,6 +1809,7 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
 
 done:
 	release_sock(sk);
+	sock_put(sk);
 	return err;
 }
 
-- 
1.7.0.4