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

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

[Andrei Emeltchenko]

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

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
- use socket backlog queue to process l2cap packets later when socket is not
owned by the process.

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

From 955a821e1ee66cd6f9717ea4a2e9b3dfdafdc22a Mon Sep 17 00:00:00 2001
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Date: Fri, 14 May 2010 17:56:39 +0300
Subject: [PATCH] Bluetooth: Check sk is not used before freeing

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

rfcomm kernel thread may be 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
can happen.

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

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

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index bb00015..7eb9a58 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3119,6 +3119,13 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 	if (!sk)
 		return 0;
 
+	/* 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;
+	}
+
 	rsp.dcid = cpu_to_le16(l2cap_pi(sk)->scid);
 	rsp.scid = cpu_to_le16(l2cap_pi(sk)->dcid);
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
-- 
1.7.0.4

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

[Gustavo F. Padovan]

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.

> - 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;

> From 955a821e1ee66cd6f9717ea4a2e9b3dfdafdc22a Mon Sep 17 00:00:00 2001
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> Date: Fri, 14 May 2010 17:56:39 +0300
> Subject: [PATCH] Bluetooth: Check sk is not used before freeing
> 
> Check that socket sk is not locked in user process before removing
> l2cap connection handler and sk.
> 
> rfcomm kernel thread may be 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
> can happen.
> 
> ...
> [  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)
> ...
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
>  net/bluetooth/l2cap.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index bb00015..7eb9a58 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -3119,6 +3119,13 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
>  	if (!sk)
>  		return 0;
>  
> +	/* 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.

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. ;)

>  	rsp.dcid = cpu_to_le16(l2cap_pi(sk)->scid);
>  	rsp.scid = cpu_to_le16(l2cap_pi(sk)->dcid);
>  	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
> -- 
> 1.7.0.4
> 

-- 
Gustavo F. Padovan
http://padovan.org

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

[Andrei Emeltchenko]

[-- 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

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

[Gustavo F. Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-05-17 12:45:=
50 +0300]:

> Hi Gustavo,
>=20
> Thanks for reviewing the patch. Please check new patches attached and
> comments below:
>=20
> 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 pr=
ocess.
> >>
> >> When sending following sequence:
> >>
> >> ...
> >> No. =A0 =A0 Time =A0 =A0 =A0 =A0Source =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
Destination =A0 =A0 =A0 =A0 =A0 Protocol Info
> >> =A0 =A0 =A089 1.951202 =A0 =A0 =A0 =A0 =A0 =A0RFCOMM =A0 Rcvd DISC DLC=
I=3D20
> >> =A0 =A0 =A090 1.951324 =A0 =A0 =A0 =A0 =A0 =A0RFCOMM =A0 Sent UA DLCI=
=3D20
> >> =A0 =A0 =A091 1.959381 =A0 =A0 =A0 =A0 =A0 =A0HCI_EVT =A0 Number of Co=
mpleted Packets
> >> =A0 =A0 =A092 1.966461 =A0 =A0 =A0 =A0 =A0 =A0RFCOMM =A0 Rcvd DISC DLC=
I=3D0
> >> =A0 =A0 =A093 1.966492 =A0 =A0 =A0 =A0 =A0 =A0L2CAP =A0 =A0Rcvd Discon=
nect Request
> >> =A0 =A0 =A094 1.972595 =A0 =A0 =A0 =A0 =A0 =A0L2CAP =A0 =A0Sent Discon=
nect Response
> >>
> >> ...
> >>
> >> krfcommd kernel thread is preempted with l2cap tasklet which remove l2=
cap_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 cr=
ash
> >> happens.
> >>
> >> ...
> >> [ =A0694.175933] Unable to handle kernel NULL pointer dereference at
> >> virtual address 00000000
> >> [ =A0694.184936] pgd =3D c0004000
> >> [ =A0694.187683] [00000000] *pgd=3D00000000
> >> [ =A0694.191711] Internal error: Oops: 5 [#1] PREEMPT
> >> [ =A0694.196350] last sysfs file:
> >> /sys/devices/platform/hci_h4p/firmware/hci_h4p/loading
> >> [ =A0694.260375] CPU: 0 =A0 =A0Not tainted =A0(2.6.32.10 #1)
> >> [ =A0694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
> >> [ =A0694.270721] LR is at 0xd7017303
> >> ...
> >> [ =A0694.525085] Backtrace:
> >> [ =A0694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap])
> >> from [<c02f2cc8>] (sock_sendmsg+0xb8
> >> [ =A0694.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.
> >
>=20
> Yes. For this cases this is enough. But proper refcounting is always
> better than this "effects" ;)
>=20
> >> - 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;
> >
> ...
> >> + =A0 =A0 /* sk is locked in krfcomm process */
> >> + =A0 =A0 if (sock_owned_by_user(sk)) {
> >> + =A0 =A0 =A0 =A0 =A0 =A0 BT_DBG("sk %p is owned by user", sk);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 bh_unlock_sock(sk);
> >> + =A0 =A0 =A0 =A0 =A0 =A0 return 0;
> >> + =A0 =A0 }
> >
> > 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.
>=20
> 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);
>=20
> 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.

