These patches are now based on the bluetooth repository instead of
bluetooth-next.
Mat Martineau (2):
Bluetooth: Restore locking semantics when looking up L2CAP channels
Bluetooth: Lock the L2CAP channel when sending
include/net/bluetooth/bluetooth.h | 2 --
net/bluetooth/l2cap_core.c | 10 +++-------
net/bluetooth/l2cap_sock.c | 19 +++++++++++--------
3 files changed, 14 insertions(+), 17 deletions(-)
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-05-16 17:50:39 +0300]:
> Hi Gustavo,
>
> On Tue, May 08, 2012 at 04:44:51AM -0300, Gustavo Padovan wrote:
> > > > Patch has been applied to the bluetooth tree. Thanks.
> > >
> > > What about applying the first patch? I actually pulled that patch from
> > > bluetooth-next and modified locking in my code but now that patch seems to
> > > be lost.
> >
> > I applied it to bluetooth.git. Now you have to wait we merge bluetooth.git
> > into bluetooth-next.git, that will happen shortly after John pull the pull
> > request I sent.
>
> What about the patch in question? Since it was in repository before I have
> modified my code and changed locking logic but then patch disappeared.
>
> Not that I think the patch is very important but I do not work to modify code too
> often...
The patches that were in bluetooth.git tree are in bluetooth-next.git. There
were reject in the pull request and I had to move them out.
Gustavo
Hi Gustavo,
On Tue, May 08, 2012 at 04:44:51AM -0300, Gustavo Padovan wrote:
> > > Patch has been applied to the bluetooth tree. Thanks.
> >
> > What about applying the first patch? I actually pulled that patch from
> > bluetooth-next and modified locking in my code but now that patch seems to
> > be lost.
>
> I applied it to bluetooth.git. Now you have to wait we merge bluetooth.git
> into bluetooth-next.git, that will happen shortly after John pull the pull
> request I sent.
What about the patch in question? Since it was in repository before I have
modified my code and changed locking logic but then patch disappeared.
Not that I think the patch is very important but I do not work to modify code too
often...
Best regards
Andrei Emeltchenko
Hi Gustavo,
On Tue, May 08, 2012 at 04:44:51AM -0300, Gustavo Padovan wrote:
> > > > include/net/bluetooth/bluetooth.h | 2 --
> > > > net/bluetooth/l2cap_sock.c | 19 +++++++++++--------
> > > > 2 files changed, 11 insertions(+), 10 deletions(-)
> > >
> > > Patch has been applied to the bluetooth tree. Thanks.
> >
> > What about applying the first patch? I actually pulled that patch from
> > bluetooth-next and modified locking in my code but now that patch seems to
> > be lost.
>
> I applied it to bluetooth.git. Now you have to wait we merge bluetooth.git
> into bluetooth-next.git, that will happen shortly after John pull the pull
> request I sent.
Should this be other way around? First we apply to testing and then to
stable?
Best regards
Andrei Emeltchenko
Hi Andrei,
* Andrei Emeltchenko <[email protected]> [2012-05-08 10:42:31 +0300]:
> Hi Gustavo,
>
> On Fri, May 04, 2012 at 09:01:04PM -0300, Gustavo Padovan wrote:
> > Hi Mat,
> >
> > * Mat Martineau <[email protected]> [2012-05-04 14:20:31 -0700]:
> >
> > > The ERTM and streaming mode transmit queue must only be accessed while
> > > the L2CAP channel lock is held. Locking the channel before calling
> > > l2cap_chan_send ensures that multiple threads cannot simultaneously
> > > manipulate the queue when sending and receiving concurrently.
> > >
> > > L2CAP channel locking had previously moved to the l2cap_chan struct
> > > instead of the associated socket, so some of the old socket locking
> > > can also be removed in this patch.
> > >
> > > Signed-off-by: Mat Martineau <[email protected]>
> > > ---
> > > include/net/bluetooth/bluetooth.h | 2 --
> > > net/bluetooth/l2cap_sock.c | 19 +++++++++++--------
> > > 2 files changed, 11 insertions(+), 10 deletions(-)
> >
> > Patch has been applied to the bluetooth tree. Thanks.
>
> What about applying the first patch? I actually pulled that patch from
> bluetooth-next and modified locking in my code but now that patch seems to
> be lost.
I applied it to bluetooth.git. Now you have to wait we merge bluetooth.git
into bluetooth-next.git, that will happen shortly after John pull the pull
request I sent.
Gustavo
Hi Gustavo,
On Fri, May 04, 2012 at 09:01:04PM -0300, Gustavo Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2012-05-04 14:20:31 -0700]:
>
> > The ERTM and streaming mode transmit queue must only be accessed while
> > the L2CAP channel lock is held. Locking the channel before calling
> > l2cap_chan_send ensures that multiple threads cannot simultaneously
> > manipulate the queue when sending and receiving concurrently.
> >
> > L2CAP channel locking had previously moved to the l2cap_chan struct
> > instead of the associated socket, so some of the old socket locking
> > can also be removed in this patch.
> >
> > Signed-off-by: Mat Martineau <[email protected]>
> > ---
> > include/net/bluetooth/bluetooth.h | 2 --
> > net/bluetooth/l2cap_sock.c | 19 +++++++++++--------
> > 2 files changed, 11 insertions(+), 10 deletions(-)
>
> Patch has been applied to the bluetooth tree. Thanks.
What about applying the first patch? I actually pulled that patch from
bluetooth-next and modified locking in my code but now that patch seems to
be lost.
Best regards
Andrei Emeltchenko
Hi Mat,
* Mat Martineau <[email protected]> [2012-05-04 14:20:31 -0700]:
> The ERTM and streaming mode transmit queue must only be accessed while
> the L2CAP channel lock is held. Locking the channel before calling
> l2cap_chan_send ensures that multiple threads cannot simultaneously
> manipulate the queue when sending and receiving concurrently.
>
> L2CAP channel locking had previously moved to the l2cap_chan struct
> instead of the associated socket, so some of the old socket locking
> can also be removed in this patch.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/bluetooth.h | 2 --
> net/bluetooth/l2cap_sock.c | 19 +++++++++++--------
> 2 files changed, 11 insertions(+), 10 deletions(-)
Patch has been applied to the bluetooth tree. Thanks.
Gustavo
As the comment for l2cap_get_chan_by_scid indicated, the function used
to return a locked socket. The lock for the socket was acquired while
the channel list was also locked.
When locking was moved over to the l2cap_chan structure, the channel
lock was no longer acquired with the channel list still locked. This
made it possible for the l2cap_chan to be deleted after
conn->chan_lock was released but before l2cap_chan_lock was called.
Making the call to l2cap_chan_lock before releasing conn->chan_lock
makes it impossible for the l2cap_chan to be deleted at the wrong
time.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2f94067..11236ac 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -98,13 +98,15 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16
}
/* Find channel with given SCID.
- * Returns locked socket */
+ * Returns locked channel. */
static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
{
struct l2cap_chan *c;
mutex_lock(&conn->chan_lock);
c = __l2cap_get_chan_by_scid(conn, cid);
+ if (c)
+ l2cap_chan_lock(c);
mutex_unlock(&conn->chan_lock);
return c;
@@ -2860,8 +2862,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
if (!chan)
return -ENOENT;
- l2cap_chan_lock(chan);
-
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;
@@ -2971,8 +2971,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
if (!chan)
return 0;
- l2cap_chan_lock(chan);
-
switch (result) {
case L2CAP_CONF_SUCCESS:
l2cap_conf_rfc_get(chan, rsp->data, len);
@@ -4296,8 +4294,6 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
return 0;
}
- l2cap_chan_lock(chan);
-
BT_DBG("chan %p, len %d", chan, skb->len);
if (chan->state != BT_CONNECTED)
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
The ERTM and streaming mode transmit queue must only be accessed while
the L2CAP channel lock is held. Locking the channel before calling
l2cap_chan_send ensures that multiple threads cannot simultaneously
manipulate the queue when sending and receiving concurrently.
L2CAP channel locking had previously moved to the l2cap_chan struct
instead of the associated socket, so some of the old socket locking
can also be removed in this patch.
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/bluetooth.h | 2 --
net/bluetooth/l2cap_sock.c | 19 +++++++++++--------
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 262ebd1..b286f0a 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -242,12 +242,10 @@ static inline struct sk_buff *bt_skb_send_alloc(struct sock *sk,
{
struct sk_buff *skb;
- release_sock(sk);
if ((skb = sock_alloc_send_skb(sk, len + BT_SKB_RESERVE, nb, err))) {
skb_reserve(skb, BT_SKB_RESERVE);
bt_cb(skb)->incoming = 0;
}
- lock_sock(sk);
if (!skb && *err)
return NULL;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 29122ed..757ddb3 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -712,16 +712,13 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock, struct ms
if (msg->msg_flags & MSG_OOB)
return -EOPNOTSUPP;
- lock_sock(sk);
-
- if (sk->sk_state != BT_CONNECTED) {
- release_sock(sk);
+ if (sk->sk_state != BT_CONNECTED)
return -ENOTCONN;
- }
+ l2cap_chan_lock(chan);
err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
+ l2cap_chan_unlock(chan);
- release_sock(sk);
return err;
}
@@ -930,9 +927,15 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
unsigned long len, int nb,
int *err)
{
- struct sock *sk = chan->sk;
+ struct sk_buff *skb;
- return bt_skb_send_alloc(sk, len, nb, err);
+ l2cap_chan_unlock(chan);
+
+ skb = bt_skb_send_alloc(chan->sk, len, nb, err);
+
+ l2cap_chan_lock(chan);
+
+ return skb;
}
static struct l2cap_ops l2cap_chan_ops = {
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum