2012-02-10 13:54:57

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 00/16] Bluetooth: Change socket lock to l2cap_chan lock

From: Andrei Emeltchenko <[email protected]>

This set is still WIP, hope to send PATCH series next week.

Those small patches which dealing with locks need to be applied at once,
if needed they can be merged together. Please voice opinion about the split.

Changes:
* 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.

This is DRAFT RFC introducing l2cap channel lock. Please make suggestion how to
make it better.

Andrei Emeltchenko (16):
Bluetooth: trivial: Fix long line
Bluetooth: Revert to mutexes from RCU list
Bluetooth: Do not use sk lock in get_chan functions
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 chan delete functions
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 | 386 +++++++++++++++++++++++++----------------
net/bluetooth/l2cap_sock.c | 23 ++-
3 files changed, 264 insertions(+), 156 deletions(-)

Cumulutive change from RFCv3->RFCv4

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9d03c46..bb0d9f7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -244,19 +244,19 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
release_sock(sk);
}

-static inline void __l2cap_set_sock_err(struct l2cap_chan *chan, int err)
+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_set_sock_err(struct l2cap_chan *chan, int err)
+static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
{
struct sock *sk = chan->sk;

lock_sock(sk);
- __l2cap_set_sock_err(chan, err);
+ __l2cap_chan_set_err(chan, err);
release_sock(sk);
}

@@ -264,11 +264,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;
+
int reason;

BT_DBG("chan %p state %d", chan, chan->state);

- mutex_lock(&chan->conn->chan_lock);
+ mutex_lock(&conn->chan_lock);
l2cap_chan_lock(chan);

if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
@@ -282,7 +284,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
l2cap_chan_close(chan, reason);

l2cap_chan_unlock(chan);
- mutex_unlock(&chan->conn->chan_lock);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
l2cap_chan_put(chan);
@@ -420,7 +422,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
sock_set_flag(sk, SOCK_ZAPPED);

if (err)
- __l2cap_set_sock_err(chan, err);
+ __l2cap_chan_set_err(chan, err);

if (parent) {
bt_accept_unlink(sk);
@@ -778,7 +780,7 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c

lock_sock(sk);
__l2cap_state_change(chan, BT_DISCONN);
- __l2cap_set_sock_err(chan, err);
+ __l2cap_chan_set_err(chan, err);
release_sock(sk);
}

@@ -1027,7 +1029,7 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)

list_for_each_entry(chan, &conn->chan_l, list) {
if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
- l2cap_set_sock_err(chan, err);
+ l2cap_chan_set_err(chan, err);
}

mutex_unlock(&conn->chan_lock);
@@ -1231,12 +1233,14 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
case BT_CONFIG:
/* Already connecting */
err = 0;
- goto sock_release;
+ release_sock(sk);
+ goto done;

case BT_CONNECTED:
/* Already connected */
err = -EISCONN;
- goto sock_release;
+ release_sock(sk);
+ goto done;

case BT_OPEN:
case BT_BOUND:
@@ -1245,7 +1249,8 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d

default:
err = -EBADFD;
- goto sock_release;
+ release_sock(sk);
+ goto done;
}

/* Set destination address and psm */
@@ -1297,11 +1302,6 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
}

err = 0;
- goto done;
-
-sock_release:
- release_sock(sk);
-
done:
l2cap_chan_unlock(chan);
hci_dev_unlock(hdev);
@@ -2886,7 +2886,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);
@@ -2900,8 +2899,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr

l2cap_chan_lock(chan);

- sk = chan->sk;
-
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;

@@ -2998,7 +2995,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);
@@ -3014,8 +3010,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr

l2cap_chan_lock(chan);

- sk = chan->sk;
-
switch (result) {
case L2CAP_CONF_SUCCESS:
l2cap_conf_rfc_get(chan, rsp->data, len);
@@ -3073,7 +3067,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
}

default:
- l2cap_set_sock_err(chan, ECONNRESET);
+ l2cap_chan_set_err(chan, ECONNRESET);

__set_chan_timer(chan,
msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));

--
1.7.8.3



2012-02-15 17:26:34

by Ulisses Furquim

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Andrei,

On Wed, Feb 15, 2012 at 7:24 AM, Emeltchenko Andrei
<[email protected]> wrote:
> Hi Ulisses,
>
> On Wed, Feb 15, 2012 at 10:16:53AM +0200, Emeltchenko Andrei wrote:
>> On Tue, Feb 14, 2012 at 03:23:54PM -0200, Ulisses Furquim wrote:
>> > On Tue, Feb 14, 2012 at 11:21 AM, Emeltchenko Andrei
>> > <[email protected]> wrote:
>> > > Hi Ulisses,
>> > >
>> > > On Mon, Feb 13, 2012 at 10:06:10PM -0300, Ulisses Furquim wrote:
>> > > ...
>> > >> Yes, I do think they belong together. And please, check l2cap_sock.c
>> > >> where l2cap_chan_close() seems to be called without locking
>> > >> conn->chan_lock in l2cap_sock_shutdown().
>> > >
>> > > In that context we do not always have l2cap_conn so maybe we return
>> > > chan list lock to chan_del or invent unlocked chan_del / chan_close?
>> >
>> > We don't have l2cap_conn? So are we already on conn->chan_l list or
>> > not? Maybe it's better to check that instead of changing everything
>> > now.
>
> Maybe this check?
>
> ? ? ? ?if (!sk->sk_shutdown) {
> + ? ? ? ? ? ? ? struct l2cap_conn *conn = chan->conn;
> +
> ? ? ? ? ? ? ? ?if (chan->mode == L2CAP_MODE_ERTM)
> ? ? ? ? ? ? ? ? ? ? ? ?err = __l2cap_wait_ack(sk);
>
> ? ? ? ? ? ? ? ?sk->sk_shutdown = SHUTDOWN_MASK;
> ? ? ? ? ? ? ? ?release_sock(sk);
> +
> + ? ? ? ? ? ? ? if (conn)
> + ? ? ? ? ? ? ? ? ? ? ? mutex_lock(&conn->chan_lock);
> ? ? ? ? ? ? ? ?l2cap_chan_close(chan, 0);
> + ? ? ? ? ? ? ? if (conn)
> + ? ? ? ? ? ? ? ? ? ? ? mutex_unlock(&conn->chan_lock);
> +
> ? ? ? ? ? ? ? ?lock_sock(sk);
>
> ? ? ? ? ? ? ? ?if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
> ? ? ? ? ? ? ? ? ? ? ? ?err = bt_sock_wait_state(sk, BT_CLOSED,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?sk->sk_lingertime);
>
> If it does not look too hackish...

l2cap_chan_del() is already doing the exact same test for conn to
remove it from conn->chan_l or not. Maybe add a comment to make it
more clear?

Regards,

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

2012-02-15 09:24:16

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Ulisses,

On Wed, Feb 15, 2012 at 10:16:53AM +0200, Emeltchenko Andrei wrote:
> On Tue, Feb 14, 2012 at 03:23:54PM -0200, Ulisses Furquim wrote:
> > On Tue, Feb 14, 2012 at 11:21 AM, Emeltchenko Andrei
> > <[email protected]> wrote:
> > > Hi Ulisses,
> > >
> > > On Mon, Feb 13, 2012 at 10:06:10PM -0300, Ulisses Furquim wrote:
> > > ...
> > >> Yes, I do think they belong together. And please, check l2cap_sock.c
> > >> where l2cap_chan_close() seems to be called without locking
> > >> conn->chan_lock in l2cap_sock_shutdown().
> > >
> > > In that context we do not always have l2cap_conn so maybe we return
> > > chan list lock to chan_del or invent unlocked chan_del / chan_close?
> >
> > We don't have l2cap_conn? So are we already on conn->chan_l list or
> > not? Maybe it's better to check that instead of changing everything
> > now.

Maybe this check?

if (!sk->sk_shutdown) {
+ struct l2cap_conn *conn = chan->conn;
+
if (chan->mode == L2CAP_MODE_ERTM)
err = __l2cap_wait_ack(sk);

sk->sk_shutdown = SHUTDOWN_MASK;
release_sock(sk);
+
+ if (conn)
+ mutex_lock(&conn->chan_lock);
l2cap_chan_close(chan, 0);
+ if (conn)
+ mutex_unlock(&conn->chan_lock);
+
lock_sock(sk);

if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
err = bt_sock_wait_state(sk, BT_CLOSED,
sk->sk_lingertime);

If it does not look too hackish...

Best regards
Andrei Emeltchenko


2012-02-15 08:21:35

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [RFCv4 06/16] Bluetooth: Add socket error function

Hi Gustavo,

On Tue, Feb 14, 2012 at 04:39:55PM -0200, Gustavo Padovan wrote:
> > @@ -974,10 +987,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);
>
> Why is this l2cap_chan_set_err()? This function wasn't holding the lock.
> Please justify this change.

You are right here, will change it to unlocked one.

Best regards
Andrei Emeltchenko

2012-02-15 08:16:53

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Ulisses,

On Tue, Feb 14, 2012 at 03:23:54PM -0200, Ulisses Furquim wrote:
> On Tue, Feb 14, 2012 at 11:21 AM, Emeltchenko Andrei
> <[email protected]> wrote:
> > Hi Ulisses,
> >
> > On Mon, Feb 13, 2012 at 10:06:10PM -0300, Ulisses Furquim wrote:
> > ...
> >> Yes, I do think they belong together. And please, check l2cap_sock.c
> >> where l2cap_chan_close() seems to be called without locking
> >> conn->chan_lock in l2cap_sock_shutdown().
> >
> > In that context we do not always have l2cap_conn so maybe we return
> > chan list lock to chan_del or invent unlocked chan_del / chan_close?
>
> We don't have l2cap_conn? So are we already on conn->chan_l list or
> not? Maybe it's better to check that instead of changing everything
> now.

I am afraid that the logic in l2cap_chan_close is a bit complicated, for
listening socket is recursively invokes itself for every sk in accept
queue. conn is created when connection is established, before that chan is
added only to global chan list. So if there is no connection we cannot get
lock in conn->chan_lock.

I believe that it is better to keep locking inside of chan_del.

Best regards
Andrei Emeltchenko


2012-02-14 18:39:55

by Gustavo Padovan

[permalink] [raw]
Subject: Re: [RFCv4 06/16] Bluetooth: Add socket error function

Hi Andrei,

* Emeltchenko Andrei <[email protected]> [2012-02-10 15:55:03 +0200]:

> From: Andrei Emeltchenko <[email protected]>
>
> Use locked and unlocked versions to help removing socket
> locks from l2cap.
>
> 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 5120a1c..e0e4cc8 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -243,6 +243,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,
> @@ -383,7 +399,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);
> @@ -715,14 +731,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);
> @@ -735,7 +748,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 ---- */
> @@ -974,10 +987,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);