Hmm, I missed the l2cap_sock_kill(). So I guess you can invert the if
condition and unlock the socket and return if sock is owned by user,
right?

Marcel, do you agree with that change?

>=20
> > 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. ;)
>=20
> Added.
>=20
> Regards,
> Andrei Emeltchenko

> 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 l2ca=
p_conn
>=20
> Check that socket sk is not locked in user process before removing
> l2cap connection handler.
>=20
> 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.
>=20
> ...
> [  694.175933] Unable to handle kernel NULL pointer dereference at virtua=
l address 00000000
> [  694.184936] pgd =3D c0004000
> [  694.187683] [00000000] *pgd=3D00000000
> [  694.191711] Internal error: Oops: 5 [#1] PREEMPT
> [  694.196350] last sysfs file: /sys/devices/platform/hci_h4p/firmware/hc=
i_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>] (ke=
rnel_sendmsg+0x48/0x80)
> ...
>=20
> Modified version after comments of Gustavo F. Padovan <gustavo@padovan.or=
g>
>=20
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
>  net/bluetooth/l2cap.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
>=20
> 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_co=
nn *conn, struct l2cap_cmd_hd
>  		break;
> =20
>  	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;
>  	}
> =20
> @@ -3135,7 +3137,10 @@ static inline int l2cap_disconnect_req(struct l2ca=
p_conn *conn, struct l2cap_cmd
>  		del_timer(&l2cap_pi(sk)->ack_timer);
>  	}
> =20
> -	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);
> =20
>  	l2cap_sock_kill(sk);
> @@ -3167,7 +3172,10 @@ static inline int l2cap_disconnect_rsp(struct l2ca=
p_conn *conn, struct l2cap_cmd
>  		del_timer(&l2cap_pi(sk)->ack_timer);
>  	}
> =20
> -	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);
> =20
>  	l2cap_sock_kill(sk);
> --=20
> 1.7.0.4
>=20

> 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 refco=
unt
>=20
> Socket sk may be freed in tasklet while still be in use in krfcommd
> process. Use refcount to mark sk as used.
>=20
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
> ---
>  net/bluetooth/l2cap.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>=20
> 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, s=
truct socket *sock, struct ms
>  	if (msg->msg_flags & MSG_OOB)
>  		return -EOPNOTSUPP;
> =20
> +	sock_hold(sk);
>  	lock_sock(sk);
> =20
>  	if (sk->sk_state !=3D BT_CONNECTED) {
> @@ -1808,6 +1809,7 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, s=
truct socket *sock, struct ms
> =20
>  done:
>  	release_sock(sk);
> +	sock_put(sk);
>  	return err;
>  }
> =20
> --=20
> 1.7.0.4
>=20


--=20
Gustavo F. Padovan
http://padovan.org

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

[Andrei Emeltchenko]

Hi Gustavo,

