[PATCHv2 0/2] Reworked send locking patches

[PATCHv2 0/2] Reworked send locking patches

[Mat Martineau]

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

[PATCHv2 1/2] Bluetooth: Restore locking semantics when looking up L2CAP channels

[Mat Martineau]

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 <mathewm@codeaurora.org>
---
 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

[PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[Mat Martineau]

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 <mathewm@codeaurora.org>
---
 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

Re: [PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[Gustavo Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [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 <mathewm@codeaurora.org>
> ---
>  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

Re: [PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[Andrei Emeltchenko]

Hi Gustavo,

On Fri, May 04, 2012 at 09:01:04PM -0300, Gustavo Padovan wrote:
> Hi Mat,
> 
> * Mat Martineau <mathewm@codeaurora.org> [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 <mathewm@codeaurora.org>
> > ---
> >  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 

Re: [PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [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 <mathewm@codeaurora.org> [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 <mathewm@codeaurora.org>
> > > ---
> > >  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

Re: [PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[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 

Re: [PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[Andrei Emeltchenko]

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 

Re: [PATCHv2 2/2] Bluetooth: Lock the L2CAP channel when sending

[Gustavo Padovan]

[-- Attachment #1: Type: text/plain, Size: 1034 bytes --]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]