Why is this l2cap_chan_set_err()? This function wasn't holding the lock.
Please justify this change.

Gustavo

2012-02-14 17:23:54

by Ulisses Furquim

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Andrei,

On Tue, Feb 14, 2012 at 11:21 AM, Emeltchenko Andrei
<[email protected]> wrote:
> Hi Ulisses,
>
> On Mon, Feb 13, 2012 at 10:06:10PM -0300, Ulisses Furquim wrote:
> ...
>> Yes, I do think they belong together. And please, check l2cap_sock.c
>> where l2cap_chan_close() seems to be called without locking
>> conn->chan_lock in l2cap_sock_shutdown().
>
> In that context we do not always have l2cap_conn so maybe we return
> chan list lock to chan_del or invent unlocked chan_del / chan_close?

We don't have l2cap_conn? So are we already on conn->chan_l list or
not? Maybe it's better to check that instead of changing everything
now.

> So far the easiest way would be first solution.
>
>> And please remove the bogus
>> comment below from l2cap_conn_start, ok?
>
> I believe this is removed in later patches.

If so, then ok, thanks.

Regards,

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

2012-02-14 13:21:20

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Ulisses,

On Mon, Feb 13, 2012 at 10:06:10PM -0300, Ulisses Furquim wrote:
...
> Yes, I do think they belong together. And please, check l2cap_sock.c
> where l2cap_chan_close() seems to be called without locking
> conn->chan_lock in l2cap_sock_shutdown().

In that context we do not always have l2cap_conn so maybe we return
chan list lock to chan_del or invent unlocked chan_del / chan_close?

So far the easiest way would be first solution.

> And please remove the bogus
> comment below from l2cap_conn_start, ok?

I believe this is removed in later patches.

Best regards
Andrei Emeltchenko

>
> /*?l2cap_chan_close()?calls?list_del(chan)
> ? *? so?release?the?lock?*/
>


2012-02-14 01:06:10

by Ulisses Furquim

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Andrei,

