* [PATCH -v2 2/5] Bluetooth: Do not access chan->sk directly
2013-10-21 16:21 [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Gustavo Padovan
@ 2013-10-21 16:21 ` Gustavo Padovan
2013-10-21 16:21 ` [PATCH -v2 3/5] Bluetooth: Hold socket in defer callback in L2CAP socket Gustavo Padovan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Gustavo Padovan @ 2013-10-21 16:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
In the process of removing socket usage from L2CAP we now access the L2CAP
socket from the data member of struct l2cap_chan. For the L2CAP socket
user the data member points to the L2CAP socket.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
net/bluetooth/l2cap_sock.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1f326d9..a0b31db 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1159,11 +1159,12 @@ static void l2cap_sock_state_change_cb(struct l2cap_chan *chan, int state,
static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
unsigned long len, int nb)
{
+ struct sock *sk = chan->data;
struct sk_buff *skb;
int err;
l2cap_chan_unlock(chan);
- skb = bt_skb_send_alloc(chan->sk, len, nb, &err);
+ skb = bt_skb_send_alloc(sk, len, nb, &err);
l2cap_chan_lock(chan);
if (!skb)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH -v2 3/5] Bluetooth: Hold socket in defer callback in L2CAP socket
2013-10-21 16:21 [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Gustavo Padovan
2013-10-21 16:21 ` [PATCH -v2 2/5] Bluetooth: Do not access chan->sk directly Gustavo Padovan
@ 2013-10-21 16:21 ` Gustavo Padovan
2013-10-21 16:21 ` [PATCH -v2 4/5] Bluetooth: Remove socket lock from l2cap_state_change() Gustavo Padovan
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Gustavo Padovan @ 2013-10-21 16:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
In both places that we use the defer callback the socket lock is held for
a indirect sk access inside __l2cap_change_state() and chan->ops->defer(),
all the rest of the code between lock_sock() and release_sock() is
already protected by the channel lock and won't be affected by this
change.
We now use l2cap_change_state(), the locked version of the change state
function, and the defer callback does the locking itself now. This does
not affect other uses of the defer callback.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
net/bluetooth/l2cap_core.c | 15 +++------------
net/bluetooth/l2cap_sock.c | 8 ++++++--
2 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 4141545..049e1c8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1299,20 +1299,16 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
rsp.dcid = cpu_to_le16(chan->scid);
if (l2cap_chan_check_security(chan)) {
- struct sock *sk = chan->sk;
-
- lock_sock(sk);
if (test_bit(FLAG_DEFER_SETUP, &chan->flags)) {
rsp.result = __constant_cpu_to_le16(L2CAP_CR_PEND);
rsp.status = __constant_cpu_to_le16(L2CAP_CS_AUTHOR_PEND);
chan->ops->defer(chan);
} else {
- __l2cap_state_change(chan, BT_CONFIG);
+ l2cap_state_change(chan, BT_CONFIG);
rsp.result = __constant_cpu_to_le16(L2CAP_CR_SUCCESS);
rsp.status = __constant_cpu_to_le16(L2CAP_CS_NO_INFO);
}
- release_sock(sk);
} else {
rsp.result = __constant_cpu_to_le16(L2CAP_CR_PEND);
rsp.status = __constant_cpu_to_le16(L2CAP_CS_AUTHEN_PEND);
@@ -6643,31 +6639,26 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
__set_chan_timer(chan, 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 (test_bit(FLAG_DEFER_SETUP, &chan->flags)) {
res = L2CAP_CR_PEND;
stat = L2CAP_CS_AUTHOR_PEND;
chan->ops->defer(chan);
} 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, L2CAP_DISC_TIMEOUT);
res = L2CAP_CR_SEC_BLOCK;
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);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index a0b31db..e559992 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1195,11 +1195,15 @@ static void l2cap_sock_ready_cb(struct l2cap_chan *chan)
static void l2cap_sock_defer_cb(struct l2cap_chan *chan)
{
- struct sock *sk = chan->data;
- struct sock *parent = bt_sk(sk)->parent;
+ struct sock *parent, *sk = chan->data;
+
+ lock_sock(sk);
+ parent = bt_sk(sk)->parent;
if (parent)
parent->sk_data_ready(parent, 0);
+
+ release_sock(sk);
}
static void l2cap_sock_resume_cb(struct l2cap_chan *chan)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH -v2 4/5] Bluetooth: Remove socket lock from l2cap_state_change()
2013-10-21 16:21 [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Gustavo Padovan
2013-10-21 16:21 ` [PATCH -v2 2/5] Bluetooth: Do not access chan->sk directly Gustavo Padovan
2013-10-21 16:21 ` [PATCH -v2 3/5] Bluetooth: Hold socket in defer callback in L2CAP socket Gustavo Padovan
@ 2013-10-21 16:21 ` Gustavo Padovan
2013-10-21 16:21 ` [PATCH -v2 5/5] Bluetooth: Remove parent socket usage from l2cap_core.c Gustavo Padovan
2013-10-21 20:01 ` [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Marcel Holtmann
4 siblings, 0 replies; 6+ messages in thread
From: Gustavo Padovan @ 2013-10-21 16:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
This simplify and make safer the state change handling inside l2cap_core.c.
we got rid of __l2cap_state_change(). And l2cap_state_change() doesn't lock
the socket anymore, instead the socket is locked inside the ops callback for
state change in l2cap_sock.c.
It makes the code safer because in some we were using a unlocked version,
and now we are calls to l2cap_state_change(), when dealing with sockets, use
the locked version.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
net/bluetooth/l2cap_core.c | 31 +++++++------------------------
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 049e1c8..c65ddc8 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -223,7 +223,7 @@ static u16 l2cap_alloc_cid(struct l2cap_conn *conn)
return 0;
}
-static void __l2cap_state_change(struct l2cap_chan *chan, int state)
+static void l2cap_state_change(struct l2cap_chan *chan, int state)
{
BT_DBG("chan %p %s -> %s", chan, state_to_string(chan->state),
state_to_string(state));
@@ -232,33 +232,16 @@ static void __l2cap_state_change(struct l2cap_chan *chan, int state)
chan->ops->state_change(chan, state, 0);
}
-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 inline void l2cap_state_change_and_error(struct l2cap_chan *chan,
int state, int err)
{
- struct sock *sk = chan->sk;
-
- lock_sock(sk);
chan->state = state;
chan->ops->state_change(chan, chan->state, err);
- release_sock(sk);
}
static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
{
- struct sock *sk = chan->sk;
-
- lock_sock(sk);
chan->ops->state_change(chan, chan->state, err);
- release_sock(sk);
}
static void __set_retrans_timer(struct l2cap_chan *chan)
@@ -3787,7 +3770,7 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE) {
if (l2cap_chan_check_security(chan)) {
if (test_bit(FLAG_DEFER_SETUP, &chan->flags)) {
- __l2cap_state_change(chan, BT_CONNECT2);
+ l2cap_state_change(chan, BT_CONNECT2);
result = L2CAP_CR_PEND;
status = L2CAP_CS_AUTHOR_PEND;
chan->ops->defer(chan);
@@ -3797,21 +3780,21 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
* physical link is up.
*/
if (amp_id == AMP_ID_BREDR) {
- __l2cap_state_change(chan, BT_CONFIG);
+ l2cap_state_change(chan, BT_CONFIG);
result = L2CAP_CR_SUCCESS;
} else {
- __l2cap_state_change(chan, BT_CONNECT2);
+ l2cap_state_change(chan, BT_CONNECT2);
result = L2CAP_CR_PEND;
}
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;
}
@@ -4738,7 +4721,7 @@ static void l2cap_do_create(struct l2cap_chan *chan, int result,
sizeof(rsp), &rsp);
if (result == L2CAP_CR_SUCCESS) {
- __l2cap_state_change(chan, BT_CONFIG);
+ l2cap_state_change(chan, BT_CONFIG);
set_bit(CONF_REQ_SENT, &chan->conf_state);
l2cap_send_cmd(chan->conn, l2cap_get_ident(chan->conn),
L2CAP_CONF_REQ,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH -v2 5/5] Bluetooth: Remove parent socket usage from l2cap_core.c
2013-10-21 16:21 [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Gustavo Padovan
` (2 preceding siblings ...)
2013-10-21 16:21 ` [PATCH -v2 4/5] Bluetooth: Remove socket lock from l2cap_state_change() Gustavo Padovan
@ 2013-10-21 16:21 ` Gustavo Padovan
2013-10-21 20:01 ` [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Marcel Holtmann
4 siblings, 0 replies; 6+ messages in thread
From: Gustavo Padovan @ 2013-10-21 16:21 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Gustavo Padovan
From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
The parent socket is not used inside the L2CAP core anymore. We only lock
it to indirect access through the new_connection() callback. The hold of
the socket lock was moved to the new_connection() callback.
Inside L2CAP core the channel lock is now used in l2cap_le_conn_ready()
and l2cap_conn_ready() to protect the execution of these two functions
during the handling of new incoming connections.
This change remove the socket lock usage from L2CAP core while keeping
the code safe against race conditions.
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
net/bluetooth/l2cap_core.c | 14 ++++----------
net/bluetooth/l2cap_sock.c | 4 ++++
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c65ddc8..bb6d35e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1362,7 +1362,6 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
static void l2cap_le_conn_ready(struct l2cap_conn *conn)
{
struct hci_conn *hcon = conn->hcon;
- struct sock *parent;
struct l2cap_chan *chan, *pchan;
u8 dst_type;
@@ -1384,9 +1383,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
if (hci_blacklist_lookup(hcon->hdev, &hcon->dst, dst_type))
return;
- parent = pchan->sk;
-
- lock_sock(parent);
+ l2cap_chan_lock(pchan);
chan = pchan->ops->new_connection(pchan);
if (!chan)
@@ -1402,7 +1399,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
__l2cap_chan_add(conn, chan);
clean:
- release_sock(parent);
+ l2cap_chan_unlock(pchan);
}
static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -3705,7 +3702,6 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
struct l2cap_conn_rsp rsp;
struct l2cap_chan *chan = NULL, *pchan;
- struct sock *parent;
int result, status = L2CAP_CS_NO_INFO;
u16 dcid = 0, scid = __le16_to_cpu(req->scid);
@@ -3721,10 +3717,8 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
goto sendresp;
}
- parent = pchan->sk;
-
mutex_lock(&conn->chan_lock);
- lock_sock(parent);
+ l2cap_chan_lock(pchan);
/* Check if the ACL is secure enough (if not SDP) */
if (psm != __constant_cpu_to_le16(L2CAP_PSM_SDP) &&
@@ -3800,7 +3794,7 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
}
response:
- release_sock(parent);
+ l2cap_chan_unlock(pchan);
mutex_unlock(&conn->chan_lock);
sendresp:
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index e559992..a159b0e 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1042,6 +1042,8 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
{
struct sock *sk, *parent = chan->data;
+ lock_sock(parent);
+
/* Check for backlog size */
if (sk_acceptq_is_full(parent)) {
BT_DBG("backlog full %d", parent->sk_ack_backlog);
@@ -1059,6 +1061,8 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(struct l2cap_chan *chan)
bt_accept_enqueue(parent, sk);
+ release_sock(parent);
+
return l2cap_pi(sk)->chan;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -v2 1/5] Bluetooth: Remove not used struct sock
2013-10-21 16:21 [PATCH -v2 1/5] Bluetooth: Remove not used struct sock Gustavo Padovan
` (3 preceding siblings ...)
2013-10-21 16:21 ` [PATCH -v2 5/5] Bluetooth: Remove parent socket usage from l2cap_core.c Gustavo Padovan
@ 2013-10-21 20:01 ` Marcel Holtmann
4 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2013-10-21 20:01 UTC (permalink / raw)
To: Gustavo Padovan; +Cc: linux-bluetooth, Gustavo Padovan
Hi Gustavo,
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> It is a leftover from the recent effort of remove sk usage from L2CAP
> core.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> net/bluetooth/l2cap_core.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
all 5 patches have been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread