[PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Gustavo F. Padovan]

It also have to change the name of the function to
l2cap_get_sock_by_addr() because we do hold the lock inside it now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 6f931cc..3d48867 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
 }
 
 /* ---- Socket interface ---- */
-static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
+static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
 {
 	struct sock *sk;
 	struct hlist_node *node;
+
+	write_lock_bh(&l2cap_sk_list.lock);
 	sk_for_each(sk, node, &l2cap_sk_list.head)
 		if (l2cap_pi(sk)->sport == psm && !bacmp(&bt_sk(sk)->src, src))
 			goto found;
 	sk = NULL;
 found:
+	write_unlock_bh(&l2cap_sk_list.lock);
 	return sk;
 }
 
@@ -1024,9 +1027,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 		}
 	}
 
-	write_lock_bh(&l2cap_sk_list.lock);
-
-	if (la.l2_psm && __l2cap_get_sock_by_addr(la.l2_psm, &la.l2_bdaddr)) {
+	if (la.l2_psm && l2cap_get_sock_by_addr(la.l2_psm, &la.l2_bdaddr)) {
 		err = -EADDRINUSE;
 	} else {
 		/* Save source address */
@@ -1040,8 +1041,6 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
 			l2cap_pi(sk)->sec_level = BT_SECURITY_SDP;
 	}
 
-	write_unlock_bh(&l2cap_sk_list.lock);
-
 done:
 	release_sock(sk);
 	return err;
@@ -1257,18 +1256,14 @@ static int l2cap_sock_listen(struct socket *sock, int backlog)
 
 		err = -EINVAL;
 
-		write_lock_bh(&l2cap_sk_list.lock);
-
 		for (psm = 0x1001; psm < 0x1100; psm += 2)
-			if (!__l2cap_get_sock_by_addr(cpu_to_le16(psm), src)) {
+			if (!l2cap_get_sock_by_addr(cpu_to_le16(psm), src)) {
 				l2cap_pi(sk)->psm   = cpu_to_le16(psm);
 				l2cap_pi(sk)->sport = cpu_to_le16(psm);
 				err = 0;
 				break;
 			}
 
-		write_unlock_bh(&l2cap_sk_list.lock);
-
 		if (err < 0)
 			goto done;
 	}
-- 
1.7.3.1

[PATCH 2/7] Bluetooth: Hold the lock inside sco_get_sock_by_addr()

[Gustavo F. Padovan]

It also have to change the name of the function to
sco_get_sock_by_addr() because we do hold the lock inside it now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/sco.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index d0927d1..85b5498 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -276,16 +276,18 @@ drop:
 }
 
 /* -------- Socket interface ---------- */
-static struct sock *__sco_get_sock_by_addr(bdaddr_t *ba)
+static struct sock *sco_get_sock_by_addr(bdaddr_t *ba)
 {
 	struct sock *sk;
 	struct hlist_node *node;
 
+	write_lock_bh(&sco_sk_list.lock);
 	sk_for_each(sk, node, &sco_sk_list.head)
 		if (!bacmp(&bt_sk(sk)->src, ba))
 			goto found;
 	sk = NULL;
 found:
+	write_unlock_bh(&sco_sk_list.lock);
 	return sk;
 }
 
@@ -469,9 +471,7 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		goto done;
 	}
 