On Mon, Feb 13, 2012 at 11:49 AM, Emeltchenko Andrei
<[email protected]> wrote:
>
> Hi Ulisses,
>
> On Mon, Feb 13, 2012 at 10:58:43AM +0200, Emeltchenko Andrei wrote:
> > Hi Ulisses,
> >
> > On Fri, Feb 10, 2012 at 04:24:57PM -0200, Ulisses Furquim wrote:
> > > Hi Andrei,
> > >
> > > On Fri, Feb 10, 2012 at 11:54 AM, 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]>
> > >
> > > No need to lock and unlock conn->chan_lock in l2cap_disconnect_rsp()?
> > > This change from RCU to mutexes really should be just one commit IMO.
> >
> > I try to add chunks which are not in different patches but then this patch
> > would several hundreds lines long. If this OK I just merge them.
>
> I've picked chunks which might come after this patch that are dealing with
> locking conn->chan_lock. Please check it below. Do you think it needs to
> be merged with "RCU remove" patch?
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c0a35c5..356ce6e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -240,11 +240,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 %d", chan, chan->state);
>
> + ? ? ? mutex_lock(&conn->chan_lock);
> ? ? ? ?lock_sock(sk);
>
> ? ? ? ?if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> @@ -258,6 +260,7 @@ static void l2cap_chan_timeout(struct work_struct
> *work)
> ? ? ? ?l2cap_chan_close(chan, reason);
>
> ? ? ? ?release_sock(sk);
> + ? ? ? mutex_unlock(&conn->chan_lock);
>
> ? ? ? ?chan->ops->close(chan->data);
> ? ? ? ?l2cap_chan_put(chan);
> @@ -2619,6 +2622,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) */
> @@ -2692,6 +2696,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);
> @@ -2733,6 +2738,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);
> @@ -2742,16 +2748,24 @@ 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;
>
> ? ? ? ?switch (result) {
> @@ -2779,7 +2793,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)
> @@ -3025,9 +3042,13 @@ 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;
>
> @@ -3039,6 +3060,7 @@ static inline int l2cap_disconnect_req(struct
> l2cap_conn *conn, struct l2cap_cmd
>
> ? ? ? ?l2cap_chan_del(chan, ECONNRESET);
> ? ? ? ?release_sock(sk);
> + ? ? ? mutex_unlock(&conn->chan_lock);
>
> ? ? ? ?chan->ops->close(chan->data);
> ? ? ? ?return 0;
> @@ -3056,14 +3078,19 @@ 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;
>
> ? ? ? ?l2cap_chan_del(chan, 0);
> ? ? ? ?release_sock(sk);
> + ? ? ? mutex_unlock(&conn->chan_lock);
>
> ? ? ? ?chan->ops->close(chan->data);
> ? ? ? ?return 0;
> --
> 1.7.8.3


Yes, I do think they belong together. And please, check l2cap_sock.c
where l2cap_chan_close() seems to be called without locking
conn->chan_lock in l2cap_sock_shutdown(). And please remove the bogus
comment below from l2cap_conn_start, ok?

/*?l2cap_chan_close()?calls?list_del(chan)
? *? so?release?the?lock?*/

Thanks,
Regards,

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

2012-02-13 14:49:13

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Ulisses,

On Mon, Feb 13, 2012 at 10:58:43AM +0200, Emeltchenko Andrei wrote:
> Hi Ulisses,
>
> On Fri, Feb 10, 2012 at 04:24:57PM -0200, Ulisses Furquim wrote:
> > Hi Andrei,
> >
> > On Fri, Feb 10, 2012 at 11:54 AM, 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]>
> >
> > No need to lock and unlock conn->chan_lock in l2cap_disconnect_rsp()?
> > This change from RCU to mutexes really should be just one commit IMO.
>
> I try to add chunks which are not in different patches but then this patch
> would several hundreds lines long. If this OK I just merge them.

I've picked chunks which might come after this patch that are dealing with
locking conn->chan_lock. Please check it below. Do you think it needs to
be merged with "RCU remove" patch?

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c0a35c5..356ce6e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -240,11 +240,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 %d", chan, chan->state);

+ mutex_lock(&conn->chan_lock);
lock_sock(sk);

if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
@@ -258,6 +260,7 @@ static void l2cap_chan_timeout(struct work_struct
*work)
l2cap_chan_close(chan, reason);

release_sock(sk);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
l2cap_chan_put(chan);
@@ -2619,6 +2622,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) */
@@ -2692,6 +2696,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);
@@ -2733,6 +2738,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);
@@ -2742,16 +2748,24 @@ 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;

switch (result) {
@@ -2779,7 +2793,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)
@@ -3025,9 +3042,13 @@ 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;

@@ -3039,6 +3060,7 @@ static inline int l2cap_disconnect_req(struct
l2cap_conn *conn, struct l2cap_cmd

l2cap_chan_del(chan, ECONNRESET);
release_sock(sk);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
return 0;
@@ -3056,14 +3078,19 @@ 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;

l2cap_chan_del(chan, 0);
release_sock(sk);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
return 0;
--
1.7.8.3


Best regards
Andrei Emeltchenko

2012-02-13 08:58:43

by Andrei Emeltchenko

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Ulisses,

On Fri, Feb 10, 2012 at 04:24:57PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
>
> On Fri, Feb 10, 2012 at 11:54 AM, 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]>
>
> No need to lock and unlock conn->chan_lock in l2cap_disconnect_rsp()?
> This change from RCU to mutexes really should be just one commit IMO.

I try to add chunks which are not in different patches but then this patch
would several hundreds lines long. If this OK I just merge them.

> This series is starting to get all confused. The change to RCU was
> only one commit so it should be possible to do the "revert" without
> breaking anything.

"Anything" is already broken in a sense that RCU updaters are not
protected at all. So the change does not make it more or less broken.

Best regards
Andrei Emeltchenko


2012-02-10 18:24:57

by Ulisses Furquim

[permalink] [raw]
Subject: Re: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

Hi Andrei,

On Fri, Feb 10, 2012 at 11:54 AM, 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 | ? 98 ++++++++++++++++++++-----------------------
> ?1 files changed, 46 insertions(+), 52 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 8dfccb3..ffc4179 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,34 @@ 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;
> ?}
>
> @@ -357,7 +343,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.
> @@ -374,8 +362,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);
>
> @@ -426,10 +413,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);
> ? ? ? ?}
> ?}
> @@ -743,13 +732,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);
> @@ -829,7 +818,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.
> @@ -941,9 +930,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);
> @@ -963,7 +952,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 */
> @@ -973,16 +962,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)
> @@ -1009,6 +998,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;
> @@ -1018,6 +1009,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)
> @@ -1075,6 +1068,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);
>
> @@ -1815,9 +1809,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;
> @@ -1833,7 +1827,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 ---- */
> @@ -4515,9 +4509,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);
> @@ -4597,7 +4591,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;
> ?}
> --
> 1.7.8.3

No need to lock and unlock conn->chan_lock in l2cap_disconnect_rsp()?
This change from RCU to mutexes really should be just one commit IMO.
This series is starting to get all confused. The change to RCU was
only one commit so it should be possible to do the "revert" without
breaking anything.

Regards,

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

2012-02-10 13:55:03

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 06/16] Bluetooth: Add socket error function

From: Andrei Emeltchenko <[email protected]>

Use locked and unlocked versions to help removing socket
locks from l2cap.

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 5120a1c..e0e4cc8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -243,6 +243,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,
@@ -383,7 +399,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);
@@ -715,14 +731,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);
@@ -735,7 +748,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 ---- */
@@ -974,10 +987,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);
@@ -2992,7 +3003,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.8.3


2012-02-10 13:55:10

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 13/16] Bluetooth: Change locking logic for conn/chan ready

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 c6fdc7c..ad5092e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -945,7 +945,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);

@@ -957,6 +961,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)
@@ -974,23 +980,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.8.3


2012-02-10 13:55:08

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 11/16] Bluetooth: Use chan lock in L2CAP conn start

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 9ea7758..cb4a96d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -782,10 +782,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;
}

@@ -794,17 +794,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;
}

@@ -824,6 +822,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);
@@ -836,6 +835,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);
@@ -846,7 +846,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;
}

@@ -856,7 +856,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.8.3


2012-02-10 13:55:13

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 16/16] Bluetooth: Remove socket lock check

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 ddac4cb..358975a 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.8.3


2012-02-10 13:55:12

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 15/16] Bluetooth: Use l2cap chan lock in socket connect

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 cbaa4e1..df5e82f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1184,7 +1184,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 &&
@@ -1211,17 +1211,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:
@@ -1231,11 +1235,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;

