This patch fixes NULL pointer dereference at running test with
connect-transfer-disconnect in loop. The problem conditions are the
following: there are 2 BT devices. The first one listens and
receives (l2test -r), the second one makes "connect-disconnect-
connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
After some time this will cause the race between functions
bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
sets the socket state to BT_CLOSED, unlinks and kills the socket
in the middle of bt_accept_dequeue, then at running the removed code
kernel oops appears.
Signed-off-by: Yuri Ershov <[email protected]>
---
net/bluetooth/af_bluetooth.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 421c45b..47c107e 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -204,13 +204,6 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
lock_sock(sk);
- /* FIXME: Is this check still needed */
- if (sk->sk_state == BT_CLOSED) {
- release_sock(sk);
- bt_accept_unlink(sk);
- continue;
- }
-
if (sk->sk_state == BT_CONNECTED || !newsock ||
bt_sk(parent)->defer_setup) {
bt_accept_unlink(sk);
--
1.6.3.3
Hi Yuri,
> This patch fixes NULL pointer dereference at running test with
> connect-transfer-disconnect in loop. The problem conditions are the
> following: there are 2 BT devices. The first one listens and
> receives (l2test -r), the second one makes "connect-disconnect-
> connect..." sequence (l2test -c -b 1000 -i hci0 -P 10 <addr>).
> After some time this will cause the race between functions
> bt_accept_dequeue and l2cap_chan_del. The function l2cap_chan_del
> sets the socket state to BT_CLOSED, unlinks and kills the socket
> in the middle of bt_accept_dequeue, then at running the removed code
> kernel oops appears.
it could be that we have this in here for RFCOMM. Can you retest it with
this with using RFCOMM and see if we get dangling RFCOMM sockets. You
need to test both directions, incoming and outgoing.
Regards
Marcel
Hi Yuri,
Please no top posting in this mainling list. It's not allowed, thanks.
* Yuri Ershov <[email protected]> [2010-10-25 16:23:47 +0400]:
> Hello Gustavo,
>
> The problem appears in case of multiple connect-transfer-disconnect
> sequence (e.g. by using l2test). The conditions are the following:
> There are 2 BT devices. The first one listens and receives (l2test -r),
> the second one makes "connect-disconnect-connect..." sequence (l2test -c
> -b 1000 -i hci0 -P 10 <addr>). After some time this will cause the race
> between functions bt_accept_dequeue and l2cap_chan_del. The fail sequence:
>
> struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
> {
> ...
> list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
> sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
>
> lock_sock(sk);
>
>
>
> In this time the function l2cap_chan_del sets the socket state to
> BT_CLOSED, unlinks and kills the socket.
>
>
>
> /* FIXME: Is this check still needed */
> if (sk->sk_state == BT_CLOSED) {
> release_sock(sk);
> bt_accept_unlink(sk);
> continue;
> }
>
> ...
>
> release_sock(sk);
> }
> return NULL;
> }
I agree with you, just add this info to your commit message and then
resend your patch so I can apply it.
> ext Gustavo F. Padovan wrote:
> > Hi Yuri,
> >
> > * Yuri Ershov <[email protected]> [2010-10-21 20:08:58 +0400]:
> >
> >
> >> This patch fixes NULL pointer dereference at running test with
> >> connect-transfer-disconnect in loop. Sometimes sk_state is
> >> BT_CLOSED and sk_refcnt equal to 0, so there is oops in
> >> bt_accept_unlink. In normal case removed block is not used.
> >>
> >
> > Question here is: Why sk_refcnt is 0 at that point of the code? The
> > socket should be destroyed if it ref is 0, but it wasn't, so something
> > in another point of the code went is wrong. "Sometimes" is not a good
> > description of the problem, you have to show why that happened.
> >
> >
>
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi
Hello Gustavo,
The problem appears in case of multiple connect-transfer-disconnect
sequence (e.g. by using l2test). The conditions are the following:
There are 2 BT devices. The first one listens and receives (l2test -r),
the second one makes "connect-disconnect-connect..." sequence (l2test -c
-b 1000 -i hci0 -P 10 <addr>). After some time this will cause the race
between functions bt_accept_dequeue and l2cap_chan_del. The fail sequence:
struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock)
{
...
list_for_each_safe(p, n, &bt_sk(parent)->accept_q) {
sk = (struct sock *) list_entry(p, struct bt_sock, accept_q);
lock_sock(sk);
In this time the function l2cap_chan_del sets the socket state to
BT_CLOSED, unlinks and kills the socket.
/* FIXME: Is this check still needed */
if (sk->sk_state == BT_CLOSED) {
release_sock(sk);
bt_accept_unlink(sk);
continue;
}
...
release_sock(sk);
}
return NULL;
}
Regards,
Yuri
ext Gustavo F. Padovan wrote:
> Hi Yuri,
>
> * Yuri Ershov <[email protected]> [2010-10-21 20:08:58 +0400]:
>
>
>> This patch fixes NULL pointer dereference at running test with
>> connect-transfer-disconnect in loop. Sometimes sk_state is
>> BT_CLOSED and sk_refcnt equal to 0, so there is oops in
>> bt_accept_unlink. In normal case removed block is not used.
>>
>
> Question here is: Why sk_refcnt is 0 at that point of the code? The
> socket should be destroyed if it ref is 0, but it wasn't, so something
> in another point of the code went is wrong. "Sometimes" is not a good
> description of the problem, you have to show why that happened.
>
>
Hi Yuri,
* Yuri Ershov <[email protected]> [2010-10-21 20:08:58 +0400]:
> This patch fixes NULL pointer dereference at running test with
> connect-transfer-disconnect in loop. Sometimes sk_state is
> BT_CLOSED and sk_refcnt equal to 0, so there is oops in
> bt_accept_unlink. In normal case removed block is not used.
Question here is: Why sk_refcnt is 0 at that point of the code? The
socket should be destroyed if it ref is 0, but it wasn't, so something
in another point of the code went is wrong. "Sometimes" is not a good
description of the problem, you have to show why that happened.
--
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi