[PATCHv2 0/2] Fix kernel crash in rfcomm/l2cap

[PATCHv2 0/2] Fix kernel crash in rfcomm/l2cap

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>

Version 2 of patches fixing kernel crash in RFCOMM / L2CAP.

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

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 |   57 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 45 insertions(+), 12 deletions(-)

[PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Emeltchenko Andrei]

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

[PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing

[Emeltchenko Andrei]

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 0221d05..2f8ac5d 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -73,6 +73,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;
@@ -82,6 +94,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 &&
@@ -98,18 +118,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

Re: [PATCHv2 2/2] Bluetooth: timer check sk is not owned before freeing

[Gustavo F. Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2010-08-12 16:25:37 +0300]:

> 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 0221d05..2f8ac5d 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -73,6 +73,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;
> @@ -82,6 +94,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;
> +	}

I'm not sure if I like this defer through timers. OTOH could be too much
overhead to do this defer through a work queue.
Also I think that 2 seconds is too much for this timer. 200 miliseconds
should be enough, what do you think?

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

[PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Emeltchenko Andrei]

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

Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Gustavo F. Padovan]

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

Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Andrei Emeltchenko]

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

Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Ville Tervo]

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

Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Gustavo F. Padovan]

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

Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn

[Andrei Emeltchenko]

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