@@ -1263,23 +1271,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 dea5418..ddac4cb 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.8.3


2012-02-10 13:55:02

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 05/16] Bluetooth: Add locked and unlocked state_change

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 9cb8f48..5120a1c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -225,7 +225,7 @@ static char *state_to_string(int state)
return "invalid state";
}

-static void l2cap_state_change(struct l2cap_chan *chan, int state)
+static void __l2cap_state_change(struct l2cap_chan *chan, int state)
{
BT_DBG("%p %s -> %s", chan, state_to_string(chan->state),
state_to_string(state));
@@ -234,6 +234,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,
@@ -370,7 +379,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)
@@ -434,7 +443,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;

@@ -725,7 +734,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;
}

@@ -791,7 +800,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);
}
@@ -894,7 +903,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:
@@ -911,7 +920,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)
@@ -943,7 +952,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)
@@ -1217,14 +1226,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);
}
@@ -2668,22 +2677,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;
}
@@ -4574,12 +4583,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.8.3


2012-02-10 13:55:05

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 08/16] Bluetooth: Use chan lock in timers

From: Andrei Emeltchenko <[email protected]>

Change locking in delayed works to use chan locks instead of sk lock.

Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 34 +++++++++++++++++++---------------
1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 632cb2d..c8cac0e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -263,12 +263,14 @@ static void l2cap_chan_timeout(struct work_struct *work)
{
struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
chan_timer.work);
- struct sock *sk = chan->sk;
+ struct l2cap_conn *conn = chan->conn;
+
int reason;

BT_DBG("chan %p state %d", chan, chan->state);

- lock_sock(sk);
+ mutex_lock(&conn->chan_lock);
+ l2cap_chan_lock(chan);

if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
reason = ECONNREFUSED;
@@ -280,7 +282,8 @@ static void l2cap_chan_timeout(struct work_struct *work)

l2cap_chan_close(chan, reason);

- release_sock(sk);
+ l2cap_chan_unlock(chan);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
l2cap_chan_put(chan);
@@ -1298,40 +1301,39 @@ 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);
- return;
+ goto done;
}

chan->retry_count++;
__set_monitor_timer(chan);

l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
- release_sock(sk);
+done:
+ 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)
@@ -2020,9 +2022,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.8.3


2012-02-10 13:55:09

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 12/16] Bluetooth: Use chan lock in receiving data

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 | 7 ++-----
net/bluetooth/l2cap_sock.c | 11 +++++++++--
2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index cb4a96d..c6fdc7c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4281,7 +4281,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;
@@ -4292,8 +4291,7 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
goto drop;
}

- sk = chan->sk;
- lock_sock(sk);
+ l2cap_chan_lock(chan);

BT_DBG("chan %p, len %d", chan, skb->len);

@@ -4364,8 +4362,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 85e4fd7..dea5418 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -864,8 +864,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);

@@ -884,6 +888,9 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
err = 0;
}

+done:
+ release_sock(sk);
+
return err;
}

--
1.7.8.3


2012-02-10 13:55:06

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 09/16] Bluetooth: Use chan lock in L2CAP sig commands

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 | 85 +++++++++++++++++++++++++++++---------------
1 files changed, 56 insertions(+), 29 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c8cac0e..94938f0 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -739,6 +739,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)
@@ -755,8 +756,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 ---- */
@@ -2646,6 +2649,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) */
@@ -2686,7 +2690,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;

@@ -2719,6 +2723,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);
@@ -2760,6 +2765,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);
@@ -2769,18 +2775,27 @@ 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;
+
+ l2cap_chan_lock(chan);
+
sk = chan->sk;
- lock_sock(sk);

switch (result) {
case L2CAP_CR_SUCCESS:
@@ -2806,8 +2821,11 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
break;
}

- release_sock(sk);
- return 0;
+ l2cap_chan_unlock(chan);
+unlock:
+ mutex_unlock(&conn->chan_lock);
+
+ return err;
}

static inline void set_default_fcs(struct l2cap_chan *chan)
@@ -2827,7 +2845,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);
@@ -2839,8 +2856,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;
@@ -2929,7 +2945,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;
}

@@ -2938,7 +2954,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);
@@ -2952,8 +2967,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:
@@ -3012,7 +3026,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));
@@ -3039,7 +3053,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;
}

@@ -3056,21 +3070,30 @@ 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;
+ }
+
+ 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);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
return 0;
@@ -3081,22 +3104,26 @@ 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);

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_lock(chan);

l2cap_chan_del(chan, 0);
- release_sock(sk);
+
+ l2cap_chan_unlock(chan);
+ mutex_unlock(&conn->chan_lock);

chan->ops->close(chan->data);
return 0;
--
1.7.8.3


2012-02-10 13:55:07

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 10/16] Bluetooth: Use chan lock in chan delete functions

From: Andrei Emeltchenko <[email protected]>

Change sk lock to chan lock in functions deleting L2CAP
channels.

Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 21 +++++++++++++++------
net/bluetooth/l2cap_sock.c | 2 ++
2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 94938f0..9ea7758 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -403,6 +403,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);

@@ -415,6 +417,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;
@@ -447,10 +451,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);
}
@@ -465,10 +469,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:
@@ -511,7 +517,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;
}
}
@@ -1020,7 +1028,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;
@@ -1033,10 +1040,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);
}

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1636029..85e4fd7 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -809,7 +809,9 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
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,
--
1.7.8.3


2012-02-10 13:55:04

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 07/16] Bluetooth: Add unlocked __l2cap_chan_add function

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 e0e4cc8..632cb2d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -322,7 +322,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);
@@ -368,8 +368,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.8.3


2012-02-10 13:55:01

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 04/16] Bluetooth: Add l2cap_chan_lock

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 42fdbb8..f9fe348 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 7aa6e7f..9cb8f48 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -269,6 +269,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.8.3


2012-02-10 13:55:00

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 03/16] Bluetooth: Do not use sk lock in get_chan functions

From: Andrei Emeltchenko <[email protected]>

Take explicit lock. This change is needed to get rid of
socket locks in L2CAP core code. Socket locks will be changed
by chan locks in the following patches.

Signed-off-by: Andrei Emeltchenko <[email protected]>
---
net/bluetooth/l2cap_core.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ffc4179..7aa6e7f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci

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;
}

@@ -128,9 +127,8 @@ static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn

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;
}

@@ -2751,6 +2749,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
}

sk = chan->sk;
+ lock_sock(sk);

switch (result) {
case L2CAP_CR_SUCCESS:
@@ -2810,6 +2809,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;
@@ -2922,6 +2922,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:
@@ -3028,6 +3029,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
return 0;

sk = chan->sk;
+ lock_sock(sk);

rsp.dcid = cpu_to_le16(chan->scid);
rsp.scid = cpu_to_le16(chan->dcid);
@@ -3059,6 +3061,7 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
return 0;

sk = chan->sk;
+ lock_sock(sk);

l2cap_chan_del(chan, 0);
release_sock(sk);
@@ -4222,6 +4225,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);

@@ -4652,6 +4656,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, "
--
1.7.8.3


2012-02-10 13:54:59

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 02/16] Bluetooth: Revert to mutexes from RCU list

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 | 98 ++++++++++++++++++++-----------------------
1 files changed, 46 insertions(+), 52 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8dfccb3..ffc4179 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,34 @@ 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;
}

@@ -357,7 +343,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.
@@ -374,8 +362,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);

@@ -426,10 +413,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);
}
}
@@ -743,13 +732,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);
@@ -829,7 +818,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.
@@ -941,9 +930,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);
@@ -963,7 +952,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 */
@@ -973,16 +962,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)
@@ -1009,6 +998,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;
@@ -1018,6 +1009,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)
@@ -1075,6 +1068,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);

@@ -1815,9 +1809,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;
@@ -1833,7 +1827,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 ---- */
@@ -4515,9 +4509,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);
@@ -4597,7 +4591,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;
}
--
1.7.8.3


2012-02-10 13:54:58

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 01/16] Bluetooth: trivial: Fix long line

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 f1a6b3c..8dfccb3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2743,7 +2743,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.8.3


2012-02-10 13:55:11

by Andrei Emeltchenko

[permalink] [raw]
Subject: [RFCv4 14/16] Bluetooth: Change locking logic in security_cfm

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 ad5092e..cbaa4e1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4589,9 +4589,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);

@@ -4601,19 +4599,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;
}

@@ -4634,9 +4632,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;
@@ -4657,6 +4658,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);
@@ -4665,7 +4668,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.8.3