On Mon, May 17, 2010 at 10:06 PM, Gustavo F. Padovan
<gustavo@padovan.org> wrote:
>> >> We have a bug with race condition between l2cap tasklet and krfcomm p=
rocess.
>> >>
>> >> When sending following sequence:
>> >>
>> >> ...
>> >> No. =A0 =A0 Time =A0 =A0 =A0 =A0Source =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0Destination =A0 =A0 =A0 =A0 =A0 Protocol Info
>> >> =A0 =A0 =A089 1.951202 =A0 =A0 =A0 =A0 =A0 =A0RFCOMM =A0 Rcvd DISC DL=
CI=3D20
>> >> =A0 =A0 =A090 1.951324 =A0 =A0 =A0 =A0 =A0 =A0RFCOMM =A0 Sent UA DLCI=
=3D20
>> >> =A0 =A0 =A091 1.959381 =A0 =A0 =A0 =A0 =A0 =A0HCI_EVT =A0 Number of C=
ompleted Packets
>> >> =A0 =A0 =A092 1.966461 =A0 =A0 =A0 =A0 =A0 =A0RFCOMM =A0 Rcvd DISC DL=
CI=3D0
>> >> =A0 =A0 =A093 1.966492 =A0 =A0 =A0 =A0 =A0 =A0L2CAP =A0 =A0Rcvd Disco=
nnect Request
>> >> =A0 =A0 =A094 1.972595 =A0 =A0 =A0 =A0 =A0 =A0L2CAP =A0 =A0Sent Disco=
nnect Response
>> >>
>> >> ...
>> >>
>> >> krfcommd kernel thread is preempted with l2cap tasklet which remove l=
2cap_conn
>> >> (L2CAP connection handler structure). Then rfcomm thread tries to sen=
d RFCOMM
>> >> UA which is reply to RFCOMM DISC and when de-referencing l2cap_conn c=
rash
>> >> happens.
>> >>
>> >> ...
>> >> [ =A0694.175933] Unable to handle kernel NULL pointer dereference at
>> >> virtual address 00000000
>> >> [ =A0694.184936] pgd =3D c0004000
>> >> [ =A0694.187683] [00000000] *pgd=3D00000000
>> >> [ =A0694.191711] Internal error: Oops: 5 [#1] PREEMPT
>> >> [ =A0694.196350] last sysfs file:
>> >> /sys/devices/platform/hci_h4p/firmware/hci_h4p/loading
>> >> [ =A0694.260375] CPU: 0 =A0 =A0Not tainted =A0(2.6.32.10 #1)
>> >> [ =A0694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
>> >> [ =A0694.270721] LR is at 0xd7017303
>> >> ...
>> >> [ =A0694.525085] Backtrace:
>> >> [ =A0694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap])
>> >> from [<c02f2cc8>] (sock_sendmsg+0xb8
>> >> [ =A0694.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 comment=
s 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 sign=
al
>> > packet, skipping the l2cap_chan_del() call;
>> >
>> ...
>> >> + =A0 =A0 /* sk is locked in krfcomm process */
>> >> + =A0 =A0 if (sock_owned_by_user(sk)) {
>> >> + =A0 =A0 =A0 =A0 =A0 =A0 BT_DBG("sk %p is owned by user", sk);
>> >> + =A0 =A0 =A0 =A0 =A0 =A0 bh_unlock_sock(sk);
>> >> + =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>> >> + =A0 =A0 }
>> >
>> > 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
>> - =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_del(sk, ECONNREFUSED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* delete l2cap channel if sk is not owned by =
user */
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (!sock_owned_by_user(sk))
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_del(sk, ECONNREFUSE=
D);
>>
>> 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.
>
> Hmm, I missed the l2cap_sock_kill(). So I guess you can invert the if
> condition and unlock the socket and return if sock is owned by user,
> right?

I am still thinking that my proposal is better.

l2cap_sock_kill(sk) does not delete socket but unlink it from the list
and defer deleting
to the time we get to sock_put(sk) in the other place sk is used.
Socket shall be deleted and this defer deletion is a good solution
IMO.

But I can also create patch with your proposed change.

> Marcel, do you agree with that change?

So what would be the right solution?

Regards,
Andrei

>
>>
>> > 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
>
>> 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 l2c=
ap_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 rep=
ly
>> (like "RFCOMM UA" reply to "RFCOMM DISC") then kernel crash happens.
>>
>> ...
>> [ =A0694.175933] Unable to handle kernel NULL pointer dereference at vir=
tual address 00000000
>> [ =A0694.184936] pgd =3D c0004000
>> [ =A0694.187683] [00000000] *pgd=3D00000000
>> [ =A0694.191711] Internal error: Oops: 5 [#1] PREEMPT
>> [ =A0694.196350] last sysfs file: /sys/devices/platform/hci_h4p/firmware=
/hci_h4p/loading
>> [ =A0694.260375] CPU: 0 =A0 =A0Not tainted =A0(2.6.32.10 #1)
>> [ =A0694.265106] PC is at l2cap_sock_sendmsg+0x43c/0x73c [l2cap]
>> [ =A0694.270721] LR is at 0xd7017303
>> ...
>> [ =A0694.525085] Backtrace:
>> [ =A0694.527587] [<bf266be0>] (l2cap_sock_sendmsg+0x0/0x73c [l2cap]) fro=
m [<c02f2cc8>] (sock_sendmsg+0xb8/0xd8)
>> [ =A0694.537292] [<c02f2c10>] (sock_sendmsg+0x0/0xd8) from [<c02f3044>] =
(kernel_sendmsg+0x48/0x80)
>> ...
>>
>> Modified version after comments of Gustavo F. Padovan <gustavo@padovan.o=
rg>
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> ---
>> =A0net/bluetooth/l2cap.c | =A0 14 +++++++++++---
>> =A01 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_c=
onn *conn, struct l2cap_cmd_hd
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>>
>> =A0 =A0 =A0 default:
>> - =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_del(sk, ECONNREFUSED);
>> + =A0 =A0 =A0 =A0 =A0 =A0 /* delete l2cap channel if sk is not owned by =
user */
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (!sock_owned_by_user(sk))
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_del(sk, ECONNREFUSE=
D);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
>> =A0 =A0 =A0 }
>>
>> @@ -3135,7 +3137,10 @@ static inline int l2cap_disconnect_req(struct l2c=
ap_conn *conn, struct l2cap_cmd
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 del_timer(&l2cap_pi(sk)->ack_timer);
>> =A0 =A0 =A0 }
>>
>> - =A0 =A0 l2cap_chan_del(sk, ECONNRESET);
>> + =A0 =A0 /* delete l2cap channel if sk is not owned by user */
>> + =A0 =A0 if (!sock_owned_by_user(sk))
>> + =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_del(sk, ECONNRESET);
>> +
>> =A0 =A0 =A0 bh_unlock_sock(sk);
>>
>> =A0 =A0 =A0 l2cap_sock_kill(sk);
>> @@ -3167,7 +3172,10 @@ static inline int l2cap_disconnect_rsp(struct l2c=
ap_conn *conn, struct l2cap_cmd
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 del_timer(&l2cap_pi(sk)->ack_timer);
>> =A0 =A0 =A0 }
>>
>> - =A0 =A0 l2cap_chan_del(sk, 0);
>> + =A0 =A0 /* delete l2cap channel if sk is not owned by user */
>> + =A0 =A0 if (!sock_owned_by_user(sk))
>> + =A0 =A0 =A0 =A0 =A0 =A0 l2cap_chan_del(sk, 0);
>> +
>> =A0 =A0 =A0 bh_unlock_sock(sk);
>>
>> =A0 =A0 =A0 l2cap_sock_kill(sk);
>> --
>> 1.7.0.4
>>
>
>> 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 refc=
ount
>>
>> 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>
>> ---
>> =A0net/bluetooth/l2cap.c | =A0 =A02 ++
>> =A01 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
>> =A0 =A0 =A0 if (msg->msg_flags & MSG_OOB)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EOPNOTSUPP;
>>
>> + =A0 =A0 sock_hold(sk);
>> =A0 =A0 =A0 lock_sock(sk);
>>
>> =A0 =A0 =A0 if (sk->sk_state !=3D BT_CONNECTED) {
>> @@ -1808,6 +1809,7 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, =
struct socket *sock, struct ms
>>
>> =A0done:
>> =A0 =A0 =A0 release_sock(sk);
>> + =A0 =A0 sock_put(sk);
>> =A0 =A0 =A0 return err;
>> =A0}
>>
>> --
>> 1.7.0.4
>>
>
>
> --
> Gustavo F. Padovan
> http://padovan.org
>

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

[Andrei Emeltchenko]

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

Hi All,

I have done second  experimental version of the patch.

Main difference is that when sk is owned by user I do not run
"l2cap_chan_del" example below:

+	/* don't delete l2cap channel if sk is owned by user */
+	if (sock_owned_by_user(sk)) {
+		sk->sk_state = BT_DISCONN;
+		l2cap_sock_clear_timer(sk);
+		l2cap_sock_set_timer(sk, HZ);
+		bh_unlock_sock(sk);
+		return 0;
+	}

I start timer which will run this function. I do also the same check
in timer context.

+	if (sock_owned_by_user(sk)) {
+		/* sk is owned by user. Try again later */
+		l2cap_sock_set_timer(sk, HZ * 2);
+		bh_unlock_sock(sk);
+		sock_put(sk);
+		return;
+	}

Please give me your comments.

Regards,
Andrei

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

From e0ab5b30d3c507224043f065c1c227f55ae29814 Mon Sep 17 00:00:00 2001
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Date: Mon, 17 May 2010 12:10:46 +0300
Subject: [PATCHv2 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 |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index bb00015..11060d6 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2927,6 +2927,13 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		break;
 
 	default:
+		/* don't delete l2cap channel if sk is owned by user */
+		if (sock_owned_by_user(sk)) {
+			sk->sk_state = BT_DISCONN;
+			l2cap_sock_clear_timer(sk);
+			l2cap_sock_set_timer(sk, HZ);
+			break;
+		}
 		l2cap_chan_del(sk, ECONNREFUSED);
 		break;
 	}
@@ -3135,6 +3142,15 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 		del_timer(&l2cap_pi(sk)->ack_timer);
 	}
 
+	/* don't delete l2cap channel if sk is owned by user */
+	if (sock_owned_by_user(sk)) {
+		sk->sk_state = BT_DISCONN;
+		l2cap_sock_clear_timer(sk);
+		l2cap_sock_set_timer(sk, HZ);
+		bh_unlock_sock(sk);
+		return 0;
+	}
+
 	l2cap_chan_del(sk, ECONNRESET);
 	bh_unlock_sock(sk);
 
@@ -3167,6 +3183,15 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 		del_timer(&l2cap_pi(sk)->ack_timer);
 	}
 
+	/* don't delete l2cap channel if sk is owned by user */
+	if (sock_owned_by_user(sk)) {
+		sk->sk_state = BT_DISCONN;
+		l2cap_sock_clear_timer(sk);
+		l2cap_sock_set_timer(sk, HZ);
+		bh_unlock_sock(sk);
+		return 0;
+	}
+
 	l2cap_chan_del(sk, 0);
 	bh_unlock_sock(sk);
 
-- 
1.7.0.4


[-- Attachment #3: 0002-Bluetooth-timer-check-sk-is-not-owned-before-freeing.patch --]
[-- Type: text/x-patch, Size: 1066 bytes --]

From 5adfee7a73883180e6b606cc46b792fbefbb6758 Mon Sep 17 00:00:00 2001
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Date: Thu, 20 May 2010 16:52:19 +0300
Subject: [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing

In timer context we might delete l2cap channel used by krfcommd.
The check makes sure that sk is not owned.

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

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 11060d6..19d2fc1 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -93,6 +93,14 @@ static void l2cap_sock_timeout(unsigned long arg)
 
 	bh_lock_sock(sk);
 
+	if (sock_owned_by_user(sk)) {
+		/* sk is owned by user. Try again later */
+		l2cap_sock_set_timer(sk, HZ * 2);
+		bh_unlock_sock(sk);
+		sock_put(sk);
+		return;
+	}
+
 	if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONFIG)
 		reason = ECONNREFUSED;
 	else if (sk->sk_state == BT_CONNECT &&
-- 
1.7.0.4

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

[Andrei Emeltchenko]

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

On Thu, May 20, 2010 at 5:13 PM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi All,
>
> I have done second  experimental version of the patch.
>
> Main difference is that when sk is owned by user I do not run
> "l2cap_chan_del" example below:
>
> +       /* don't delete l2cap channel if sk is owned by user */
> +       if (sock_owned_by_user(sk)) {
> +               sk->sk_state = BT_DISCONN;
> +               l2cap_sock_clear_timer(sk);
> +               l2cap_sock_set_timer(sk, HZ);
> +               bh_unlock_sock(sk);
> +               return 0;
> +       }
>
> I start timer which will run this function. I do also the same check
> in timer context.
>
> +       if (sock_owned_by_user(sk)) {
> +               /* sk is owned by user. Try again later */
> +               l2cap_sock_set_timer(sk, HZ * 2);
> +               bh_unlock_sock(sk);
> +               sock_put(sk);
> +               return;
> +       }

Little modification to the second patch attached.

-- Regards, Andrei

[-- Attachment #2: 0002-Bluetooth-timer-check-sk-is-not-owned-before-freeing.patch --]
[-- Type: text/x-patch, Size: 2241 bytes --]

From 99f3c96b432c2732dce8d84155a8be346e633a56 Mon Sep 17 00:00:00 2001
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Date: Thu, 20 May 2010 16:52:19 +0300
Subject: [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing

In timer context we might delete l2cap channel used by krfcommd.
The check makes sure that sk is not owned.

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

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 11060d6..1a6c5bb 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -84,6 +84,18 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn,
 				u8 code, u8 ident, u16 dlen, void *data);
 
 /* ---- L2CAP timers ---- */
+static void l2cap_sock_set_timer(struct sock *sk, long timeout)
+{
+	BT_DBG("sk %p state %d timeout %ld", sk, sk->sk_state, timeout);
+	sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
+}
+
+static void l2cap_sock_clear_timer(struct sock *sk)
+{
+	BT_DBG("sock %p state %d", sk, sk->sk_state);
+	sk_stop_timer(sk, &sk->sk_timer);
+}
+
 static void l2cap_sock_timeout(unsigned long arg)
 {
 	struct sock *sk = (struct sock *) arg;
@@ -93,6 +105,14 @@ static void l2cap_sock_timeout(unsigned long arg)
 
 	bh_lock_sock(sk);
 
+	if (sock_owned_by_user(sk)) {
+		/* sk is owned by user. Try again later */
+		l2cap_sock_set_timer(sk, HZ * 2);
+		bh_unlock_sock(sk);
+		sock_put(sk);
+		return;
+	}
+
 	if (sk->sk_state == BT_CONNECTED || sk->sk_state == BT_CONFIG)
 		reason = ECONNREFUSED;
 	else if (sk->sk_state == BT_CONNECT &&
@@ -109,18 +129,6 @@ static void l2cap_sock_timeout(unsigned long arg)
 	sock_put(sk);
 }
 
-static void l2cap_sock_set_timer(struct sock *sk, long timeout)
-{
-	BT_DBG("sk %p state %d timeout %ld", sk, sk->sk_state, timeout);
-	sk_reset_timer(sk, &sk->sk_timer, jiffies + timeout);
-}
-
-static void l2cap_sock_clear_timer(struct sock *sk)
-{
-	BT_DBG("sock %p state %d", sk, sk->sk_state);
-	sk_stop_timer(sk, &sk->sk_timer);
-}
-
 /* ---- L2CAP channels ---- */
 static struct sock *__l2cap_get_chan_by_dcid(struct l2cap_chan_list *l, u16 cid)
 {
-- 
1.7.0.4