* [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap
@ 2010-11-03 10:32 Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Emeltchenko Andrei @ 2010-11-03 10:32 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
*v4: taken Gustavo comments about timer HZ -> HZ/5
Do not delete l2cap channel and socket sk when sk is owned by user.
To delete l2cap channel standard timer is used.
lock_sock and release_sock do not hold a normal spinlock directly but
instead hold the owner field. This means bh_lock_sock can still execute
even if the socket is "locked". More info can be found here:
http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
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.
Andrei Emeltchenko (2):
Bluetooth: Check sk is not owned before freeing l2cap_conn
Bluetooth: timer check sk is not owned before freeing
net/bluetooth/l2cap.c | 58 ++++++++++++++++++++++++++++++++++++++----------
1 files changed, 46 insertions(+), 12 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv4 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-11-03 10:32 [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Emeltchenko Andrei
@ 2010-11-03 10:32 ` Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 2/2] Bluetooth: timer check sk is not owned before freeing Emeltchenko Andrei
2010-11-10 5:36 ` [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Marcel Holtmann
2 siblings, 0 replies; 6+ messages in thread
From: Emeltchenko Andrei @ 2010-11-03 10:32 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
Check that socket sk is not locked in user process before removing
l2cap connection handler.
lock_sock and release_sock do not hold a normal spinlock directly but
instead hold the owner field. This means bh_lock_sock can still execute
even if the socket is "locked". More info can be found here:
http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
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)
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
---
net/bluetooth/l2cap.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 6f931cc..b5436f7 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3078,6 +3078,14 @@ 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 / 5);
+ break;
+ }
+
l2cap_chan_del(sk, ECONNREFUSED);
break;
}
@@ -3283,6 +3291,15 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
sk->sk_shutdown = SHUTDOWN_MASK;
+ /* 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 / 5);
+ bh_unlock_sock(sk);
+ return 0;
+ }
+
l2cap_chan_del(sk, ECONNRESET);
bh_unlock_sock(sk);
@@ -3305,6 +3322,15 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
if (!sk)
return 0;
+ /* 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 / 5);
+ bh_unlock_sock(sk);
+ return 0;
+ }
+
l2cap_chan_del(sk, 0);
bh_unlock_sock(sk);
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCHv4 2/2] Bluetooth: timer check sk is not owned before freeing
2010-11-03 10:32 [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
@ 2010-11-03 10:32 ` Emeltchenko Andrei
2010-11-10 5:36 ` [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Marcel Holtmann
2 siblings, 0 replies; 6+ messages in thread
From: Emeltchenko Andrei @ 2010-11-03 10:32 UTC (permalink / raw)
To: linux-bluetooth
From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
In timer context we might delete l2cap channel used by krfcommd.
The check makes sure that sk is not owned. If sk is owned we
restart timer for HZ/5.
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 b5436f7..b2f309d 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -83,6 +83,18 @@ static struct sk_buff *l2cap_build_cmd(struct l2cap_conn *conn,
static int l2cap_ertm_data_rcv(struct sock *sk, struct sk_buff *skb);
/* ---- 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;
@@ -92,6 +104,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 / 5);
+ 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 &&
@@ -108,18 +128,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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap
2010-11-03 10:32 [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 2/2] Bluetooth: timer check sk is not owned before freeing Emeltchenko Andrei
@ 2010-11-10 5:36 ` Marcel Holtmann
2010-11-10 15:24 ` Andrei Emeltchenko
2 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2010-11-10 5:36 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
Hi Andrei,
> Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
> *v4: taken Gustavo comments about timer HZ -> HZ/5
>
> Do not delete l2cap channel and socket sk when sk is owned by user.
> To delete l2cap channel standard timer is used.
>
> lock_sock and release_sock do not hold a normal spinlock directly but
> instead hold the owner field. This means bh_lock_sock can still execute
> even if the socket is "locked". More info can be found here:
> http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
>
> 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.
so I assume you have tested this extensively with various RFCOMM corner
cases like incoming RFCOMM. Since a lot of profiles require proper
disconnects and we have to ensure that our reference counting is
correct.
Other then that it seems fine to me.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap
2010-11-10 5:36 ` [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Marcel Holtmann
@ 2010-11-10 15:24 ` Andrei Emeltchenko
2010-11-10 16:32 ` Gustavo F. Padovan
0 siblings, 1 reply; 6+ messages in thread
From: Andrei Emeltchenko @ 2010-11-10 15:24 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
> Hi Andrei,
>
> > Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
> > *v4: taken Gustavo comments about timer HZ -> HZ/5
> >
> > Do not delete l2cap channel and socket sk when sk is owned by user.
> > To delete l2cap channel standard timer is used.
> >
> > lock_sock and release_sock do not hold a normal spinlock directly but
> > instead hold the owner field. This means bh_lock_sock can still execute
> > even if the socket is "locked". More info can be found here:
> > http://www.linuxfoundation.org/collaborate/workgroups/networking/socketlocks
> >
> > 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.
>
> so I assume you have tested this extensively with various RFCOMM corner
> cases like incoming RFCOMM. Since a lot of profiles require proper
> disconnects and we have to ensure that our reference counting is
> correct.
We have the slightly modified patch applied for a several months. No regression found.
Regards,
Andrei
>
> Other then that it seems fine to me.
>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>
> Regards
>
> Marcel
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap
2010-11-10 15:24 ` Andrei Emeltchenko
@ 2010-11-10 16:32 ` Gustavo F. Padovan
0 siblings, 0 replies; 6+ messages in thread
From: Gustavo F. Padovan @ 2010-11-10 16:32 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Marcel Holtmann, linux-bluetooth
Hi Andrei,
* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2010-11-10 17:24:=
54 +0200]:
> Hi Marcel,
>=20
> > Hi Andrei,
> >=20
> > > Yet another version of patches fixing kernel crash in RFCOMM / L2CAP.
> > > *v4: taken Gustavo comments about timer HZ -> HZ/5
> > >=20
> > > Do not delete l2cap channel and socket sk when sk is owned by user.
> > > To delete l2cap channel standard timer is used.
> > >=20
> > > lock_sock and release_sock do not hold a normal spinlock directly but=
=20
> > > instead hold the owner field. This means bh_lock_sock can still execu=
te
> > > even if the socket is "locked". More info can be found here:
> > > http://www.linuxfoundation.org/collaborate/workgroups/networking/sock=
etlocks
> > >=20
> > > When sending following sequence:
> > > ...
> > > No.=A0 =A0 =A0 =A0 Time=A0 =A0 =A0 =A0 =A0 =A0 =A0 Source=A0 =A0 =
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 Destination=A0 =A0 =A0=
=A0 =A0 =A0 =A0 =A0 =A0 =20
> > > Protocol Info 89 1.951202=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
RFCOMM=A0 =A0 Rcvd DISC DLCI=3D20
> > > 90 1.951324=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 RFCOMM=A0 =A0=
Sent UA DLCI=3D20
> > > 91 1.959381=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 HCI_EVT=A0 =
=A0 Number of Completed Packets
> > > 92 1.966461=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 RFCOMM=A0 =A0=
Rcvd DISC DLCI=3D0
> > > 93 1.966492=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 L2CAP=A0 =A0 =
=A0 Rcvd Disconnect Request
> > > 94 1.972595=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 L2CAP=A0 =A0 =
=A0 Sent Disconnect Response
> > >=20
> > > ...
> > >=20
> > > 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.
> >=20
> > so I assume you have tested this extensively with various RFCOMM corner
> > cases like incoming RFCOMM. Since a lot of profiles require proper
> > disconnects and we have to ensure that our reference counting is
> > correct.
>=20
> We have the slightly modified patch applied for a several months. No regr=
ession found.
>=20
> Regards,
> Andrei
>=20
> >=20
> > Other then that it seems fine to me.
> >=20
> > Acked-by: Marcel Holtmann <marcel@holtmann.org>
Applied to bluetooth-next-2,6, thanks.
--=20
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-10 16:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-11-03 10:32 [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
2010-11-03 10:32 ` [PATCHv4 2/2] Bluetooth: timer check sk is not owned before freeing Emeltchenko Andrei
2010-11-10 5:36 ` [PATCHv4 0/2] Fix kernel crash in rfcomm/l2cap Marcel Holtmann
2010-11-10 15:24 ` Andrei Emeltchenko
2010-11-10 16:32 ` Gustavo F. Padovan
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).