Return-Path: Sender: "Gustavo F. Padovan" Date: Mon, 6 Dec 2010 19:15:17 -0200 From: "Gustavo F. Padovan" To: Yuri Ershov Cc: marcel@holtmann.org, davem@davemloft.net, jprvita@profusion.mobi, linux-bluetooth@vger.kernel.org, ville.tervo@nokia.com, andrei.emeltchenko@nokia.com Subject: Re: [PATCH 1/1] bluetooth: Fix NULL pointer dereference issue Message-ID: <20101206211516.GH883@vigoh> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: List-ID: Hi Yuri, * Yuri Ershov [2010-11-25 12:55:33 +0300]: > This patch is an addition to my previous patch for this issue. > The problem is in resynchronization between two loops: > 1. Main controlling loop (l2cap_connect_req, l2cap_config_req, > l2cap_config_rsp, l2cap_disconnect_req, etc.) > 2. Loop waiting of BT_CONNECTED state of socket (l2cap_sock_accept, > bt_accept_dequeue, etc.). > In case of fast sequence of connect/disconnect operations the loop #1 > makes several cycles, while the loop #2 only has time to make one > cycle and it results crash. > The aim of the patch is to skeep handling of sockets queued for > deletion. > > Signed-off-by: Yuri Ershov > --- > net/bluetooth/af_bluetooth.c | 2 ++ > net/bluetooth/l2cap.c | 6 ++++-- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c > index c4cf3f5..f9389da 100644 > --- a/net/bluetooth/af_bluetooth.c > +++ b/net/bluetooth/af_bluetooth.c > @@ -200,6 +200,8 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock) > BT_DBG("parent %p", parent); > > list_for_each_safe(p, n, &bt_sk(parent)->accept_q) { > + if (n == p) > + break; So in which situations (n == p), or (p == p->next)? That should happen only when p is the only element in the list, then p == head, right? > sk = (struct sock *) list_entry(p, struct bt_sock, accept_q); > > lock_sock(sk); > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c > index 12b4aa2..29f30b0 100644 > --- a/net/bluetooth/l2cap.c > +++ b/net/bluetooth/l2cap.c > @@ -133,7 +133,8 @@ static struct sock *__l2cap_get_chan_by_dcid(struct l2cap_chan_list *l, u16 cid) > { > struct sock *s; > for (s = l->head; s; s = l2cap_pi(s)->next_c) { > - if (l2cap_pi(s)->dcid == cid) > + if ((l2cap_pi(s)->dcid == cid) && > + (sk->sk_state != BT_DISCONN) && (sk->sk_state != BT_CLOSED)) I think its better check for the cid first, and if it matches check for the socket states, if they are BT_DISCONN or BT_CLOSED return NULL. Then you avoid unnecessary loops here. > break; > } > return s; > @@ -143,7 +144,8 @@ static struct sock *__l2cap_get_chan_by_scid(struct l2cap_chan_list *l, u16 cid) > { > struct sock *s; > for (s = l->head; s; s = l2cap_pi(s)->next_c) { > - if (l2cap_pi(s)->scid == cid) > + if ((l2cap_pi(s)->scid == cid) && > + (sk->sk_state != BT_DISCONN) && (sk->sk_state != BT_CLOSED)) > break; Same for this one. -- Gustavo F. Padovan http://profusion.mobi