-	write_lock_bh(&sco_sk_list.lock);
-
-	if (bacmp(src, BDADDR_ANY) && __sco_get_sock_by_addr(src)) {
+	if (bacmp(src, BDADDR_ANY) && sco_get_sock_by_addr(src)) {
 		err = -EADDRINUSE;
 	} else {
 		/* Save source address */
@@ -479,8 +479,6 @@ static int sco_sock_bind(struct socket *sock, struct sockaddr *addr, int addr_le
 		sk->sk_state = BT_BOUND;
 	}
 
-	write_unlock_bh(&sco_sk_list.lock);
-
 done:
 	release_sock(sk);
 	return err;
-- 
1.7.3.1

[PATCH 3/7] Bluetooth: Hold the lock inside rfcomm_get_sock_by_addr()

[Gustavo F. Padovan]

It also have to change the name of the function to
rfcomm_get_sock_by_addr() because we do hold the lock inside it now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/rfcomm/sock.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index aec505f..4ed9499 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -123,16 +123,18 @@ static void rfcomm_sk_state_change(struct rfcomm_dlc *d, int err)
 }
 
 /* ---- Socket functions ---- */
-static struct sock *__rfcomm_get_sock_by_addr(u8 channel, bdaddr_t *src)
+static struct sock *rfcomm_get_sock_by_addr(u8 channel, bdaddr_t *src)
 {
 	struct sock *sk = NULL;
 	struct hlist_node *node;
 
+	write_lock_bh(&rfcomm_sk_list.lock);
 	sk_for_each(sk, node, &rfcomm_sk_list.head) {
 		if (rfcomm_pi(sk)->channel == channel &&
 				!bacmp(&bt_sk(sk)->src, src))
 			break;
 	}
+	write_unlock_bh(&rfcomm_sk_list.lock);
 
 	return node ? sk : NULL;
 }
@@ -374,9 +376,7 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
 		goto done;
 	}
 
-	write_lock_bh(&rfcomm_sk_list.lock);
-
-	if (sa->rc_channel && __rfcomm_get_sock_by_addr(sa->rc_channel, &sa->rc_bdaddr)) {
+	if (sa->rc_channel && rfcomm_get_sock_by_addr(sa->rc_channel, &sa->rc_bdaddr)) {
 		err = -EADDRINUSE;
 	} else {
 		/* Save source address */
@@ -385,8 +385,6 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
 		sk->sk_state = BT_BOUND;
 	}
 
-	write_unlock_bh(&rfcomm_sk_list.lock);
-
 done:
 	release_sock(sk);
 	return err;
@@ -459,17 +457,13 @@ static int rfcomm_sock_listen(struct socket *sock, int backlog)
 
 		err = -EINVAL;
 
-		write_lock_bh(&rfcomm_sk_list.lock);
-
 		for (channel = 1; channel < 31; channel++)
-			if (!__rfcomm_get_sock_by_addr(channel, src)) {
+			if (!rfcomm_get_sock_by_addr(channel, src)) {
 				rfcomm_pi(sk)->channel = channel;
 				err = 0;
 				break;
 			}
 
-		write_unlock_bh(&rfcomm_sk_list.lock);
-
 		if (err < 0)
 			goto done;
 	}
-- 
1.7.3.1

[PATCH 4/7] Bluetooth: Get ride of __l2cap_get_sock_by_psm()

[Gustavo F. Padovan]

l2cap_get_sock_by_psm() was the only user of this function, so I merged
both into l2cap_get_sock_by_psm(). The socket lock now should be hold
outside of l2cap_get_sock_by_psm() once we hold and release it inside the
same function now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 3d48867..27199bc 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -746,11 +746,13 @@ found:
 /* Find socket with psm and source bdaddr.
  * Returns closest match.
  */
-static struct sock *__l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src)
+static struct sock *l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src)
 {
 	struct sock *sk = NULL, *sk1 = NULL;
 	struct hlist_node *node;
 
+	read_lock(&l2cap_sk_list.lock);
+
 	sk_for_each(sk, node, &l2cap_sk_list.head) {
 		if (state && sk->sk_state != state)
 			continue;
@@ -765,20 +767,10 @@ static struct sock *__l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src
 				sk1 = sk;
 		}
 	}
-	return node ? sk : sk1;
-}
 
-/* Find socket with given address (psm, src).
- * Returns locked socket */
-static inline struct sock *l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src)
-{
-	struct sock *s;
-	read_lock(&l2cap_sk_list.lock);
-	s = __l2cap_get_sock_by_psm(state, psm, src);
-	if (s)
-		bh_lock_sock(s);
 	read_unlock(&l2cap_sk_list.lock);
-	return s;
+
+	return node ? sk : sk1;
 }
 
 static void l2cap_sock_destruct(struct sock *sk)
@@ -2921,6 +2913,8 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		goto sendresp;
 	}
 
+	bh_lock_sock(parent);
+
 	/* Check if the ACL is secure enough (if not SDP) */
 	if (psm != cpu_to_le16(0x0001) &&
 				!hci_conn_check_link_mode(conn->hcon)) {
@@ -4425,6 +4419,8 @@ static inline int l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm, str
 	if (!sk)
 		goto drop;
 
+	bh_lock_sock(sk);
+
 	BT_DBG("sk %p, len %d", sk, skb->len);
 
 	if (sk->sk_state != BT_BOUND && sk->sk_state != BT_CONNECTED)
-- 
1.7.3.1

[PATCH 5/7] Bluetooth: Get ride of __rfcomm_get_sock_by_channel()

[Gustavo F. Padovan]

rfcomm_get_sock_by_channel() was the only user of this function, so I merged
both into rfcomm_get_sock_by_channel(). The socket lock now should be hold
outside of rfcomm_get_sock_by_channel() once we hold and release it inside the
same function now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/rfcomm/sock.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 4ed9499..e48fbca 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -142,11 +142,13 @@ static struct sock *rfcomm_get_sock_by_addr(u8 channel, bdaddr_t *src)
 /* Find socket with channel and source bdaddr.
  * Returns closest match.
  */
-static struct sock *__rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
+static struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
 {
 	struct sock *sk = NULL, *sk1 = NULL;
 	struct hlist_node *node;
 
+	read_lock(&rfcomm_sk_list.lock);
+
 	sk_for_each(sk, node, &rfcomm_sk_list.head) {
 		if (state && sk->sk_state != state)
 			continue;
@@ -161,19 +163,10 @@ static struct sock *__rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t
 				sk1 = sk;
 		}
 	}
-	return node ? sk : sk1;
-}
 
-/* Find socket with given address (channel, src).
- * Returns locked socket */
-static inline struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
-{
-	struct sock *s;
-	read_lock(&rfcomm_sk_list.lock);
-	s = __rfcomm_get_sock_by_channel(state, channel, src);
-	if (s) bh_lock_sock(s);
 	read_unlock(&rfcomm_sk_list.lock);
-	return s;
+
+	return node ? sk : sk1;
 }
 
 static void rfcomm_sock_destruct(struct sock *sk)
@@ -939,6 +932,8 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 	if (!parent)
 		return 0;
 
+	bh_lock_sock(parent);
+
 	/* Check for backlog size */
 	if (sk_acceptq_is_full(parent)) {
 		BT_DBG("backlog full %d", parent->sk_ack_backlog);
-- 
1.7.3.1

[PATCH 6/7] Bluetooth: Fix not returning proper error in SCO

[Gustavo F. Padovan]

Return 0 in that situation could lead to errors in the caller.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/sco.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 85b5498..f031b62 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -880,7 +880,7 @@ static int sco_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr, __u8 type)
 	int lm = 0;
 
 	if (type != SCO_LINK && type != ESCO_LINK)
-		return 0;
+		return -EINVAL;
 
 	BT_DBG("hdev %s, bdaddr %s", hdev->name, batostr(bdaddr));
 
@@ -906,7 +906,7 @@ static int sco_connect_cfm(struct hci_conn *hcon, __u8 status)
 	BT_DBG("hcon %p bdaddr %s status %d", hcon, batostr(&hcon->dst), status);
 
 	if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK)
-		return 0;
+		return -EINVAL;
 
 	if (!status) {
 		struct sco_conn *conn;
@@ -925,7 +925,7 @@ static int sco_disconn_cfm(struct hci_conn *hcon, __u8 reason)
 	BT_DBG("hcon %p reason %d", hcon, reason);
 
 	if (hcon->type != SCO_LINK && hcon->type != ESCO_LINK)
-		return 0;
+		return -EINVAL;
 
 	sco_conn_del(hcon, bt_err(reason));
 
-- 
1.7.3.1

[PATCH 7/7] Bluetooth: Fix not returning proper error in RFCOMM

[Gustavo F. Padovan]

Return 0 in that situation could lead to errors in the caller.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/rfcomm/sock.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index e48fbca..cd7e27a 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -930,7 +930,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 	/* Check if we have socket listening on channel */
 	parent = rfcomm_get_sock_by_channel(BT_LISTEN, channel, &src);
 	if (!parent)
-		return 0;
+		return -EINVAL;
 
 	bh_lock_sock(parent);
 
-- 
1.7.3.1

Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Ville Tervo]

Hi Gustavo,

On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> It also have to change the name of the function to
> l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> 
> Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> ---
>  net/bluetooth/l2cap.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 6f931cc..3d48867 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
>  }
>  
>  /* ---- Socket interface ---- */
> -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
>  {
>  	struct sock *sk;
>  	struct hlist_node *node;
> +
> +	write_lock_bh(&l2cap_sk_list.lock);

Code is only reading so read_lock_bh would be enough?

>
>  	sk_for_each(sk, node, &l2cap_sk_list.head)
>  		if (l2cap_pi(sk)->sport == psm && !bacmp(&bt_sk(sk)->src, src))
>  			goto found;
>  	sk = NULL;
>  found:
> +	write_unlock_bh(&l2cap_sk_list.lock);
>  	return sk;
>  }
>  
> @@ -1024,9 +1027,7 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
>  		}
>  	}
>  
> -	write_lock_bh(&l2cap_sk_list.lock);
> -
> -	if (la.l2_psm && __l2cap_get_sock_by_addr(la.l2_psm, &la.l2_bdaddr)) {
> +	if (la.l2_psm && l2cap_get_sock_by_addr(la.l2_psm, &la.l2_bdaddr)) {
>  		err = -EADDRINUSE;
>  	} else {
>  		/* Save source address */
> @@ -1040,8 +1041,6 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
>  			l2cap_pi(sk)->sec_level = BT_SECURITY_SDP;
>  	}
>  
> -	write_unlock_bh(&l2cap_sk_list.lock);
> -
>  done:
>  	release_sock(sk);
>  	return err;
> @@ -1257,18 +1256,14 @@ static int l2cap_sock_listen(struct socket *sock, int backlog)
>  
>  		err = -EINVAL;
>  
> -		write_lock_bh(&l2cap_sk_list.lock);
> -
>  		for (psm = 0x1001; psm < 0x1100; psm += 2)
> -			if (!__l2cap_get_sock_by_addr(cpu_to_le16(psm), src)) {
> +			if (!l2cap_get_sock_by_addr(cpu_to_le16(psm), src)) {
>  				l2cap_pi(sk)->psm   = cpu_to_le16(psm);
>  				l2cap_pi(sk)->sport = cpu_to_le16(psm);
>  				err = 0;
>  				break;
>  			}
>  
> -		write_unlock_bh(&l2cap_sk_list.lock);
> -
>  		if (err < 0)
>  			goto done;
>  	}
> -- 
> 1.7.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] Bluetooth: Fix not returning proper error in RFCOMM

[Ville Tervo]

Hi Gustavo,

On Tue, Nov 02, 2010 at 04:03:18PM +0100, ext Gustavo F. Padovan wrote:
> Return 0 in that situation could lead to errors in the caller.
> 
> Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> ---
>  net/bluetooth/rfcomm/sock.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index e48fbca..cd7e27a 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -930,7 +930,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
>  	/* Check if we have socket listening on channel */
>  	parent = rfcomm_get_sock_by_channel(BT_LISTEN, channel, &src);
>  	if (!parent)
> -		return 0;
> +		return -EINVAL;

Did you check rfcomm/core.c?

I think this change breaks the logic in there. IOW continues connection
establisment instead of tearing down the link.

-- 
Ville

Re: [PATCH 7/7] Bluetooth: Fix not returning proper error in RFCOMM

[Gustavo F. Padovan]

Hi Ville,

* Ville Tervo <ville.tervo@nokia.com> [2010-11-05 16:01:58 +0200]:

> Hi Gustavo,
> 
> On Tue, Nov 02, 2010 at 04:03:18PM +0100, ext Gustavo F. Padovan wrote:
> > Return 0 in that situation could lead to errors in the caller.
> > 
> > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > ---
> >  net/bluetooth/rfcomm/sock.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> > index e48fbca..cd7e27a 100644
> > --- a/net/bluetooth/rfcomm/sock.c
> > +++ b/net/bluetooth/rfcomm/sock.c
> > @@ -930,7 +930,7 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
> >  	/* Check if we have socket listening on channel */
> >  	parent = rfcomm_get_sock_by_channel(BT_LISTEN, channel, &src);
> >  	if (!parent)
> > -		return 0;
> > +		return -EINVAL;
> 
> Did you check rfcomm/core.c?
> 
> I think this change breaks the logic in there. IOW continues connection
> establisment instead of tearing down the link.

I understood that part of the code wrong, I'm going to drop this patch.
Thanks fo looking into this.

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Gustavo F. Padovan]

Hi Ville,

* Ville Tervo <ville.tervo@nokia.com> [2010-11-05 15:49:35 +0200]:

> Hi Gustavo,
> 
> On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > It also have to change the name of the function to
> > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > 
> > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > ---
> >  net/bluetooth/l2cap.c |   17 ++++++-----------
> >  1 files changed, 6 insertions(+), 11 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > index 6f931cc..3d48867 100644
> > --- a/net/bluetooth/l2cap.c
> > +++ b/net/bluetooth/l2cap.c
> > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> >  }
> >  
> >  /* ---- Socket interface ---- */
> > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> >  {
> >  	struct sock *sk;
> >  	struct hlist_node *node;
> > +
> > +	write_lock_bh(&l2cap_sk_list.lock);
> 
> Code is only reading so read_lock_bh would be enough?

Sure, I didn't looked to that, I just keept the same code that we were
using before. I'll fix it.

-- 
Gustavo F. Padovan
ProFUSION embedded systems - http://profusion.mobi

Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Marcel Holtmann]

Hi Gustavo,

> > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > It also have to change the name of the function to
> > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > > 
> > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > ---
> > >  net/bluetooth/l2cap.c |   17 ++++++-----------
> > >  1 files changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > index 6f931cc..3d48867 100644
> > > --- a/net/bluetooth/l2cap.c
> > > +++ b/net/bluetooth/l2cap.c
> > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > >  }
> > >  
> > >  /* ---- Socket interface ---- */
> > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > >  {
> > >  	struct sock *sk;
> > >  	struct hlist_node *node;
> > > +
> > > +	write_lock_bh(&l2cap_sk_list.lock);
> > 
> > Code is only reading so read_lock_bh would be enough?
> 
> Sure, I didn't looked to that, I just keept the same code that we were
> using before. I'll fix it.

we might also not just bother with read/write locks. Since they are not
always the right thing to do. In a lot of cases a pure spinlock is just
better. And in case of Bluetooth I think we would be just fine with
using a pure spinlock. You might run some tests with this.

Regards

Marcel

Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Gustavo F. Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2010-11-10 14:39:55 +0900]:

