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