* [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
@ 2010-05-21 10:04 Emeltchenko Andrei
2010-05-21 10:04 ` [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing Emeltchenko Andrei
2010-05-23 2:39 ` [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Gustavo F. Padovan
0 siblings, 2 replies; 8+ messages in thread
From: Emeltchenko Andrei @ 2010-05-21 10:04 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.
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing
2010-05-21 10:04 [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
@ 2010-05-21 10:04 ` Emeltchenko Andrei
2010-05-23 2:39 ` [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Gustavo F. Padovan
1 sibling, 0 replies; 8+ messages in thread
From: Emeltchenko Andrei @ 2010-05-21 10:04 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.
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-05-21 10:04 [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
2010-05-21 10:04 ` [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing Emeltchenko Andrei
@ 2010-05-23 2:39 ` Gustavo F. Padovan
2010-05-24 12:14 ` Andrei Emeltchenko
2010-05-28 7:14 ` Ville Tervo
1 sibling, 2 replies; 8+ messages in thread
From: Gustavo F. Padovan @ 2010-05-23 2:39 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: linux-bluetooth
Hi Andrei,
* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-05-21 13:04:52 +0300]:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> 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);
Using the sock timer like you are you using looks too hackish, there are
kernel struct for such defer works. I still prefer the first solution,
that avoids the call to l2cap_chan_del() only.
But we have to solve the problem with the sock_kill() call, I'm
wondering if add a check inside l2cap_sock_kill is good idea. So we
check if the socket is owned by user and if yes, we just return, however
may have problem with socket refcnt doing that.
Looking to the rfcomm code I found something that could be cause of the
problem, there isn't any sock_hold() in the rfcomm code, maybe is it
missing? Nevertheless it does the sock_put() without call sock_hold().
Like you I'm trying to figure out how to fix this issue, I don't know
yet how to fix it properly. I advice to take a look on the rfcomm code
and check if we really are missing a sock_hold() there.
Also is not easy to reproduce such sequence of l2cap and rfcomm packets,
so I can't test the issue here too.
> + 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-05-23 2:39 ` [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Gustavo F. Padovan
@ 2010-05-24 12:14 ` Andrei Emeltchenko
2010-05-28 7:14 ` Ville Tervo
1 sibling, 0 replies; 8+ messages in thread
From: Andrei Emeltchenko @ 2010-05-24 12:14 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth
Hi Gustavo
On Sun, May 23, 2010 at 5:39 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-05-21 13:04:52 +0300]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> 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);
>
> Using the sock timer like you are you using looks too hackish, there are
> kernel struct for such defer works. I still prefer the first solution,
> that avoids the call to l2cap_chan_del() only.
OK. Then let's go for the first solution.
> But we have to solve the problem with the sock_kill() call, I'm
> wondering if add a check inside l2cap_sock_kill is good idea. So we
> check if the socket is owned by user and if yes, we just return, however
> may have problem with socket refcnt doing that.
I think by just using refcounting (sock_hold / sock_put) we overcome
the problem.
No need for extra checks; they looks hackish when we can use proper way.
> Looking to the rfcomm code I found something that could be cause of the
> problem, there isn't any sock_hold() in the rfcomm code, maybe is it
> missing? Nevertheless it does the sock_put() without call sock_hold().
Do you mean it is unbalanced? sock_hold is probably executed in some
function returning sk (which is not the best practice).
> Like you I'm trying to figure out how to fix this issue, I don't know
> yet how to fix it properly. I advice to take a look on the rfcomm code
> and check if we really are missing a sock_hold() there.
My first version of the patch adds refcounting to RFCOMM send function.
> Also is not easy to reproduce such sequence of l2cap and rfcomm packets,
> so I can't test the issue here too.
I guess this is harder to reproduce with desktop system but you can
add mdelay when sending RFCOMM. This way tasklet has more chances to
preempt krfcommd process.
@@ -1668,6 +1671,8 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct
if (err)
goto done;
+ mdelay(100);
+
if (msg->msg_flags & MSG_OOB) {
err = -EOPNOTSUPP;
goto done;
Could you try adding mdelay? There are reports that reproducing is
possible with rctest
Regards,
Andrei
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-05-23 2:39 ` [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Gustavo F. Padovan
2010-05-24 12:14 ` Andrei Emeltchenko
@ 2010-05-28 7:14 ` Ville Tervo
2010-06-28 16:07 ` Gustavo F. Padovan
1 sibling, 1 reply; 8+ messages in thread
From: Ville Tervo @ 2010-05-28 7:14 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Emeltchenko Andrei, linux-bluetooth
Hi,
On Sun, May 23, 2010 at 5:39 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
> Using the sock timer like you are you using looks too hackish, there are
> kernel struct for such defer works. I still prefer the first solution,
> that avoids the call to l2cap_chan_del() only.
> But we have to solve the problem with the sock_kill() call, I'm
> wondering if add a check inside l2cap_sock_kill is good idea. So we
> check if the socket is owned by user and if yes, we just return, however
> may have problem with socket refcnt doing that.
>
> Looking to the rfcomm code I found something that could be cause of the
> problem, there isn't any sock_hold() in the rfcomm code, maybe is it
> missing? Nevertheless it does the sock_put() without call sock_hold().
>
> Like you I'm trying to figure out how to fix this issue, I don't know
> yet how to fix it properly. I advice to take a look on the rfcomm code
> and check if we really are missing a sock_hold() there.
Wouldn't backlogging of destructive operations (l2cap disc rsp and
req) solve these issues? All operations cannot be backlogged since
they cannot mapped to certain sock.
--
Ville
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-05-28 7:14 ` Ville Tervo
@ 2010-06-28 16:07 ` Gustavo F. Padovan
2010-06-29 12:48 ` Andrei Emeltchenko
0 siblings, 1 reply; 8+ messages in thread
From: Gustavo F. Padovan @ 2010-06-28 16:07 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: Ville Tervo, linux-bluetooth
Hi Andrei,
* Ville Tervo <ville.tervo@iki.fi> [2010-05-28 10:14:31 +0300]:
> Hi,
>
> On Sun, May 23, 2010 at 5:39 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
>
> > Using the sock timer like you are you using looks too hackish, there are
> > kernel struct for such defer works. I still prefer the first solution,
> > that avoids the call to l2cap_chan_del() only.
> > But we have to solve the problem with the sock_kill() call, I'm
> > wondering if add a check inside l2cap_sock_kill is good idea. So we
> > check if the socket is owned by user and if yes, we just return, however
> > may have problem with socket refcnt doing that.
> >
> > Looking to the rfcomm code I found something that could be cause of the
> > problem, there isn't any sock_hold() in the rfcomm code, maybe is it
> > missing? Nevertheless it does the sock_put() without call sock_hold().
> >
> > Like you I'm trying to figure out how to fix this issue, I don't know
> > yet how to fix it properly. I advice to take a look on the rfcomm code
> > and check if we really are missing a sock_hold() there.
>
> Wouldn't backlogging of destructive operations (l2cap disc rsp and
> req) solve these issues? All operations cannot be backlogged since
> they cannot mapped to certain sock.
I've been thinking about this issue and now I agree that backlog them is
the better solution. We're already using backlog queue for ERTM, so you
will have to extend it only. Check the branch for-next of my tree to see
how I did that.
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-06-28 16:07 ` Gustavo F. Padovan
@ 2010-06-29 12:48 ` Andrei Emeltchenko
0 siblings, 0 replies; 8+ messages in thread
From: Andrei Emeltchenko @ 2010-06-29 12:48 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Ville Tervo, linux-bluetooth
Hi Gustavo,
On Mon, Jun 28, 2010 at 7:07 PM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
> Hi Andrei,
>
> * Ville Tervo <ville.tervo@iki.fi> [2010-05-28 10:14:31 +0300]:
>
>> Hi,
>>
>> On Sun, May 23, 2010 at 5:39 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
>>
>> > Using the sock timer like you are you using looks too hackish, there are
>> > kernel struct for such defer works. I still prefer the first solution,
>> > that avoids the call to l2cap_chan_del() only.
>> > But we have to solve the problem with the sock_kill() call, I'm
>> > wondering if add a check inside l2cap_sock_kill is good idea. So we
>> > check if the socket is owned by user and if yes, we just return, however
>> > may have problem with socket refcnt doing that.
>> >
>> > Looking to the rfcomm code I found something that could be cause of the
>> > problem, there isn't any sock_hold() in the rfcomm code, maybe is it
>> > missing? Nevertheless it does the sock_put() without call sock_hold().
>> >
>> > Like you I'm trying to figure out how to fix this issue, I don't know
>> > yet how to fix it properly. I advice to take a look on the rfcomm code
>> > and check if we really are missing a sock_hold() there.
>>
>> Wouldn't backlogging of destructive operations (l2cap disc rsp and
>> req) solve these issues? All operations cannot be backlogged since
>> they cannot mapped to certain sock.
>
> I've been thinking about this issue and now I agree that backlog them is
> the better solution. We're already using backlog queue for ERTM, so you
> will have to extend it only. Check the branch for-next of my tree to see
> how I did that.
IMO The case we are talking about is different. Personally I do not
think we need to backlog
packets for removed connection. Those packets will be discarded anyway
since there will
be no connection and this operation will consume CPU just for nothing.
Regards,
Andrei
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
2010-08-12 13:25 [PATCHv2 0/2] Fix kernel crash in rfcomm/l2cap Emeltchenko Andrei
@ 2010-08-12 13:25 ` Emeltchenko Andrei
0 siblings, 0 replies; 8+ messages in thread
From: Emeltchenko Andrei @ 2010-08-12 13:25 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.
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>
Fixes: NB#164721 - Device reboots while executing OBEX FTP tests
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 1874ece..0221d05 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2708,6 +2708,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;
}
@@ -2914,6 +2921,15 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
del_timer(&l2cap_pi(sk)->monitor_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);
@@ -2944,6 +2960,15 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
del_timer(&l2cap_pi(sk)->monitor_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
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-08-12 13:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-21 10:04 [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
2010-05-21 10:04 ` [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing Emeltchenko Andrei
2010-05-23 2:39 ` [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Gustavo F. Padovan
2010-05-24 12:14 ` Andrei Emeltchenko
2010-05-28 7:14 ` Ville Tervo
2010-06-28 16:07 ` Gustavo F. Padovan
2010-06-29 12:48 ` Andrei Emeltchenko
-- strict thread matches above, loose matches on Subject: below --
2010-08-12 13:25 [PATCHv2 0/2] Fix kernel crash in rfcomm/l2cap Emeltchenko Andrei
2010-08-12 13:25 ` [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn Emeltchenko Andrei
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).