Re: [PATCH] bluetooth: Fix NULL pointer dereference issue

["Gustavo F. Padovan" <padovan@profusion.mobi>]

Hi Yuri,

Please no top posting in this mainling list. It's not allowed, thanks.

* Yuri Ershov <ext-yuri.ershov@nokia.com> [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 <ext-yuri.ershov@nokia.com> [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