> Hi Gustavo,
> 
> > > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > > It also have to change the name of the function to
> > > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > > > 
> > > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > > ---
> > > >  net/bluetooth/l2cap.c |   17 ++++++-----------
> > > >  1 files changed, 6 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > > index 6f931cc..3d48867 100644
> > > > --- a/net/bluetooth/l2cap.c
> > > > +++ b/net/bluetooth/l2cap.c
> > > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > > >  }
> > > >  
> > > >  /* ---- Socket interface ---- */
> > > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > >  {
> > > >  	struct sock *sk;
> > > >  	struct hlist_node *node;
> > > > +
> > > > +	write_lock_bh(&l2cap_sk_list.lock);
> > > 
> > > Code is only reading so read_lock_bh would be enough?
> > 
> > Sure, I didn't looked to that, I just keept the same code that we were
> > using before. I'll fix it.
> 
> we might also not just bother with read/write locks. Since they are not
> always the right thing to do. In a lot of cases a pure spinlock is just
> better. And in case of Bluetooth I think we would be just fine with
> using a pure spinlock. You might run some tests with this.


Better than spinlock is move everything to RCU, than we will just need a
mutex or spinlock in the write operations. Using a RCU is a lot simpler.

These patches are a clean up to add RCU to sk_list, so I propose to
upstream the new version of my patches and do the RCU change on top
of them.

After all these changes we will be ready for the l2cap.c split.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Gustavo F. Padovan]

* Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-05 10:37:11 -0400]:

> Hi Ville,
> 
> * Ville Tervo <ville.tervo@nokia.com> [2010-11-05 15:49:35 +0200]:
> 
> > Hi Gustavo,
> > 
> > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > It also have to change the name of the function to
> > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > > 
> > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > ---
> > >  net/bluetooth/l2cap.c |   17 ++++++-----------
> > >  1 files changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > index 6f931cc..3d48867 100644
> > > --- a/net/bluetooth/l2cap.c
> > > +++ b/net/bluetooth/l2cap.c
> > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > >  }
> > >  
> > >  /* ---- Socket interface ---- */
> > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > >  {
> > >  	struct sock *sk;
> > >  	struct hlist_node *node;
> > > +
> > > +	write_lock_bh(&l2cap_sk_list.lock);
> > 
> > Code is only reading so read_lock_bh would be enough?
> 
> Sure, I didn't looked to that, I just keept the same code that we were
> using before. I'll fix it.

I figured out that we need write_lock_bh() here, because set the psm and
sport is like a new element to the list. l2cap_get_sock_by_addr()
searches for either psm or sport. 

I'm also dropping the option to use RCU on the bt_sk_list(), It does not
fit on our case. We can't have anyone writing the list while we are
reading it.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()

[Gustavo F. Padovan]

* Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-17 21:11:41 -0200]:

> * Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-05 10:37:11 -0400]:
> 
> > Hi Ville,
> > 
> > * Ville Tervo <ville.tervo@nokia.com> [2010-11-05 15:49:35 +0200]:
> > 
> > > Hi Gustavo,
> > > 
> > > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > > It also have to change the name of the function to
> > > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > > > 
> > > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > > ---
> > > >  net/bluetooth/l2cap.c |   17 ++++++-----------
> > > >  1 files changed, 6 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > > index 6f931cc..3d48867 100644
> > > > --- a/net/bluetooth/l2cap.c
> > > > +++ b/net/bluetooth/l2cap.c
> > > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > > >  }
> > > >  
> > > >  /* ---- Socket interface ---- */
> > > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > >  {
> > > >  	struct sock *sk;
> > > >  	struct hlist_node *node;
> > > > +
> > > > +	write_lock_bh(&l2cap_sk_list.lock);
> > > 
> > > Code is only reading so read_lock_bh would be enough?
> > 
> > Sure, I didn't looked to that, I just keept the same code that we were
> > using before. I'll fix it.
> 
> I figured out that we need write_lock_bh() here, because set the psm and
> sport is like a new element to the list. l2cap_get_sock_by_addr()
> searches for either psm or sport. 
> 
> I'm also dropping the option to use RCU on the bt_sk_list(), It does not
> fit on our case. We can't have anyone writing the list while we are
> reading it.

That said, only patch 4 and 5 are still valid (I'll resend them), and 6 is
so trivial that I put it upstream already.  

-- 
Gustavo F. Padovan
http://profusion.mobi