Re: [PATCHv1 08/14] Bluetooth: Use chan lock in L2CAP sig commands

[Ulisses Furquim <ulisses@profusion.mobi>]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Convert sk lock to chan lock for L2CAP signalling commands.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   61 +++++++++++++++++++++++++------------------
>  net/bluetooth/l2cap_sock.c |    7 +++-
>  2 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c008608..ab6bede 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -377,6 +377,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>                hci_conn_put(conn->hcon);
>        }
>
> +       lock_sock(sk);
> +
>        __l2cap_state_change(chan, BT_CLOSED);
>        sock_set_flag(sk, SOCK_ZAPPED);
>
> @@ -389,6 +391,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>        } else
>                sk->sk_state_change(sk);
>
> +       release_sock(sk);
> +
>        if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
>                        test_bit(CONF_INPUT_DONE, &chan->conf_state)))
>                return;

You're making l2cap_chan_del() deal with sock lock so please remove
the comment above the function signature. It doesn't make sense to
leave old comments around.

> @@ -421,10 +425,10 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
>        while ((sk = bt_accept_dequeue(parent, NULL))) {
>                struct l2cap_chan *chan = l2cap_pi(sk)->chan;
>
> +               l2cap_chan_lock(chan);
>                __clear_chan_timer(chan);
> -               lock_sock(sk);
>                l2cap_chan_close(chan, ECONNRESET);
> -               release_sock(sk);
> +               l2cap_chan_unlock(chan);
>
>                chan->ops->close(chan->data);
>        }

Ok.

> @@ -440,10 +444,12 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>
>        switch (chan->state) {
>        case BT_LISTEN:
> +               lock_sock(sk);
>                l2cap_chan_cleanup_listen(sk);
>
>                __l2cap_state_change(chan, BT_CLOSED);
>                sock_set_flag(sk, SOCK_ZAPPED);
> +               release_sock(sk);
>                break;
>
>        case BT_CONNECTED:

Do we need to lock sock around l2cap_chan_cleanup_listen() as well?
l2cap_chan_close() will call l2cap_chan_del() which now does
lock_sock/release_sock, right?

> @@ -486,7 +492,9 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>                break;
>
>        default:
> +               lock_sock(sk);
>                sock_set_flag(sk, SOCK_ZAPPED);
> +               release_sock(sk);
>                break;
>        }
>  }

Hmm, this indeed looks correct. However, it'd be good to later revisit
this as we want core without any sock handling, right?

> @@ -714,6 +722,7 @@ static inline int l2cap_mode_supported(__u8 mode, __u32 feat_mask)
>
>  static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *chan, int err)
>  {
> +       struct sock *sk = chan->sk;
>        struct l2cap_disconn_req req;
>
>        if (!conn)
> @@ -730,8 +739,10 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
>        l2cap_send_cmd(conn, l2cap_get_ident(conn),
>                        L2CAP_DISCONN_REQ, sizeof(req), &req);
>
> +       lock_sock(sk);
>        __l2cap_state_change(chan, BT_DISCONN);
>        __l2cap_chan_set_err(chan, err);
> +       release_sock(sk);
>  }
>
>  /* ---- L2CAP connections ---- */

It seems we didn't have any locking around these before. Why? Do we
really need it now?

<snip>

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs