From: Andrei Emeltchenko <[email protected]>
Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
protocol above L2CAP without creating sockets.
Changes:
* RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
following recommendations from review.
* RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
l2cap_sock_shutdown function, fixed several styles issues.
* RFCv4: Better split patches so they looks more clear and obvious,
taking coments about naming change and delete unused vars. See diff change
from the previous version below:
* RFCv3: Split the big patch to several small (I believe logical) chunks,
remove unneded locks from cleanup_listen, use the same arguments for
locked/unlocked socket error functions.
* RFCv2: Convert l2cap channel list back to mutex from RCU list.
Andrei Emeltchenko (14):
Bluetooth: trivial: Fix long line
Bluetooth: Revert to mutexes from RCU list
Bluetooth: Add l2cap_chan_lock
Bluetooth: Add locked and unlocked state_change
Bluetooth: Add socket error function
Bluetooth: Add unlocked __l2cap_chan_add function
Bluetooth: Use chan lock in timers
Bluetooth: Use chan lock in L2CAP sig commands
Bluetooth: Use chan lock in L2CAP conn start
Bluetooth: Use chan lock in receiving data
Bluetooth: Change locking logic for conn/chan ready
Bluetooth: Change locking logic in security_cfm
Bluetooth: Use l2cap chan lock in socket connect
Bluetooth: Remove socket lock check
include/net/bluetooth/l2cap.h | 11 ++
net/bluetooth/l2cap_core.c | 391 +++++++++++++++++++++++++----------------
net/bluetooth/l2cap_sock.c | 34 +++-
3 files changed, 280 insertions(+), 156 deletions(-)
--
1.7.9
Hi all,
On Mon, Feb 20, 2012 at 03:53:14PM +0100, Marcel Holtmann wrote:
> > >> so what is the general status of this patch series. Are there still
> > >> concerns or opens? Or should it be go for final review and be merged?
> > >
> > > The code looks now good enough for final review.
> >
> > Marcel, the code looks good for final review and merge. The only thing
> > concerns me is the change to chan->lock instead of sock lock seems to
> > be split too much. I mean that we have this change done in a series of
> > patches while it might be better to change everything at once. Not
> > sure if worrying about intermediate states here is something you care
> > or not, though, because I'm almost sure they'll be broken doing it in
> > small pieces.
>
> I am fine either way at this point.
Then I send patches as is, if there is need to merge some patches this can
be easily done.
Best regards
Andrei Emeltchenko
* Ulisses Furquim <[email protected]> [2012-02-20 15:57:52 -0300]:
> Hi Gustavo,
>
> On Mon, Feb 20, 2012 at 3:33 PM, Gustavo Padovan <[email protected]> wrote:
> >
> > Hi Andrei,
> >
> > * Emeltchenko Andrei <[email protected]> [2012-02-20 16:21:13 +0200]:
> >
> > > From: Andrei Emeltchenko <[email protected]>
> > >
> > > Usage of RCU list looks not reasonalbe for a number of reasons:
> > > our code sleep and we have to use socket spinlocks, some parts
> > > of code are updaters thus we need to use mutexes anyway.
> > >
> > > Signed-off-by: Andrei Emeltchenko <[email protected]>
> > > ---
> > > ?net/bluetooth/l2cap_core.c | ?165 ++++++++++++++++++++++++++------------------
> > > ?net/bluetooth/l2cap_sock.c | ? 10 +++
> > > ?2 files changed, 108 insertions(+), 67 deletions(-)add an extra blank line here please.
>
> <snip>
>
> > > +unlock:
> > > + ? ? mutex_unlock(&conn->chan_lock);
> > > +
> > > + ? ? return err;
> > > ?}
> > >
> > > ?static inline void set_default_fcs(struct l2cap_chan *chan)
> > > @@ -2793,6 +2804,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> > > ? ? ? ? ? ? ? return -ENOENT;
> > >
> > > ? ? ? sk = chan->sk;
> > > + ? ? lock_sock(sk);
> > >
> > > ? ? ? if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
> > > ? ? ? ? ? ? ? struct l2cap_cmd_rej_cid rej;
> > > @@ -2905,6 +2917,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> > > ? ? ? ? ? ? ? return 0;
> > >
> > > ? ? ? sk = chan->sk;
> > > + ? ? lock_sock(sk);
> > >
> > > ? ? ? switch (result) {
> > > ? ? ? case L2CAP_CONF_SUCCESS:
> > > @@ -3006,11 +3019,16 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
> > >
> > > ? ? ? BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
> > >
> > > - ? ? chan = l2cap_get_chan_by_scid(conn, dcid);
> > > - ? ? if (!chan)
> > > + ? ? mutex_lock(&conn->chan_lock);
> > > +
> > > + ? ? chan = __l2cap_get_chan_by_scid(conn, dcid);
> > > + ? ? if (!chan) {
> > > + ? ? ? ? ? ? mutex_unlock(&conn->chan_lock);
> > > ? ? ? ? ? ? ? return 0;
> >
> > Seems to me we can use l2cap_get_chan_by_scid() here.
>
> It's better if we don't as we'll call l2cap_chan_del() which might
> remove chan from conn->chan_l that's protected by conn->chan_lock.
> Let's leave it as he sent.
Yes, I didn't realized the l2cap_chan_del() part. So after fix the new line
fix:
Acked-by: Gustavo F. Padovan <[email protected]>
Gustavo
Hi Gustavo,
On Mon, Feb 20, 2012 at 3:33 PM, Gustavo Padovan <[email protected]> wrote:
>
> Hi Andrei,
>
> * Emeltchenko Andrei <[email protected]> [2012-02-20 16:21:13 +0200]:
>
> > From: Andrei Emeltchenko <[email protected]>
> >
> > Usage of RCU list looks not reasonalbe for a number of reasons:
> > our code sleep and we have to use socket spinlocks, some parts
> > of code are updaters thus we need to use mutexes anyway.
> >
> > Signed-off-by: Andrei Emeltchenko <[email protected]>
> > ---
> > ?net/bluetooth/l2cap_core.c | ?165 ++++++++++++++++++++++++++------------------
> > ?net/bluetooth/l2cap_sock.c | ? 10 +++
> > ?2 files changed, 108 insertions(+), 67 deletions(-)add an extra blank line here please.
<snip>
> > +unlock:
> > + ? ? mutex_unlock(&conn->chan_lock);
> > +
> > + ? ? return err;
> > ?}
> >
> > ?static inline void set_default_fcs(struct l2cap_chan *chan)
> > @@ -2793,6 +2804,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> > ? ? ? ? ? ? ? return -ENOENT;
> >
> > ? ? ? sk = chan->sk;
> > + ? ? lock_sock(sk);
> >
> > ? ? ? if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
> > ? ? ? ? ? ? ? struct l2cap_cmd_rej_cid rej;
> > @@ -2905,6 +2917,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> > ? ? ? ? ? ? ? return 0;
> >
> > ? ? ? sk = chan->sk;
> > + ? ? lock_sock(sk);
> >
> > ? ? ? switch (result) {
> > ? ? ? case L2CAP_CONF_SUCCESS:
> > @@ -3006,11 +3019,16 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
> >
> > ? ? ? BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
> >
> > - ? ? chan = l2cap_get_chan_by_scid(conn, dcid);
> > - ? ? if (!chan)
> > + ? ? mutex_lock(&conn->chan_lock);
> > +
> > + ? ? chan = __l2cap_get_chan_by_scid(conn, dcid);
> > + ? ? if (!chan) {
> > + ? ? ? ? ? ? mutex_unlock(&conn->chan_lock);
> > ? ? ? ? ? ? ? return 0;
>
> Seems to me we can use l2cap_get_chan_by_scid() here.
It's better if we don't as we'll call l2cap_chan_del() which might
remove chan from conn->chan_l that's protected by conn->chan_lock.
Let's leave it as he sent.
> > + ? ? }
> >
> > ? ? ? sk = chan->sk;
> > + ? ? lock_sock(sk);
> >
> > ? ? ? rsp.dcid = cpu_to_le16(chan->scid);
> > ? ? ? rsp.scid = cpu_to_le16(chan->dcid);
> > @@ -3022,6 +3040,9 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
> > ? ? ? release_sock(sk);
> >
> > ? ? ? chan->ops->close(chan->data);
> > +
> > + ? ? mutex_unlock(&conn->chan_lock);
> > +
> > ? ? ? return 0;
> > ?}
> >
> > @@ -3037,16 +3058,24 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
> >
> > ? ? ? BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
> >
> > - ? ? chan = l2cap_get_chan_by_scid(conn, scid);
> > - ? ? if (!chan)
> > + ? ? mutex_lock(&conn->chan_lock);
> > +
> > + ? ? chan = __l2cap_get_chan_by_scid(conn, scid);
> > + ? ? if (!chan) {
> > + ? ? ? ? ? ? mutex_unlock(&conn->chan_lock);
> > ? ? ? ? ? ? ? return 0;
>
> Same here.
Same comment above applies here.
Apart from the extra new line that Padovan asked I'm ok with this commit.
Reviewed-by: Ulisses Furquim <[email protected]>
Regards,
--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
Hi Andrei,
* Emeltchenko Andrei <[email protected]> [2012-02-20 16:21:13 +0200]:
> From: Andrei Emeltchenko <[email protected]>
>
> Usage of RCU list looks not reasonalbe for a number of reasons:
> our code sleep and we have to use socket spinlocks, some parts
> of code are updaters thus we need to use mutexes anyway.
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 165 ++++++++++++++++++++++++++------------------
> net/bluetooth/l2cap_sock.c | 10 +++
> 2 files changed, 108 insertions(+), 67 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 63539f9..90e29ac 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -77,36 +77,24 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
>
> static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
> {
> - struct l2cap_chan *c, *r = NULL;
> -
> - rcu_read_lock();
> + struct l2cap_chan *c;
>
> - list_for_each_entry_rcu(c, &conn->chan_l, list) {
> - if (c->dcid == cid) {
> - r = c;
> - break;
> - }
> + list_for_each_entry(c, &conn->chan_l, list) {
> + if (c->dcid == cid)
> + return c;
> }
> -
> - rcu_read_unlock();
> - return r;
> + return NULL;
> }
>
> static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
> {
> - struct l2cap_chan *c, *r = NULL;
> -
> - rcu_read_lock();
> + struct l2cap_chan *c;
>
> - list_for_each_entry_rcu(c, &conn->chan_l, list) {
> - if (c->scid == cid) {
> - r = c;
> - break;
> - }
> + list_for_each_entry(c, &conn->chan_l, list) {
> + if (c->scid == cid)
> + return c;
> }
> -
> - rcu_read_unlock();
> - return r;
> + return NULL;
> }
>
> /* Find channel with given SCID.
> @@ -115,36 +103,32 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
> {
> struct l2cap_chan *c;
>
> + mutex_lock(&conn->chan_lock);
> c = __l2cap_get_chan_by_scid(conn, cid);
> - if (c)
> - lock_sock(c->sk);
> + mutex_unlock(&conn->chan_lock);
> +
> return c;
> }
>
> static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
> {
> - struct l2cap_chan *c, *r = NULL;
> -
> - rcu_read_lock();
> + struct l2cap_chan *c;
>
> - list_for_each_entry_rcu(c, &conn->chan_l, list) {
> - if (c->ident == ident) {
> - r = c;
> - break;
> - }
> + list_for_each_entry(c, &conn->chan_l, list) {
> + if (c->ident == ident)
> + return c;
> }
> -
> - rcu_read_unlock();
> - return r;
> + return NULL;
> }
>
> static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
> {
> struct l2cap_chan *c;
>
> + mutex_lock(&conn->chan_lock);
> c = __l2cap_get_chan_by_ident(conn, ident);
> - if (c)
> - lock_sock(c->sk);
> + mutex_unlock(&conn->chan_lock);
> +
> return c;
> }
>
> @@ -228,11 +212,13 @@ static void l2cap_chan_timeout(struct work_struct *work)
> {
> struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
> chan_timer.work);
> + struct l2cap_conn *conn = chan->conn;
> struct sock *sk = chan->sk;
> int reason;
>
> BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
>
> + mutex_lock(&conn->chan_lock);
> lock_sock(sk);
>
> if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> @@ -248,6 +234,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
> release_sock(sk);
>
> chan->ops->close(chan->data);
> + mutex_unlock(&conn->chan_lock);
> +
> l2cap_chan_put(chan);
> }
>
> @@ -331,7 +319,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>
> l2cap_chan_hold(chan);
>
> - list_add_rcu(&chan->list, &conn->chan_l);
> + mutex_lock(&conn->chan_lock);
> + list_add(&chan->list, &conn->chan_l);
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* Delete channel.
> @@ -348,8 +338,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>
> if (conn) {
> /* Delete from channel list */
> - list_del_rcu(&chan->list);
> - synchronize_rcu();
> + list_del(&chan->list);
>
> l2cap_chan_put(chan);
>
> @@ -400,10 +389,12 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
> /* Close not yet accepted channels */
> while ((sk = bt_accept_dequeue(parent, NULL))) {
> struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +
> __clear_chan_timer(chan);
> lock_sock(sk);
> l2cap_chan_close(chan, ECONNRESET);
> release_sock(sk);
> +
> chan->ops->close(chan->data);
> }
> }
> @@ -718,13 +709,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
> /* ---- L2CAP connections ---- */
> static void l2cap_conn_start(struct l2cap_conn *conn)
> {
> - struct l2cap_chan *chan;
> + struct l2cap_chan *chan, *tmp;
>
> BT_DBG("conn %p", conn);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
>
> bh_lock_sock(sk);
> @@ -804,7 +795,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
> bh_unlock_sock(sk);
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* Find socket with cid and source bdaddr.
> @@ -916,9 +907,9 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> if (conn->hcon->out && conn->hcon->type == LE_LINK)
> smp_conn_security(conn, conn->hcon->pending_sec_level);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry(chan, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
>
> bh_lock_sock(sk);
> @@ -938,7 +929,7 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> bh_unlock_sock(sk);
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* Notify sockets that we cannot guaranty reliability anymore */
> @@ -948,16 +939,16 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
>
> BT_DBG("conn %p", conn);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry(chan, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
>
> if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
> sk->sk_err = err;
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> static void l2cap_info_timeout(struct work_struct *work)
> @@ -984,6 +975,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>
> kfree_skb(conn->rx_skb);
>
> + mutex_lock(&conn->chan_lock);
> +
> /* Kill channels */
> list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
> sk = chan->sk;
> @@ -993,6 +986,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> chan->ops->close(chan->data);
> }
>
> + mutex_unlock(&conn->chan_lock);
> +
> hci_chan_del(conn->hchan);
>
> if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> @@ -1050,6 +1045,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> conn->feat_mask = 0;
>
> spin_lock_init(&conn->lock);
> + mutex_init(&conn->chan_lock);
>
> INIT_LIST_HEAD(&conn->chan_l);
>
> @@ -1792,9 +1788,9 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
>
> BT_DBG("conn %p", conn);
>
> - rcu_read_lock();
> + mutex_lock(&conn->chan_lock);
>
> - list_for_each_entry_rcu(chan, &conn->chan_l, list) {
> + list_for_each_entry(chan, &conn->chan_l, list) {
> struct sock *sk = chan->sk;
> if (chan->chan_type != L2CAP_CHAN_RAW)
> continue;
> @@ -1810,7 +1806,7 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
> kfree_skb(nskb);
> }
>
> - rcu_read_unlock();
> + mutex_unlock(&conn->chan_lock);
> }
>
> /* ---- L2CAP signalling commands ---- */
> @@ -2600,6 +2596,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>
> parent = pchan->sk;
>
> + mutex_lock(&conn->chan_lock);
> lock_sock(parent);
>
> /* Check if the ACL is secure enough (if not SDP) */
> @@ -2673,6 +2670,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>
> response:
> release_sock(parent);
> + mutex_unlock(&conn->chan_lock);
>
> sendresp:
> rsp.scid = cpu_to_le16(scid);
> @@ -2714,6 +2712,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> struct l2cap_chan *chan;
> struct sock *sk;
> u8 req[128];
> + int err;
>
> scid = __le16_to_cpu(rsp->scid);
> dcid = __le16_to_cpu(rsp->dcid);
> @@ -2723,17 +2722,26 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
> dcid, scid, result, status);
>
> + mutex_lock(&conn->chan_lock);
> +
> if (scid) {
> - chan = l2cap_get_chan_by_scid(conn, scid);
> - if (!chan)
> - return -EFAULT;
> + chan = __l2cap_get_chan_by_scid(conn, scid);
> + if (!chan) {
> + err = -EFAULT;
> + goto unlock;
> + }
> } else {
> - chan = l2cap_get_chan_by_ident(conn, cmd->ident);
> - if (!chan)
> - return -EFAULT;
> + chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
> + if (!chan) {
> + err = -EFAULT;
> + goto unlock;
> + }
> }
>
> + err = 0;
> +
> sk = chan->sk;
> + lock_sock(sk);
>
> switch (result) {
> case L2CAP_CR_SUCCESS:
> @@ -2760,7 +2768,10 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> }
>
> release_sock(sk);
> - return 0;
add an extra blank line here please.
> +unlock:
> + mutex_unlock(&conn->chan_lock);
> +
> + return err;
> }
>
> static inline void set_default_fcs(struct l2cap_chan *chan)
> @@ -2793,6 +2804,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> return -ENOENT;
>
> sk = chan->sk;
> + lock_sock(sk);
>
> if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
> struct l2cap_cmd_rej_cid rej;
> @@ -2905,6 +2917,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
> return 0;
>
> sk = chan->sk;
> + lock_sock(sk);
>
> switch (result) {
> case L2CAP_CONF_SUCCESS:
> @@ -3006,11 +3019,16 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
>
> BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
>
> - chan = l2cap_get_chan_by_scid(conn, dcid);
> - if (!chan)
> + mutex_lock(&conn->chan_lock);
> +
> + chan = __l2cap_get_chan_by_scid(conn, dcid);
> + if (!chan) {
> + mutex_unlock(&conn->chan_lock);
> return 0;
Seems to me we can use l2cap_get_chan_by_scid() here.
> + }
>
> sk = chan->sk;
> + lock_sock(sk);
>
> rsp.dcid = cpu_to_le16(chan->scid);
> rsp.scid = cpu_to_le16(chan->dcid);
> @@ -3022,6 +3040,9 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
> release_sock(sk);
>
> chan->ops->close(chan->data);
> +
> + mutex_unlock(&conn->chan_lock);
> +
> return 0;
> }
>
> @@ -3037,16 +3058,24 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
>
> BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
>
> - chan = l2cap_get_chan_by_scid(conn, scid);
> - if (!chan)
> + mutex_lock(&conn->chan_lock);
> +
> + chan = __l2cap_get_chan_by_scid(conn, scid);
> + if (!chan) {
> + mutex_unlock(&conn->chan_lock);
> return 0;
Same here.
Gustavo
Hi Andrei,
* Emeltchenko Andrei <[email protected]> [2012-02-20 16:21:12 +0200]:
> From: Andrei Emeltchenko <[email protected]>
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> Acked-by: Marcel Holtmann <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 37736c6..63539f9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2720,7 +2720,8 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
> result = __le16_to_cpu(rsp->result);
> status = __le16_to_cpu(rsp->status);
>
> - BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x", dcid, scid, result, status);
> + BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
> + dcid, scid, result, status);
>
> if (scid) {
> chan = l2cap_get_chan_by_scid(conn, scid);
Acked-by: Gustavo F. Padovan <[email protected]>
Gustavo
Hi Ulisses,
> >> > Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
> >> > protocol above L2CAP without creating sockets.
> >> >
> >> > Changes:
> >> > * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
> >> > following recommendations from review.
> >> > * RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
> >> > l2cap_sock_shutdown function, fixed several styles issues.
> >> > * RFCv4: Better split patches so they looks more clear and obvious,
> >> > taking coments about naming change and delete unused vars. See diff change
> >> > from the previous version below:
> >> > * RFCv3: Split the big patch to several small (I believe logical) chunks,
> >> > remove unneded locks from cleanup_listen, use the same arguments for
> >> > locked/unlocked socket error functions.
> >> > * RFCv2: Convert l2cap channel list back to mutex from RCU list.
> >>
> >> so what is the general status of this patch series. Are there still
> >> concerns or opens? Or should it be go for final review and be merged?
> >
> > The code looks now good enough for final review.
>
> Marcel, the code looks good for final review and merge. The only thing
> concerns me is the change to chan->lock instead of sock lock seems to
> be split too much. I mean that we have this change done in a series of
> patches while it might be better to change everything at once. Not
> sure if worrying about intermediate states here is something you care
> or not, though, because I'm almost sure they'll be broken doing it in
> small pieces.
I am fine either way at this point.
> And IMO it'd be good if Padovan could take a look at the patches
> moving to chan->lock as well.
Then please add proper reviewed-by tags to the patches.
Regards
Marcel
Hi,
On Mon, Feb 20, 2012 at 12:44 PM, Emeltchenko Andrei
<[email protected]> wrote:
> Hi Marcel,
>
> On Mon, Feb 20, 2012 at 03:29:33PM +0100, Marcel Holtmann wrote:
>> Hi Andrei,
>>
>> > Changing socket lock to L2CAP chan lock in L2CAP code. Needed for impl=
ementing
>> > protocol above L2CAP without creating sockets.
>> >
>> > Changes:
>> > =A0 =A0 * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged=
together
>> > =A0 =A0 following recommendations from review.
>> > =A0 =A0 * RFCv5: Fixed locking bug in l2cap_data_channel, added locks =
in
>> > =A0 =A0 l2cap_sock_shutdown function, fixed several styles issues.
>> > =A0 =A0 * RFCv4: Better split patches so they looks more clear and obv=
ious,
>> > =A0 =A0 taking coments about naming change and delete unused vars. See=
diff change
>> > =A0 =A0 from the previous version below:
>> > =A0 =A0 * RFCv3: Split the big patch to several small (I believe logic=
al) chunks,
>> > =A0 =A0 remove unneded locks from cleanup_listen, use the same argumen=
ts for
>> > =A0 =A0 locked/unlocked socket error functions.
>> > =A0 =A0 * RFCv2: Convert l2cap channel list back to mutex from RCU lis=
t.
>>
>> so what is the general status of this patch series. Are there still
>> concerns or opens? Or should it be go for final review and be merged?
>
> The code looks now good enough for final review.
Marcel, the code looks good for final review and merge. The only thing
concerns me is the change to chan->lock instead of sock lock seems to
be split too much. I mean that we have this change done in a series of
patches while it might be better to change everything at once. Not
sure if worrying about intermediate states here is something you care
or not, though, because I'm almost sure they'll be broken doing it in
small pieces.
And IMO it'd be good if Padovan could take a look at the patches
moving to chan->lock as well.
Regards,
--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
Hi Marcel,
On Mon, Feb 20, 2012 at 03:29:33PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
>
> > Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
> > protocol above L2CAP without creating sockets.
> >
> > Changes:
> > * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
> > following recommendations from review.
> > * RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
> > l2cap_sock_shutdown function, fixed several styles issues.
> > * RFCv4: Better split patches so they looks more clear and obvious,
> > taking coments about naming change and delete unused vars. See diff change
> > from the previous version below:
> > * RFCv3: Split the big patch to several small (I believe logical) chunks,
> > remove unneded locks from cleanup_listen, use the same arguments for
> > locked/unlocked socket error functions.
> > * RFCv2: Convert l2cap channel list back to mutex from RCU list.
>
> so what is the general status of this patch series. Are there still
> concerns or opens? Or should it be go for final review and be merged?
The code looks now good enough for final review.
Best regards
Andrei Emeltchenko
Hi Andrei,
On Mon, Feb 20, 2012 at 12:21 PM, Emeltchenko Andrei
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Usage of RCU list looks not reasonalbe for a number of reasons:
> our code sleep and we have to use socket spinlocks, some parts
> of code are updaters thus we need to use mutexes anyway.
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> ?net/bluetooth/l2cap_core.c | ?165 ++++++++++++++++++++++++++------------------
> ?net/bluetooth/l2cap_sock.c | ? 10 +++
> ?2 files changed, 108 insertions(+), 67 deletions(-)
Now it does look good to me. Thanks.
Regards,
--
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
Hi Andrei,
> Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
> protocol above L2CAP without creating sockets.
>
> Changes:
> * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
> following recommendations from review.
> * RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
> l2cap_sock_shutdown function, fixed several styles issues.
> * RFCv4: Better split patches so they looks more clear and obvious,
> taking coments about naming change and delete unused vars. See diff change
> from the previous version below:
> * RFCv3: Split the big patch to several small (I believe logical) chunks,
> remove unneded locks from cleanup_listen, use the same arguments for
> locked/unlocked socket error functions.
> * RFCv2: Convert l2cap channel list back to mutex from RCU list.
so what is the general status of this patch series. Are there still
concerns or opens? Or should it be go for final review and be merged?
Regards
Marcel
From: Andrei Emeltchenko <[email protected]>
Split to locked and unlocked versions of l2cap_state_change helping
to remove socket locks from l2cap code.
Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
net/bluetooth/l2cap_core.c | 41 +++++++++++++++++++++++++----------------
1 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 98e116b..a206a24 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -199,7 +199,7 @@ static u16 l2cap_alloc_cid(struct l2cap_conn *conn)
return 0;
}
-static void l2cap_state_change(struct l2cap_chan *chan, int state)
+static void __l2cap_state_change(struct l2cap_chan *chan, int state)
{
BT_DBG("chan %p %s -> %s", chan, state_to_string(chan->state),
state_to_string(state));
@@ -208,6 +208,15 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
chan->ops->state_change(chan->data, state);
}
+static void l2cap_state_change(struct l2cap_chan *chan, int state)
+{
+ struct sock *sk = chan->sk;
+
+ lock_sock(sk);
+ __l2cap_state_change(chan, state);
+ release_sock(sk);
+}
+
static void l2cap_chan_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
@@ -348,7 +357,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
hci_conn_put(conn->hcon);
}
- l2cap_state_change(chan, BT_CLOSED);
+ __l2cap_state_change(chan, BT_CLOSED);
sock_set_flag(sk, SOCK_ZAPPED);
if (err)
@@ -413,7 +422,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
case BT_LISTEN:
l2cap_chan_cleanup_listen(sk);
- l2cap_state_change(chan, BT_CLOSED);
+ __l2cap_state_change(chan, BT_CLOSED);
sock_set_flag(sk, SOCK_ZAPPED);
break;
@@ -704,7 +713,7 @@ 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);
- l2cap_state_change(chan, BT_DISCONN);
+ __l2cap_state_change(chan, BT_DISCONN);
sk->sk_err = err;
}
@@ -770,7 +779,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
parent->sk_data_ready(parent, 0);
} else {
- l2cap_state_change(chan, BT_CONFIG);
+ __l2cap_state_change(chan, BT_CONFIG);
rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
rsp.status = cpu_to_le16(L2CAP_CS_NO_INFO);
}
@@ -873,7 +882,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
__set_chan_timer(chan, sk->sk_sndtimeo);
- l2cap_state_change(chan, BT_CONNECTED);
+ __l2cap_state_change(chan, BT_CONNECTED);
parent->sk_data_ready(parent, 0);
clean:
@@ -890,7 +899,7 @@ static void l2cap_chan_ready(struct l2cap_chan *chan)
chan->conf_state = 0;
__clear_chan_timer(chan);
- l2cap_state_change(chan, BT_CONNECTED);
+ __l2cap_state_change(chan, BT_CONNECTED);
sk->sk_state_change(sk);
if (parent)
@@ -922,7 +931,7 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
} else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
__clear_chan_timer(chan);
- l2cap_state_change(chan, BT_CONNECTED);
+ __l2cap_state_change(chan, BT_CONNECTED);
sk->sk_state_change(sk);
} else if (chan->state == BT_CONNECT)
@@ -1196,14 +1205,14 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
l2cap_chan_add(conn, chan);
- l2cap_state_change(chan, BT_CONNECT);
+ __l2cap_state_change(chan, BT_CONNECT);
__set_chan_timer(chan, sk->sk_sndtimeo);
if (hcon->state == BT_CONNECTED) {
if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
__clear_chan_timer(chan);
if (l2cap_chan_check_security(chan))
- l2cap_state_change(chan, BT_CONNECTED);
+ __l2cap_state_change(chan, BT_CONNECTED);
} else
l2cap_do_start(chan);
}
@@ -2650,22 +2659,22 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE) {
if (l2cap_chan_check_security(chan)) {
if (bt_sk(sk)->defer_setup) {
- l2cap_state_change(chan, BT_CONNECT2);
+ __l2cap_state_change(chan, BT_CONNECT2);
result = L2CAP_CR_PEND;
status = L2CAP_CS_AUTHOR_PEND;
parent->sk_data_ready(parent, 0);
} else {
- l2cap_state_change(chan, BT_CONFIG);
+ __l2cap_state_change(chan, BT_CONFIG);
result = L2CAP_CR_SUCCESS;
status = L2CAP_CS_NO_INFO;
}
} else {
- l2cap_state_change(chan, BT_CONNECT2);
+ __l2cap_state_change(chan, BT_CONNECT2);
result = L2CAP_CR_PEND;
status = L2CAP_CS_AUTHEN_PEND;
}
} else {
- l2cap_state_change(chan, BT_CONNECT2);
+ __l2cap_state_change(chan, BT_CONNECT2);
result = L2CAP_CR_PEND;
status = L2CAP_CS_NO_INFO;
}
@@ -4583,12 +4592,12 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
if (parent)
parent->sk_data_ready(parent, 0);
} else {
- l2cap_state_change(chan, BT_CONFIG);
+ __l2cap_state_change(chan, BT_CONFIG);
res = L2CAP_CR_SUCCESS;
stat = L2CAP_CS_NO_INFO;
}
} else {
- l2cap_state_change(chan, BT_DISCONN);
+ __l2cap_state_change(chan, BT_DISCONN);
__set_chan_timer(chan,
msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
res = L2CAP_CR_SEC_BLOCK;
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Channel lock will be used to lock L2CAP channels which are locked
currently by socket locks.
Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
include/net/bluetooth/l2cap.h | 11 +++++++++++
net/bluetooth/l2cap_core.c | 2 ++
2 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index bbb0e21..d6d8ec8 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -497,6 +497,7 @@ struct l2cap_chan {
void *data;
struct l2cap_ops *ops;
+ struct mutex lock;
};
struct l2cap_ops {
@@ -609,6 +610,16 @@ static inline void l2cap_chan_put(struct l2cap_chan *c)
kfree(c);
}
+static inline void l2cap_chan_lock(struct l2cap_chan *chan)
+{
+ mutex_lock(&chan->lock);
+}
+
+static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
+{
+ mutex_unlock(&chan->lock);
+}
+
static inline void l2cap_set_timer(struct l2cap_chan *chan,
struct delayed_work *work, long timeout)
{
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 90e29ac..98e116b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -247,6 +247,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk)
if (!chan)
return NULL;
+ mutex_init(&chan->lock);
+
chan->sk = sk;
write_lock(&chan_list_lock);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Change locking logic when receiving ACL and L2CAP data.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 11 +++++------
net/bluetooth/l2cap_sock.c | 11 +++++++++--
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 71c7a08..31810d3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4261,7 +4261,6 @@ drop:
static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk_buff *skb)
{
struct l2cap_chan *chan;
- struct sock *sk = NULL;
u32 control;
u16 tx_seq;
int len;
@@ -4269,11 +4268,12 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
chan = l2cap_get_chan_by_scid(conn, cid);
if (!chan) {
BT_DBG("unknown cid 0x%4.4x", cid);
- goto drop;
+ /* Drop packet and return */
+ kfree(skb);
+ return 0;
}
- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);
BT_DBG("chan %p, len %d", chan, skb->len);
@@ -4344,8 +4344,7 @@ drop:
kfree_skb(skb);
done:
- if (sk)
- release_sock(sk);
+ l2cap_chan_unlock(chan);
return 0;
}
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index db38fae..a278858 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -877,8 +877,12 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
struct sock *sk = data;
struct l2cap_pinfo *pi = l2cap_pi(sk);
- if (pi->rx_busy_skb)
- return -ENOMEM;
+ lock_sock(sk);
+
+ if (pi->rx_busy_skb) {
+ err = -ENOMEM;
+ goto done;
+ }
err = sock_queue_rcv_skb(sk, skb);
@@ -897,6 +901,9 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
err = 0;
}
+done:
+ release_sock(sk);
+
return err;
}
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Change locking in delayed works to chan lock instead of sk lock.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 26 ++++++++++++++------------
1 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index db1640c..727a1dd 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -238,13 +238,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
chan_timer.work);
struct l2cap_conn *conn = chan->conn;
- struct sock *sk = chan->sk;
int reason;
BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
mutex_lock(&conn->chan_lock);
- lock_sock(sk);
+ l2cap_chan_lock(chan);
if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
reason = ECONNREFUSED;
@@ -256,7 +255,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
l2cap_chan_close(chan, reason);
- release_sock(sk);
+ l2cap_chan_unlock(chan);
chan->ops->close(chan->data);
mutex_unlock(&conn->chan_lock);
@@ -1277,14 +1276,14 @@ static void l2cap_monitor_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
monitor_timer.work);
- struct sock *sk = chan->sk;
BT_DBG("chan %p", chan);
- lock_sock(sk);
+ l2cap_chan_lock(chan);
+
if (chan->retry_count >= chan->remote_max_tx) {
l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
- release_sock(sk);
+ l2cap_chan_unlock(chan);
return;
}
@@ -1292,25 +1291,26 @@ static void l2cap_monitor_timeout(struct work_struct *work)
__set_monitor_timer(chan);
l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
- release_sock(sk);
+ l2cap_chan_unlock(chan);
}
static void l2cap_retrans_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
retrans_timer.work);
- struct sock *sk = chan->sk;
BT_DBG("chan %p", chan);
- lock_sock(sk);
+ l2cap_chan_lock(chan);
+
chan->retry_count = 1;
__set_monitor_timer(chan);
set_bit(CONN_WAIT_F, &chan->conn_state);
l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
- release_sock(sk);
+
+ l2cap_chan_unlock(chan);
}
static void l2cap_drop_acked_frames(struct l2cap_chan *chan)
@@ -2001,9 +2001,11 @@ static void l2cap_ack_timeout(struct work_struct *work)
BT_DBG("chan %p", chan);
- lock_sock(chan->sk);
+ l2cap_chan_lock(chan);
+
__l2cap_send_ack(chan);
- release_sock(chan->sk);
+
+ l2cap_chan_unlock(chan);
l2cap_chan_put(chan);
}
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Signed-off-by: Andrei Emeltchenko <[email protected]>
Acked-by: Marcel Holtmann <[email protected]>
---
net/bluetooth/l2cap_core.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 37736c6..63539f9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2720,7 +2720,8 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
result = __le16_to_cpu(rsp->result);
status = __le16_to_cpu(rsp->status);
- BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x", dcid, scid, result, status);
+ BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
+ dcid, scid, result, status);
if (scid) {
chan = l2cap_get_chan_by_scid(conn, scid);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Change sk lock to chan lock in l2cap_conn_start. bh_locks were used
because of being RCU critical section.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a36b2c3..71c7a08 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -757,10 +757,10 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
struct sock *sk = chan->sk;
- bh_lock_sock(sk);
+ l2cap_chan_lock(chan);
if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
@@ -769,17 +769,15 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
if (!l2cap_chan_check_security(chan) ||
!__l2cap_no_conn_pending(chan)) {
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
if (!l2cap_mode_supported(chan->mode, conn->feat_mask)
&& test_bit(CONF_STATE2_DEVICE,
&chan->conf_state)) {
- /* l2cap_chan_close() calls list_del(chan)
- * so release the lock */
l2cap_chan_close(chan, ECONNRESET);
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
@@ -799,6 +797,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
rsp.dcid = cpu_to_le16(chan->scid);
if (l2cap_chan_check_security(chan)) {
+ lock_sock(sk);
if (bt_sk(sk)->defer_setup) {
struct sock *parent = bt_sk(sk)->parent;
rsp.result = cpu_to_le16(L2CAP_CR_PEND);
@@ -811,6 +810,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
rsp.status = cpu_to_le16(L2CAP_CS_NO_INFO);
}
+ release_sock(sk);
} else {
rsp.result = cpu_to_le16(L2CAP_CR_PEND);
rsp.status = cpu_to_le16(L2CAP_CS_AUTHEN_PEND);
@@ -821,7 +821,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
if (test_bit(CONF_REQ_SENT, &chan->conf_state) ||
rsp.result != L2CAP_CR_SUCCESS) {
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
@@ -831,7 +831,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
chan->num_conf_req++;
}
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
}
mutex_unlock(&conn->chan_lock);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 1e3ebc5..db1640c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -300,7 +300,7 @@ void l2cap_chan_destroy(struct l2cap_chan *chan)
l2cap_chan_put(chan);
}
-static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
{
BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
chan->psm, chan->dcid);
@@ -346,8 +346,13 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
l2cap_chan_hold(chan);
- mutex_lock(&conn->chan_lock);
list_add(&chan->list, &conn->chan_l);
+}
+
+void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+{
+ mutex_lock(&conn->chan_lock);
+ __l2cap_chan_add(conn, chan);
mutex_unlock(&conn->chan_lock);
}
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Use locked and unlocked versions to help removing socket
locks from l2cap core functions.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 30 +++++++++++++++++++++---------
1 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a206a24..1e3ebc5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -217,6 +217,22 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
release_sock(sk);
}
+static inline void __l2cap_chan_set_err(struct l2cap_chan *chan, int err)
+{
+ struct sock *sk = chan->sk;
+
+ sk->sk_err = err;
+}
+
+static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
+{
+ struct sock *sk = chan->sk;
+
+ lock_sock(sk);
+ __l2cap_chan_set_err(chan, err);
+ release_sock(sk);
+}
+
static void l2cap_chan_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
@@ -361,7 +377,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
sock_set_flag(sk, SOCK_ZAPPED);
if (err)
- sk->sk_err = err;
+ __l2cap_chan_set_err(chan, err);
if (parent) {
bt_accept_unlink(sk);
@@ -694,14 +710,11 @@ 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;
struct l2cap_disconn_req req;
if (!conn)
return;
- sk = chan->sk;
-
if (chan->mode == L2CAP_MODE_ERTM) {
__clear_retrans_timer(chan);
__clear_monitor_timer(chan);
@@ -714,7 +727,7 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
L2CAP_DISCONN_REQ, sizeof(req), &req);
__l2cap_state_change(chan, BT_DISCONN);
- sk->sk_err = err;
+ __l2cap_chan_set_err(chan, err);
}
/* ---- L2CAP connections ---- */
@@ -953,10 +966,8 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
mutex_lock(&conn->chan_lock);
list_for_each_entry(chan, &conn->chan_l, list) {
- struct sock *sk = chan->sk;
-
if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
- sk->sk_err = err;
+ __l2cap_chan_set_err(chan, err);
}
mutex_unlock(&conn->chan_lock);
@@ -2987,7 +2998,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
}
default:
- sk->sk_err = ECONNRESET;
+ __l2cap_chan_set_err(chan, ECONNRESET);
+
__set_chan_timer(chan,
msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
l2cap_send_disconn_req(conn, chan, ECONNRESET);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Simplify code so that we do not need to check whether socket is locked.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_sock.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index bbc1747..e2fc24b 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -125,15 +125,15 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
err = l2cap_chan_connect(chan, la.l2_psm, la.l2_cid, &la.l2_bdaddr);
if (err)
- goto done;
+ return err;
lock_sock(sk);
err = bt_sock_wait_state(sk, BT_CONNECTED,
sock_sndtimeo(sk, flags & O_NONBLOCK));
-done:
- if (sock_owned_by_user(sk))
- release_sock(sk);
+
+ release_sock(sk);
+
return err;
}
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
In l2cap_conn_ready and l2cap_chan_ready change locking logic and
use explicit socket when needed.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 31810d3..6f28cf5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -920,7 +920,11 @@ clean:
static void l2cap_chan_ready(struct l2cap_chan *chan)
{
struct sock *sk = chan->sk;
- struct sock *parent = bt_sk(sk)->parent;
+ struct sock *parent;
+
+ lock_sock(sk);
+
+ parent = bt_sk(sk)->parent;
BT_DBG("sk %p, parent %p", sk, parent);
@@ -932,6 +936,8 @@ static void l2cap_chan_ready(struct l2cap_chan *chan)
if (parent)
parent->sk_data_ready(parent, 0);
+
+ release_sock(sk);
}
static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -949,23 +955,25 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
mutex_lock(&conn->chan_lock);
list_for_each_entry(chan, &conn->chan_l, list) {
- struct sock *sk = chan->sk;
- bh_lock_sock(sk);
+ l2cap_chan_lock(chan);
if (conn->hcon->type == LE_LINK) {
if (smp_conn_security(conn, chan->sec_level))
l2cap_chan_ready(chan);
} else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
+ struct sock *sk = chan->sk;
__clear_chan_timer(chan);
+ lock_sock(sk);
__l2cap_state_change(chan, BT_CONNECTED);
sk->sk_state_change(sk);
+ release_sock(sk);
} else if (chan->state == BT_CONNECT)
l2cap_do_start(chan);
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
}
mutex_unlock(&conn->chan_lock);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Function l2cap_chan_connect does not return with locked socket
anymore. So we take explicit lock in l2cap_sock_connect.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 18 ++++++++++++++----
net/bluetooth/l2cap_sock.c | 2 ++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b117014..ae4df24 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1159,7 +1159,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
hci_dev_lock(hdev);
- lock_sock(sk);
+ l2cap_chan_lock(chan);
/* PSM must be odd and lsb of upper byte must be 0 */
if ((__le16_to_cpu(psm) & 0x0101) != 0x0001 && !cid &&
@@ -1186,17 +1186,21 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
goto done;
}
+ lock_sock(sk);
+
switch (sk->sk_state) {
case BT_CONNECT:
case BT_CONNECT2:
case BT_CONFIG:
/* Already connecting */
err = 0;
+ release_sock(sk);
goto done;
case BT_CONNECTED:
/* Already connected */
err = -EISCONN;
+ release_sock(sk);
goto done;
case BT_OPEN:
@@ -1206,11 +1210,15 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
default:
err = -EBADFD;
+ release_sock(sk);
goto done;
}
/* Set destination address and psm */
bacpy(&bt_sk(sk)->dst, dst);
+
+ release_sock(sk);
+
chan->psm = psm;
chan->dcid = cid;
@@ -1238,23 +1246,25 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
/* Update source addr of the socket */
bacpy(src, conn->src);
+ l2cap_chan_unlock(chan);
l2cap_chan_add(conn, chan);
+ l2cap_chan_lock(chan);
- __l2cap_state_change(chan, BT_CONNECT);
+ l2cap_state_change(chan, BT_CONNECT);
__set_chan_timer(chan, sk->sk_sndtimeo);
if (hcon->state == BT_CONNECTED) {
if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
__clear_chan_timer(chan);
if (l2cap_chan_check_security(chan))
- __l2cap_state_change(chan, BT_CONNECTED);
+ l2cap_state_change(chan, BT_CONNECTED);
} else
l2cap_do_start(chan);
}
err = 0;
-
done:
+ l2cap_chan_unlock(chan);
hci_dev_unlock(hdev);
hci_dev_put(hdev);
return err;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index a278858..bbc1747 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -127,6 +127,8 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
if (err)
goto done;
+ lock_sock(sk);
+
err = bt_sock_wait_state(sk, BT_CONNECTED,
sock_sndtimeo(sk, flags & O_NONBLOCK));
done:
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Change bh_ locking functions to mutex_locks since we can now sleep.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 17 ++++++++++-------
1 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6f28cf5..b117014 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4571,9 +4571,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
mutex_lock(&conn->chan_lock);
list_for_each_entry(chan, &conn->chan_l, list) {
- struct sock *sk = chan->sk;
-
- bh_lock_sock(sk);
+ l2cap_chan_lock(chan);
BT_DBG("chan->scid %d", chan->scid);
@@ -4583,19 +4581,19 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
l2cap_chan_ready(chan);
}
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
if (test_bit(CONF_CONNECT_PEND, &chan->conf_state)) {
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
if (!status && (chan->state == BT_CONNECTED ||
chan->state == BT_CONFIG)) {
l2cap_check_encryption(chan, encrypt);
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
continue;
}
@@ -4616,9 +4614,12 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
}
} else if (chan->state == BT_CONNECT2) {
+ struct sock *sk = chan->sk;
struct l2cap_conn_rsp rsp;
__u16 res, stat;
+ lock_sock(sk);
+
if (!status) {
if (bt_sk(sk)->defer_setup) {
struct sock *parent = bt_sk(sk)->parent;
@@ -4639,6 +4640,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
stat = L2CAP_CS_NO_INFO;
}
+ release_sock(sk);
+
rsp.scid = cpu_to_le16(chan->dcid);
rsp.dcid = cpu_to_le16(chan->scid);
rsp.result = cpu_to_le16(res);
@@ -4647,7 +4650,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
sizeof(rsp), &rsp);
}
- bh_unlock_sock(sk);
+ l2cap_chan_unlock(chan);
}
mutex_unlock(&conn->chan_lock);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Convert sk lock to chan lock for L2CAP signalling commands.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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 727a1dd..a36b2c3 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;
@@ -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);
}
@@ -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:
@@ -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;
}
}
@@ -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 ---- */
@@ -992,7 +1003,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
{
struct l2cap_conn *conn = hcon->l2cap_data;
struct l2cap_chan *chan, *l;
- struct sock *sk;
if (!conn)
return;
@@ -1005,10 +1015,12 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
/* Kill channels */
list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);
+
l2cap_chan_del(chan, err);
- release_sock(sk);
+
+ l2cap_chan_unlock(chan);
+
chan->ops->close(chan->data);
}
@@ -2666,7 +2678,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
bt_accept_enqueue(parent, sk);
- l2cap_chan_add(conn, chan);
+ __l2cap_chan_add(conn, chan);
dcid = chan->scid;
@@ -2739,7 +2751,6 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
struct l2cap_conn_rsp *rsp = (struct l2cap_conn_rsp *) data;
u16 scid, dcid, result, status;
struct l2cap_chan *chan;
- struct sock *sk;
u8 req[128];
int err;
@@ -2769,8 +2780,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
err = 0;
- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);
switch (result) {
case L2CAP_CR_SUCCESS:
@@ -2796,7 +2806,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
break;
}
- release_sock(sk);
+ l2cap_chan_unlock(chan);
unlock:
mutex_unlock(&conn->chan_lock);
@@ -2820,7 +2830,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
u16 dcid, flags;
u8 rsp[64];
struct l2cap_chan *chan;
- struct sock *sk;
int len;
dcid = __le16_to_cpu(req->dcid);
@@ -2832,8 +2841,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
if (!chan)
return -ENOENT;
- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;
@@ -2922,7 +2930,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
}
unlock:
- release_sock(sk);
+ l2cap_chan_unlock(chan);
return 0;
}
@@ -2931,7 +2939,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
struct l2cap_conf_rsp *rsp = (struct l2cap_conf_rsp *)data;
u16 scid, flags, result;
struct l2cap_chan *chan;
- struct sock *sk;
int len = cmd->len - sizeof(*rsp);
scid = __le16_to_cpu(rsp->scid);
@@ -2945,8 +2952,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
if (!chan)
return 0;
- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);
switch (result) {
case L2CAP_CONF_SUCCESS:
@@ -3005,7 +3011,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
}
default:
- __l2cap_chan_set_err(chan, ECONNRESET);
+ l2cap_chan_set_err(chan, ECONNRESET);
__set_chan_timer(chan,
msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
@@ -3032,7 +3038,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
}
done:
- release_sock(sk);
+ l2cap_chan_unlock(chan);
return 0;
}
@@ -3057,17 +3063,21 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
return 0;
}
+ l2cap_chan_lock(chan);
+
sk = chan->sk;
- lock_sock(sk);
rsp.dcid = cpu_to_le16(chan->scid);
rsp.scid = cpu_to_le16(chan->dcid);
l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
+ lock_sock(sk);
sk->sk_shutdown = SHUTDOWN_MASK;
+ release_sock(sk);
l2cap_chan_del(chan, ECONNRESET);
- release_sock(sk);
+
+ l2cap_chan_unlock(chan);
chan->ops->close(chan->data);
@@ -3081,7 +3091,6 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
struct l2cap_disconn_rsp *rsp = (struct l2cap_disconn_rsp *) data;
u16 dcid, scid;
struct l2cap_chan *chan;
- struct sock *sk;
scid = __le16_to_cpu(rsp->scid);
dcid = __le16_to_cpu(rsp->dcid);
@@ -3096,11 +3105,11 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
return 0;
}
- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);
l2cap_chan_del(chan, 0);
- release_sock(sk);
+
+ l2cap_chan_unlock(chan);
chan->ops->close(chan->data);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1273fcb..db38fae 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -809,15 +809,18 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
if (conn)
mutex_lock(&conn->chan_lock);
-
+ l2cap_chan_lock(chan);
lock_sock(sk);
+
if (!sk->sk_shutdown) {
if (chan->mode == L2CAP_MODE_ERTM)
err = __l2cap_wait_ack(sk);
sk->sk_shutdown = SHUTDOWN_MASK;
+ release_sock(sk);
l2cap_chan_close(chan, 0);
+ lock_sock(sk);
if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
err = bt_sock_wait_state(sk, BT_CLOSED,
@@ -828,7 +831,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
err = -sk->sk_err;
release_sock(sk);
-
+ l2cap_chan_unlock(chan);
if (conn)
mutex_unlock(&conn->chan_lock);
--
1.7.9
From: Andrei Emeltchenko <[email protected]>
Usage of RCU list looks not reasonalbe for a number of reasons:
our code sleep and we have to use socket spinlocks, some parts
of code are updaters thus we need to use mutexes anyway.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 165 ++++++++++++++++++++++++++------------------
net/bluetooth/l2cap_sock.c | 10 +++
2 files changed, 108 insertions(+), 67 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 63539f9..90e29ac 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -77,36 +77,24 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
{
- struct l2cap_chan *c, *r = NULL;
-
- rcu_read_lock();
+ struct l2cap_chan *c;
- list_for_each_entry_rcu(c, &conn->chan_l, list) {
- if (c->dcid == cid) {
- r = c;
- break;
- }
+ list_for_each_entry(c, &conn->chan_l, list) {
+ if (c->dcid == cid)
+ return c;
}
-
- rcu_read_unlock();
- return r;
+ return NULL;
}
static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
{
- struct l2cap_chan *c, *r = NULL;
-
- rcu_read_lock();
+ struct l2cap_chan *c;
- list_for_each_entry_rcu(c, &conn->chan_l, list) {
- if (c->scid == cid) {
- r = c;
- break;
- }
+ list_for_each_entry(c, &conn->chan_l, list) {
+ if (c->scid == cid)
+ return c;
}
-
- rcu_read_unlock();
- return r;
+ return NULL;
}
/* Find channel with given SCID.
@@ -115,36 +103,32 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
{
struct l2cap_chan *c;
+ mutex_lock(&conn->chan_lock);
c = __l2cap_get_chan_by_scid(conn, cid);
- if (c)
- lock_sock(c->sk);
+ mutex_unlock(&conn->chan_lock);
+
return c;
}
static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
{
- struct l2cap_chan *c, *r = NULL;
-
- rcu_read_lock();
+ struct l2cap_chan *c;
- list_for_each_entry_rcu(c, &conn->chan_l, list) {
- if (c->ident == ident) {
- r = c;
- break;
- }
+ list_for_each_entry(c, &conn->chan_l, list) {
+ if (c->ident == ident)
+ return c;
}
-
- rcu_read_unlock();
- return r;
+ return NULL;
}
static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
{
struct l2cap_chan *c;
+ mutex_lock(&conn->chan_lock);
c = __l2cap_get_chan_by_ident(conn, ident);
- if (c)
- lock_sock(c->sk);
+ mutex_unlock(&conn->chan_lock);
+
return c;
}
@@ -228,11 +212,13 @@ static void l2cap_chan_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
chan_timer.work);
+ struct l2cap_conn *conn = chan->conn;
struct sock *sk = chan->sk;
int reason;
BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
+ mutex_lock(&conn->chan_lock);
lock_sock(sk);
if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
@@ -248,6 +234,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
release_sock(sk);
chan->ops->close(chan->data);
+ mutex_unlock(&conn->chan_lock);
+
l2cap_chan_put(chan);
}
@@ -331,7 +319,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
l2cap_chan_hold(chan);
- list_add_rcu(&chan->list, &conn->chan_l);
+ mutex_lock(&conn->chan_lock);
+ list_add(&chan->list, &conn->chan_l);
+ mutex_unlock(&conn->chan_lock);
}
/* Delete channel.
@@ -348,8 +338,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
if (conn) {
/* Delete from channel list */
- list_del_rcu(&chan->list);
- synchronize_rcu();
+ list_del(&chan->list);
l2cap_chan_put(chan);
@@ -400,10 +389,12 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
/* Close not yet accepted channels */
while ((sk = bt_accept_dequeue(parent, NULL))) {
struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+
__clear_chan_timer(chan);
lock_sock(sk);
l2cap_chan_close(chan, ECONNRESET);
release_sock(sk);
+
chan->ops->close(chan->data);
}
}
@@ -718,13 +709,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
/* ---- L2CAP connections ---- */
static void l2cap_conn_start(struct l2cap_conn *conn)
{
- struct l2cap_chan *chan;
+ struct l2cap_chan *chan, *tmp;
BT_DBG("conn %p", conn);
- rcu_read_lock();
+ mutex_lock(&conn->chan_lock);
- list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+ list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
struct sock *sk = chan->sk;
bh_lock_sock(sk);
@@ -804,7 +795,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
bh_unlock_sock(sk);
}
- rcu_read_unlock();
+ mutex_unlock(&conn->chan_lock);
}
/* Find socket with cid and source bdaddr.
@@ -916,9 +907,9 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
if (conn->hcon->out && conn->hcon->type == LE_LINK)
smp_conn_security(conn, conn->hcon->pending_sec_level);
- rcu_read_lock();
+ mutex_lock(&conn->chan_lock);
- list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+ list_for_each_entry(chan, &conn->chan_l, list) {
struct sock *sk = chan->sk;
bh_lock_sock(sk);
@@ -938,7 +929,7 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
bh_unlock_sock(sk);
}
- rcu_read_unlock();
+ mutex_unlock(&conn->chan_lock);
}
/* Notify sockets that we cannot guaranty reliability anymore */
@@ -948,16 +939,16 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
BT_DBG("conn %p", conn);
- rcu_read_lock();
+ mutex_lock(&conn->chan_lock);
- list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+ list_for_each_entry(chan, &conn->chan_l, list) {
struct sock *sk = chan->sk;
if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
sk->sk_err = err;
}
- rcu_read_unlock();
+ mutex_unlock(&conn->chan_lock);
}
static void l2cap_info_timeout(struct work_struct *work)
@@ -984,6 +975,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
kfree_skb(conn->rx_skb);
+ mutex_lock(&conn->chan_lock);
+
/* Kill channels */
list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
sk = chan->sk;
@@ -993,6 +986,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
chan->ops->close(chan->data);
}
+ mutex_unlock(&conn->chan_lock);
+
hci_chan_del(conn->hchan);
if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
@@ -1050,6 +1045,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
conn->feat_mask = 0;
spin_lock_init(&conn->lock);
+ mutex_init(&conn->chan_lock);
INIT_LIST_HEAD(&conn->chan_l);
@@ -1792,9 +1788,9 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
BT_DBG("conn %p", conn);
- rcu_read_lock();
+ mutex_lock(&conn->chan_lock);
- list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+ list_for_each_entry(chan, &conn->chan_l, list) {
struct sock *sk = chan->sk;
if (chan->chan_type != L2CAP_CHAN_RAW)
continue;
@@ -1810,7 +1806,7 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
kfree_skb(nskb);
}
- rcu_read_unlock();
+ mutex_unlock(&conn->chan_lock);
}
/* ---- L2CAP signalling commands ---- */
@@ -2600,6 +2596,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
parent = pchan->sk;
+ mutex_lock(&conn->chan_lock);
lock_sock(parent);
/* Check if the ACL is secure enough (if not SDP) */
@@ -2673,6 +2670,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
response:
release_sock(parent);
+ mutex_unlock(&conn->chan_lock);
sendresp:
rsp.scid = cpu_to_le16(scid);
@@ -2714,6 +2712,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
struct l2cap_chan *chan;
struct sock *sk;
u8 req[128];
+ int err;
scid = __le16_to_cpu(rsp->scid);
dcid = __le16_to_cpu(rsp->dcid);
@@ -2723,17 +2722,26 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
dcid, scid, result, status);
+ mutex_lock(&conn->chan_lock);
+
if (scid) {
- chan = l2cap_get_chan_by_scid(conn, scid);
- if (!chan)
- return -EFAULT;
+ chan = __l2cap_get_chan_by_scid(conn, scid);
+ if (!chan) {
+ err = -EFAULT;
+ goto unlock;
+ }
} else {
- chan = l2cap_get_chan_by_ident(conn, cmd->ident);
- if (!chan)
- return -EFAULT;
+ chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
+ if (!chan) {
+ err = -EFAULT;
+ goto unlock;
+ }
}
+ err = 0;
+
sk = chan->sk;
+ lock_sock(sk);
switch (result) {
case L2CAP_CR_SUCCESS:
@@ -2760,7 +2768,10 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
}
release_sock(sk);
- return 0;
+unlock:
+ mutex_unlock(&conn->chan_lock);
+
+ return err;
}
static inline void set_default_fcs(struct l2cap_chan *chan)
@@ -2793,6 +2804,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
return -ENOENT;
sk = chan->sk;
+ lock_sock(sk);
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;
@@ -2905,6 +2917,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
return 0;
sk = chan->sk;
+ lock_sock(sk);
switch (result) {
case L2CAP_CONF_SUCCESS:
@@ -3006,11 +3019,16 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
- chan = l2cap_get_chan_by_scid(conn, dcid);
- if (!chan)
+ mutex_lock(&conn->chan_lock);
+
+ chan = __l2cap_get_chan_by_scid(conn, dcid);
+ if (!chan) {
+ mutex_unlock(&conn->chan_lock);
return 0;
+ }
sk = chan->sk;
+ lock_sock(sk);
rsp.dcid = cpu_to_le16(chan->scid);
rsp.scid = cpu_to_le16(chan->dcid);
@@ -3022,6 +3040,9 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
release_sock(sk);
chan->ops->close(chan->data);
+
+ mutex_unlock(&conn->chan_lock);
+
return 0;
}
@@ -3037,16 +3058,24 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
- chan = l2cap_get_chan_by_scid(conn, scid);
- if (!chan)
+ mutex_lock(&conn->chan_lock);
+
+ chan = __l2cap_get_chan_by_scid(conn, scid);
+ if (!chan) {
+ mutex_unlock(&conn->chan_lock);
return 0;
+ }
sk = chan->sk;
+ lock_sock(sk);
l2cap_chan_del(chan, 0);
release_sock(sk);
chan->ops->close(chan->data);
+
+ mutex_unlock(&conn->chan_lock);
+
return 0;
}
@@ -4205,6 +4234,7 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
}
sk = chan->sk;
+ lock_sock(sk);
BT_DBG("chan %p, len %d", chan, skb->len);
@@ -4492,9 +4522,9 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
cancel_delayed_work(&conn->security_timer);
}
- rcu_read_lock();
+ mutex_lock(&conn->chan_lock);
- list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+ list_for_each_entry(chan, &conn->chan_l, list) {
struct sock *sk = chan->sk;
bh_lock_sock(sk);
@@ -4574,7 +4604,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
bh_unlock_sock(sk);
}
- rcu_read_unlock();
+ mutex_unlock(&conn->chan_lock);
return 0;
}
@@ -4635,6 +4665,7 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
if (chan && chan->sk) {
struct sock *sk = chan->sk;
+ lock_sock(sk);
if (chan->imtu < len - L2CAP_HDR_SIZE) {
BT_ERR("Frame exceeding recv MTU (len %d, "
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index b48d6c1..1273fcb 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -796,6 +796,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
{
struct sock *sk = sock->sk;
struct l2cap_chan *chan;
+ struct l2cap_conn *conn;
int err = 0;
BT_DBG("sock %p, sk %p", sock, sk);
@@ -804,6 +805,10 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
return 0;
chan = l2cap_pi(sk)->chan;
+ conn = chan->conn;
+
+ if (conn)
+ mutex_lock(&conn->chan_lock);
lock_sock(sk);
if (!sk->sk_shutdown) {
@@ -811,6 +816,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
err = __l2cap_wait_ack(sk);
sk->sk_shutdown = SHUTDOWN_MASK;
+
l2cap_chan_close(chan, 0);
if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
@@ -822,6 +828,10 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
err = -sk->sk_err;
release_sock(sk);
+
+ if (conn)
+ mutex_unlock(&conn->chan_lock);
+
return err;
}
--
1.7.9