[PATCHv1 0/2] Don't delete l2cap chan and sk when sk is owned by user

[PATCHv1 0/2] Don't delete l2cap chan and sk when sk is owned by user

[Emeltchenko Andrei]

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

When sk is owned by user we shall not delete l2cap shannel structures and socket sk.
The first patch which is less hackish than using timer to delete l2cap channel.

Andrei Emeltchenko (2):
  Bluetooth: Check sk is not owned before freeing l2cap_conn
  Bluetooth: Prevent sk freeing in tasklet using refcount

 net/bluetooth/l2cap.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

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

[PATCHv1 2/2] Bluetooth: Prevent sk freeing in tasklet using refcount

[Emeltchenko Andrei]

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

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: [PATCHv1 0/2] Don't delete l2cap chan and sk when sk is owned by user

[Andrei Emeltchenko]

On Wed, May 26, 2010 at 5:21 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>
> When sk is owned by user we shall not delete l2cap shannel structures and socket sk.
> The first patch which is less hackish than using timer to delete l2cap channel.
>
> Andrei Emeltchenko (2):
>  Bluetooth: Check sk is not owned before freeing l2cap_conn
>  Bluetooth: Prevent sk freeing in tasklet using refcount
>
>  net/bluetooth/l2cap.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)

Ping

Re: [PATCHv1 0/2] Don't delete l2cap chan and sk when sk is owned by user

[Andrei Emeltchenko]

On Mon, Jun 7, 2010 at 5:11 PM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> On Wed, May 26, 2010 at 5:21 PM, Emeltchenko Andrei
> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> From: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>>
>> When sk is owned by user we shall not delete l2cap shannel structures and socket sk.
>> The first patch which is less hackish than using timer to delete l2cap channel.
>>
>> Andrei Emeltchenko (2):
>>  Bluetooth: Check sk is not owned before freeing l2cap_conn
>>  Bluetooth: Prevent sk freeing in tasklet using refcount
>>
>>  net/bluetooth/l2cap.c |   16 +++++++++++++---
>>  1 files changed, 13 insertions(+), 3 deletions(-)
>
> Ping

Ping

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

[Marcel Holtmann]

Hi Andrei,

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

the patch seems to be fine, but I have some extra questions/concerns.

Who is now taking care of deleting the channel in this case? I think you
need to show that the code flow is still valid.

Also the question is how RFCOMM can send this UA or DISC with not
locking the socket. The comment on l2cap_chan_del clearly states that
the socket must be locked and inside L2CAP we do that. Is RFCOMM maybe
at fault here?

Regards

Marcel

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

[Andrei Emeltchenko]

Hi Marcel,

On Tue, Jul 6, 2010 at 6:21 PM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
> Hi Andrei,
>
>> 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>
>
> the patch seems to be fine, but I have some extra questions/concerns.
>
> Who is now taking care of deleting the channel in this case? I think you
> need to show that the code flow is still valid.

I have the other version of the I have sent already to ML where I use
standard l2cap
timer which will delete channel like the code below:

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

> Also the question is how RFCOMM can send this UA or DISC with not
> locking the socket. The comment on l2cap_chan_del clearly states that
> the socket must be locked and inside L2CAP we do that. Is RFCOMM maybe
> at fault here?

when RFCOMM send packets it lock_sock which marks sk as owned
sk->sk_lock.owned =3D 1;
and then can be preempted.

Regards,
Andrei

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

[Marcel Holtmann]

Hi Andrei,

> >> 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>
> >
> > the patch seems to be fine, but I have some extra questions/concerns.
> >
> > Who is now taking care of deleting the channel in this case? I think you
> > need to show that the code flow is still valid.
> 
> I have the other version of the I have sent already to ML where I use
> standard l2cap
> timer which will delete channel like the code 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;
> +       }
> 
> > Also the question is how RFCOMM can send this UA or DISC with not
> > locking the socket. The comment on l2cap_chan_del clearly states that
> > the socket must be locked and inside L2CAP we do that. Is RFCOMM maybe
> > at fault here?
> 
> when RFCOMM send packets it lock_sock which marks sk as owned
> sk->sk_lock.owned = 1;
> and then can be preempted.

I need a new patch with a proper and most likely lengthy commit message
explaining every single detail here. Since right now you lost me.

Regards

Marcel

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

[Andrei Emeltchenko]

Hi Marcel,

On Thu, Aug 12, 2010 at 2:27 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andrei,
>
>> >> 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 remo=
ve
>> >> l2cap_conn structure. If krfcommd is in process of sending of RFCOMM =
reply
>> >> (like "RFCOMM UA" reply to "RFCOMM DISC") then kernel crash 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/firmw=
are/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/0xd8)
>> >> [ =A0694.537292] [<c02f2c10>] (sock_sendmsg+0x0/0xd8) from [<c02f3044=
>] (kernel_sendmsg+0x48/0x80)
>> >> ...
>> >>
>> >> Modified version after comments of Gustavo F. Padovan <gustavo@padova=
n.org>
>> >>
>> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@nokia.com>
>> >
>> > the patch seems to be fine, but I have some extra questions/concerns.
>> >
>> > Who is now taking care of deleting the channel in this case? I think y=
ou
>> > need to show that the code flow is still valid.
>>
>> I have the other version of the I have sent already to ML where I use
>> standard l2cap
>> timer which will delete channel like the code below:
>>
>> + =A0 =A0 =A0 /* don't delete l2cap channel if sk is owned by user */
>> + =A0 =A0 =A0 if (sock_owned_by_user(sk)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 sk->sk_state =3D BT_DISCONN;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_sock_clear_timer(sk);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 l2cap_sock_set_timer(sk, HZ);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bh_unlock_sock(sk);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>> + =A0 =A0 =A0 }
>>
>> > Also the question is how RFCOMM can send this UA or DISC with not
>> > locking the socket. The comment on l2cap_chan_del clearly states that
>> > the socket must be locked and inside L2CAP we do that. Is RFCOMM maybe
>> > at fault here?
>>
>> when RFCOMM send packets it lock_sock which marks sk as owned
>> sk->sk_lock.owned =3D 1;
>> and then can be preempted.
>
> I need a new patch with a proper and most likely lengthy commit message
> explaining every single detail here. Since right now you lost me.

I am sending other version of the patch with l2cap timer which clears
L2CAP channels.
This way we are sure that channel will be deleted despite the
technique does not look
perfect...
BTW: The crash is easy to reproduce on ARM with our test tools :-)

Regards,
Andrei