Note: Please do not merge **any** of these RFC patches to
bluetooth-next yet. The patches that were merged from the RFCv1 set
were ok. There are several changes to locking code in this series that
need thorough testing and there are some non-obvious dependencies
between certain patches that require them to be merged in order. I
will repost with [PATCH] headers after further review and testing.
This is the second patch series reworking the ERTM state machine and
closely related streaming mode code. These patches include bug fixes
and segmentation of outgoing L2CAP data.
RFCv1: Four of eight patches were merged already.
RFCv2: Fixed the send lock patch, found and fixed a few more issues
with locking, reference counting, and unused code.
Mat Martineau (8):
Bluetooth: Initialize new l2cap_chan structure members
Bluetooth: Remove unused function
Bluetooth: Make better use of l2cap_chan reference counting
Bluetooth: Fix a redundant and problematic incoming MTU check
Bluetooth: Restore locking semantics when looking up L2CAP channels
Bluetooth: Lock the L2CAP channel when sending
Bluetooth: Refactor L2CAP ERTM and streaming transmit segmentation
Bluetooth: Add Code Aurora Forum copyright
include/net/bluetooth/bluetooth.h | 2 -
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/l2cap_core.c | 227 +++++++++++++++++++------------------
net/bluetooth/l2cap_sock.c | 18 +--
4 files changed, 130 insertions(+), 118 deletions(-)
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
On Mon, Apr 30, 2012 at 6:04 PM, Mat Martineau <[email protected]> wro=
te:
>
> Gustavo -
>
>
> On Fri, 27 Apr 2012, Gustavo Padovan wrote:
>
>> Hi Mat,
>>
>> * Mat Martineau <[email protected]> [2012-04-27 16:50:51 -0700]:
>>
>>> The L2CAP MTU for incoming data is verified differently depending on
>>> the L2CAP mode, so the check is best performed in a mode-specific
>>> context. =A0Checking the incoming MTU before HCI fragment reassembly is
>>> a layer violation and assumes all bytes after the standard L2CAP
>>> header are L2CAP data.
>>>
>>> This approadch causes issues with unsegmented ERTM or streaming mode
>
>
> Oops, I need to fix this "approadch" typo.
>
>
>>> frames, where there are additional enhanced or extended headers before
>>> the data payload and possible FCS bytes after the data payload. =A0A
>>> valid frame could be as many as 10 bytes larger than the MTU.
>>>
>>> Removing this code is the best fix, because the MTU is checked later
>>> on for all L2CAP data frames (connectionless, basic, ERTM, and
>>> streaming). =A0This also gets rid of outdated locking (socket instead o=
f
>>> l2cap_chan) and an extra lookup of the channel ID.
>>
>>
>> I don't think we can remove this code from here. This check is different
>> from the ones you are talking, those are l2cap packets, here we are stil=
l
>> dealing with ACL fragments. This check is there to avoid accept a first
>> ACL packet that is too big. See 8979481328d.
>>
>> One possible solution is to add a slack value to the check, so we can
>> fit those 10+ bytes packets in it.
>
>
> If we just add slack, then we're depending on the real MTU checks to work
> correctly later. =A0Why bother with this early check at all?
>
> I think there's a misunderstanding about the code that is being removed. =
=A0It
> *is* checking the L2CAP MTU for that channel against the value in the L2C=
AP
> length header before the whole frame is assembled, which is why it should=
n't
> be involved with ACL fragments to begin with. =A0It is the only place in
> l2cap_recv_acldata that uses channel-specific information.
>
> This code tries to throw out the first ACL fragment if the L2CAP payload =
is
> longer than than the L2CAP MTU for that channel. =A0The ERTM and streamin=
g
> mode MTU has different meaning - it applies to the reassembled SDU payloa=
d,
> not the size of an individual PDU segment with the extra headers and FCS
> bytes. =A0There is already a length check in the fragment reassembly code=
that
> makes sure no reassembled ACL frame exceeds 65535 bytes (look at how
> conn->rx_len is used).
>
> The original discussion around this code is here:
>
> http://thread.gmane.org/gmane.linux.bluez.kernel/7505
>
> The purpose was to address a memory allocation failure in __get_free_page=
s -
> which suggests heap corruption. =A0Even if the start fragment is too larg=
e,
> that shouldn't lead to heap corruption, especially if the fragment is
> properly freed. =A0Throwing out this large packet is just hiding the real=
bug!
>
> The MTU is checked everywhere that it needs to be for L2CAP data, after t=
he
> ACL fragments are reassembled:
>
> * In l2cap_reassemble_sdu for ERTM and Streaming Mode
> * In l2cap_data_channel for Basic Mode
> * In l2cap_connless_channel for connectionless channels
>
> The code being removed doesn't address the signaling channel, because tha=
t
> channel is not in the channel list. The spec defines the MTU for the
> signaling channel to be "MTUsig", and only requires it to be at least 48
> bytes (672 bytes if extended flowspec is supported). =A0It is valid for u=
s not
> to enforce a limit on it other than the maximum L2CAP frame size.
>
>
> We removed this code (because it broke AMP) from our Android kernel back =
in
> September 2011, and have been through several qualifications and rounds o=
f
> testing since then without problems.
Mat, I agree with you here and the patch looks good IMO.
Gustavo, maybe you can have a second look at this patch? The check
indeed looks to be in the wrong layer and it'd be good to remove sk
lock usage and the extra channel lookup for every ACL initial
fragment.
Best regards,
--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs
Gustavo -
On Fri, 27 Apr 2012, Gustavo Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2012-04-27 16:50:51 -0700]:
>
>> The L2CAP MTU for incoming data is verified differently depending on
>> the L2CAP mode, so the check is best performed in a mode-specific
>> context. Checking the incoming MTU before HCI fragment reassembly is
>> a layer violation and assumes all bytes after the standard L2CAP
>> header are L2CAP data.
>>
>> This approadch causes issues with unsegmented ERTM or streaming mode
Oops, I need to fix this "approadch" typo.
>> frames, where there are additional enhanced or extended headers before
>> the data payload and possible FCS bytes after the data payload. A
>> valid frame could be as many as 10 bytes larger than the MTU.
>>
>> Removing this code is the best fix, because the MTU is checked later
>> on for all L2CAP data frames (connectionless, basic, ERTM, and
>> streaming). This also gets rid of outdated locking (socket instead of
>> l2cap_chan) and an extra lookup of the channel ID.
>
> I don't think we can remove this code from here. This check is different
> from the ones you are talking, those are l2cap packets, here we are still
> dealing with ACL fragments. This check is there to avoid accept a first
> ACL packet that is too big. See 8979481328d.
>
> One possible solution is to add a slack value to the check, so we can
> fit those 10+ bytes packets in it.
If we just add slack, then we're depending on the real MTU checks to
work correctly later. Why bother with this early check at all?
I think there's a misunderstanding about the code that is being
removed. It *is* checking the L2CAP MTU for that channel against the
value in the L2CAP length header before the whole frame is assembled,
which is why it shouldn't be involved with ACL fragments to begin
with. It is the only place in l2cap_recv_acldata that uses
channel-specific information.
This code tries to throw out the first ACL fragment if the L2CAP
payload is longer than than the L2CAP MTU for that channel. The ERTM
and streaming mode MTU has different meaning - it applies to the
reassembled SDU payload, not the size of an individual PDU segment
with the extra headers and FCS bytes. There is already a length check
in the fragment reassembly code that makes sure no reassembled ACL
frame exceeds 65535 bytes (look at how conn->rx_len is used).
The original discussion around this code is here:
http://thread.gmane.org/gmane.linux.bluez.kernel/7505
The purpose was to address a memory allocation failure in
__get_free_pages - which suggests heap corruption. Even if the start
fragment is too large, that shouldn't lead to heap corruption,
especially if the fragment is properly freed. Throwing out this large
packet is just hiding the real bug!
The MTU is checked everywhere that it needs to be for L2CAP data,
after the ACL fragments are reassembled:
* In l2cap_reassemble_sdu for ERTM and Streaming Mode
* In l2cap_data_channel for Basic Mode
* In l2cap_connless_channel for connectionless channels
The code being removed doesn't address the signaling channel, because
that channel is not in the channel list. The spec defines the MTU for
the signaling channel to be "MTUsig", and only requires it to be at
least 48 bytes (672 bytes if extended flowspec is supported). It is
valid for us not to enforce a limit on it other than the maximum L2CAP
frame size.
We removed this code (because it broke AMP) from our Android kernel
back in September 2011, and have been through several qualifications
and rounds of testing since then without problems.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Gustavo -
On Fri, 27 Apr 2012, Gustavo Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2012-04-27 16:50:53 -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 | 15 ++++++++-------
>> 2 files changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
>> index 2fb268f..90678a9 100644
>> --- a/include/net/bluetooth/bluetooth.h
>> +++ b/include/net/bluetooth/bluetooth.h
>> @@ -256,12 +256,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 82b6368..ac8ce10 100644
>> --- a/net/bluetooth/l2cap_sock.c
>> +++ b/net/bluetooth/l2cap_sock.c
>> @@ -716,16 +716,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);
>
> I would move these l2cap_chan_{,un}lock calls to inside
> l2cap_chan_send(). The less we use l2cap_chan_* functions here the
> better.
In V1 of this RFC set, Andrei requested that the locking calls be
outside of l2cap_chan_send(). The A2MP code uses l2cap_chan_send(),
and already holds the channel lock.
>>
>> - release_sock(sk);
>> return err;
>> }
>>
>> @@ -936,9 +933,13 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
>> struct sk_buff *skb;
>> int err;
>>
>> + l2cap_chan_unlock(chan);
>> +
>> skb = bt_skb_send_alloc(chan->sk, len, nb, &err);
>> if (!skb)
>> - return (ERR_PTR(err));
>> + skb = ERR_PTR(err);
>> +
>> + l2cap_chan_lock(chan);
>
> I would suggest the same here, but it might not be worth, since we would
> need to create wrapper to call alloc_skb().
bt_skb_send_alloc is also used by HCI raw sockets and SCO, so the
locking needs to be done here.
Regards,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Gustavo -
On Sun, 29 Apr 2012, Gustavo Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <[email protected]> [2012-04-27 16:50:52 -0700]:
>
>> 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(-)
>
> Applied to bluetooth-next. Thanks.
Please revert this for now. This patch causes a locking imbalance if
patch 4/8 is not merged first, and is the main reason I requested that
*none* of these patches be merged yet in my cover letter message.
Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
* Mat Martineau <[email protected]> [2012-04-27 16:50:55 -0700]:
> Adding Code Aurora Forum copyright information due to significant
> additions of code.
>
> Acked-by: Marcel Holtmann <[email protected]>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 1 +
> 1 file changed, 1 insertion(+)
Applied to bluetooth-next. Thanks.
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2012-04-27 16:50:52 -0700]:
> 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(-)
Applied to bluetooth-next. Thanks.
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2012-04-27 16:50:50 -0700]:
> L2CAP sockets contain a pointer to l2cap_chan that needs to be
> reference counted in order to prevent a possible dangling pointer when
> the channel is freed.
>
> There were a few other cases where an l2cap_chan pointer on the stack
> was dereferenced after a call to l2cap_chan_del. Those pointers are
> also now reference counted.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 6 ++++++
> net/bluetooth/l2cap_sock.c | 3 +++
> 2 files changed, 9 insertions(+)
Patches 1 to 3 applied to bluetooth-next. Thanks.
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2012-04-27 16:50:53 -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 | 15 ++++++++-------
> 2 files changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 2fb268f..90678a9 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -256,12 +256,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 82b6368..ac8ce10 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -716,16 +716,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);
I would move these l2cap_chan_{,un}lock calls to inside
l2cap_chan_send(). The less we use l2cap_chan_* functions here the
better.
>
> - release_sock(sk);
> return err;
> }
>
> @@ -936,9 +933,13 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
> struct sk_buff *skb;
> int err;
>
> + l2cap_chan_unlock(chan);
> +
> skb = bt_skb_send_alloc(chan->sk, len, nb, &err);
> if (!skb)
> - return (ERR_PTR(err));
> + skb = ERR_PTR(err);
> +
> + l2cap_chan_lock(chan);
I would suggest the same here, but it might not be worth, since we would
need to create wrapper to call alloc_skb().
Gustavo
Hi Mat,
* Mat Martineau <[email protected]> [2012-04-27 16:50:51 -0700]:
> The L2CAP MTU for incoming data is verified differently depending on
> the L2CAP mode, so the check is best performed in a mode-specific
> context. Checking the incoming MTU before HCI fragment reassembly is
> a layer violation and assumes all bytes after the standard L2CAP
> header are L2CAP data.
>
> This approadch causes issues with unsegmented ERTM or streaming mode
> frames, where there are additional enhanced or extended headers before
> the data payload and possible FCS bytes after the data payload. A
> valid frame could be as many as 10 bytes larger than the MTU.
>
> Removing this code is the best fix, because the MTU is checked later
> on for all L2CAP data frames (connectionless, basic, ERTM, and
> streaming). This also gets rid of outdated locking (socket instead of
> l2cap_chan) and an extra lookup of the channel ID.
I don't think we can remove this code from here. This check is different
from the ones you are talking, those are l2cap packets, here we are still
dealing with ACL fragments. This check is there to avoid accept a first
ACL packet that is too big. See 8979481328d.
One possible solution is to add a slack value to the check, so we can
fit those 10+ bytes packets in it.
Gustavo
Use more common code for ERTM and streaming mode segmentation and
transmission, and begin using skb control block data for delaying
extended or enhanced header generation until just before the packet is
transmitted. This code is also better suited for resegmentation,
which is needed when L2CAP links are reconfigured after an AMP channel
move.
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/l2cap_core.c | 154 ++++++++++++++++++++++++-----------------
2 files changed, 92 insertions(+), 63 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 92c0423..88c128a 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -44,6 +44,7 @@
#define L2CAP_DEFAULT_MAX_SDU_SIZE 0xFFFF
#define L2CAP_DEFAULT_SDU_ITIME 0xFFFFFFFF
#define L2CAP_DEFAULT_ACC_LAT 0xFFFFFFFF
+#define L2CAP_BREDR_MAX_PAYLOAD 1019 /* 3-DH5 packet */
#define L2CAP_DISC_TIMEOUT msecs_to_jiffies(100)
#define L2CAP_DISC_REJ_TIMEOUT msecs_to_jiffies(5000)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 347aaa5..d424b86 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1636,6 +1636,7 @@ static void l2cap_streaming_send(struct l2cap_chan *chan)
while ((skb = skb_dequeue(&chan->tx_q))) {
control = __get_control(chan, skb->data + L2CAP_HDR_SIZE);
control |= __set_txseq(chan, chan->next_tx_seq);
+ control |= __set_ctrl_sar(chan, bt_cb(skb)->control.sar);
__put_control(chan, control, skb->data + L2CAP_HDR_SIZE);
if (chan->fcs == L2CAP_FCS_CRC16) {
@@ -1708,6 +1709,9 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
if (chan->state != BT_CONNECTED)
return -ENOTCONN;
+ if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state))
+ return 0;
+
while ((skb = chan->tx_send_head) && (!l2cap_tx_window_full(chan))) {
if (chan->remote_max_tx &&
@@ -1728,6 +1732,7 @@ static int l2cap_ertm_send(struct l2cap_chan *chan)
control |= __set_reqseq(chan, chan->buffer_seq);
control |= __set_txseq(chan, chan->next_tx_seq);
+ control |= __set_ctrl_sar(chan, bt_cb(skb)->control.sar);
__put_control(chan, control, tx_skb->data + L2CAP_HDR_SIZE);
@@ -1923,7 +1928,7 @@ static struct sk_buff *l2cap_create_basic_pdu(struct l2cap_chan *chan,
static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
struct msghdr *msg, size_t len,
- u32 control, u16 sdulen)
+ u16 sdulen)
{
struct l2cap_conn *conn = chan->conn;
struct sk_buff *skb;
@@ -1958,7 +1963,7 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
lh->cid = cpu_to_le16(chan->dcid);
lh->len = cpu_to_le16(len + (hlen - L2CAP_HDR_SIZE));
- __put_control(chan, control, skb_put(skb, __ctrl_size(chan)));
+ __put_control(chan, 0, skb_put(skb, __ctrl_size(chan)));
if (sdulen)
put_unaligned_le16(sdulen, skb_put(skb, L2CAP_SDULEN_SIZE));
@@ -1976,57 +1981,78 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct l2cap_chan *chan,
return skb;
}
-static int l2cap_sar_segment_sdu(struct l2cap_chan *chan, struct msghdr *msg, size_t len)
+static int l2cap_segment_sdu(struct l2cap_chan *chan,
+ struct sk_buff_head *seg_queue,
+ struct msghdr *msg, size_t len)
{
struct sk_buff *skb;
- struct sk_buff_head sar_queue;
- u32 control;
- size_t size = 0;
+ u16 sdu_len;
+ size_t pdu_len;
+ int err = 0;
+ u8 sar;
- skb_queue_head_init(&sar_queue);
- control = __set_ctrl_sar(chan, L2CAP_SAR_START);
- skb = l2cap_create_iframe_pdu(chan, msg, chan->remote_mps, control, len);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
+ BT_DBG("chan %p, msg %p, len %d", chan, msg, (int)len);
- __skb_queue_tail(&sar_queue, skb);
- len -= chan->remote_mps;
- size += chan->remote_mps;
+ /* It is critical that ERTM PDUs fit in a single HCI fragment,
+ * so fragmented skbs are not used. The HCI layer's handling
+ * of fragmented skbs is not compatible with ERTM's queueing.
+ */
+
+ /* PDU size is derived from the HCI MTU */
+ pdu_len = chan->conn->mtu;
+
+ pdu_len = min_t(size_t, pdu_len, L2CAP_BREDR_MAX_PAYLOAD);
+
+ /* Adjust for largest possible L2CAP overhead. */
+ pdu_len -= L2CAP_EXT_HDR_SIZE + L2CAP_FCS_SIZE;
+
+ /* Remote device may have requested smaller PDUs */
+ pdu_len = min_t(size_t, pdu_len, chan->remote_mps);
+
+ if (len <= pdu_len) {
+ sar = L2CAP_SAR_UNSEGMENTED;
+ sdu_len = 0;
+ pdu_len = len;
+ } else {
+ sar = L2CAP_SAR_START;
+ sdu_len = len;
+ pdu_len -= L2CAP_SDULEN_SIZE;
+ }
while (len > 0) {
- size_t buflen;
+ skb = l2cap_create_iframe_pdu(chan, msg, pdu_len, sdu_len);
- if (len > chan->remote_mps) {
- control = __set_ctrl_sar(chan, L2CAP_SAR_CONTINUE);
- buflen = chan->remote_mps;
- } else {
- control = __set_ctrl_sar(chan, L2CAP_SAR_END);
- buflen = len;
- }
-
- skb = l2cap_create_iframe_pdu(chan, msg, buflen, control, 0);
if (IS_ERR(skb)) {
- skb_queue_purge(&sar_queue);
+ __skb_queue_purge(seg_queue);
return PTR_ERR(skb);
}
- __skb_queue_tail(&sar_queue, skb);
- len -= buflen;
- size += buflen;
- }
- skb_queue_splice_tail(&sar_queue, &chan->tx_q);
- if (chan->tx_send_head == NULL)
- chan->tx_send_head = sar_queue.next;
+ bt_cb(skb)->control.sar = sar;
+ __skb_queue_tail(seg_queue, skb);
- return size;
+ len -= pdu_len;
+ if (sdu_len) {
+ sdu_len = 0;
+ pdu_len += L2CAP_SDULEN_SIZE;
+ }
+
+ if (len <= pdu_len) {
+ sar = L2CAP_SAR_END;
+ pdu_len = len;
+ } else {
+ sar = L2CAP_SAR_CONTINUE;
+ }
+ }
+
+ return err;
}
int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
u32 priority)
{
struct sk_buff *skb;
- u32 control;
int err;
+ struct sk_buff_head seg_queue;
/* Connectionless channel */
if (chan->chan_type == L2CAP_CHAN_CONN_LESS) {
@@ -2055,42 +2081,44 @@ int l2cap_chan_send(struct l2cap_chan *chan, struct msghdr *msg, size_t len,
case L2CAP_MODE_ERTM:
case L2CAP_MODE_STREAMING:
- /* Entire SDU fits into one PDU */
- if (len <= chan->remote_mps) {
- control = __set_ctrl_sar(chan, L2CAP_SAR_UNSEGMENTED);
- skb = l2cap_create_iframe_pdu(chan, msg, len, control,
- 0);
- if (IS_ERR(skb))
- return PTR_ERR(skb);
-
- __skb_queue_tail(&chan->tx_q, skb);
-
- if (chan->tx_send_head == NULL)
- chan->tx_send_head = skb;
-
- } else {
- /* Segment SDU into multiples PDUs */
- err = l2cap_sar_segment_sdu(chan, msg, len);
- if (err < 0)
- return err;
+ /* Check outgoing MTU */
+ if (len > chan->omtu) {
+ err = -EMSGSIZE;
+ break;
}
- if (chan->mode == L2CAP_MODE_STREAMING) {
+ __skb_queue_head_init(&seg_queue);
+
+ /* Do segmentation before calling in to the state machine,
+ * since it's possible to block while waiting for memory
+ * allocation.
+ */
+ err = l2cap_segment_sdu(chan, &seg_queue, msg, len);
+
+ /* The channel could have been closed while segmenting,
+ * check that it is still connected.
+ */
+ if (chan->state != BT_CONNECTED) {
+ __skb_queue_purge(&seg_queue);
+ err = -ENOTCONN;
+ }
+
+ if (err)
+ break;
+
+ skb_queue_splice_tail_init(&seg_queue, &chan->tx_q);
+ if (chan->mode == L2CAP_MODE_ERTM)
+ err = l2cap_ertm_send(chan);
+ else
l2cap_streaming_send(chan);
- err = len;
- break;
- }
- if (test_bit(CONN_REMOTE_BUSY, &chan->conn_state) &&
- test_bit(CONN_WAIT_F, &chan->conn_state)) {
- err = len;
- break;
- }
-
- err = l2cap_ertm_send(chan);
if (err >= 0)
err = len;
+ /* If the skbs were not queued for sending, they'll still be in
+ * seg_queue and need to be purged.
+ */
+ __skb_queue_purge(&seg_queue);
break;
default:
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Adding Code Aurora Forum copyright information due to significant
additions of code.
Acked-by: Marcel Holtmann <[email protected]>
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d424b86..b855847 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4,6 +4,7 @@
Copyright (C) 2009-2010 Gustavo F. Padovan <[email protected]>
Copyright (C) 2010 Google Inc.
Copyright (C) 2011 ProFUSION Embedded Systems
+ Copyright (c) 2012 Code Aurora Forum. All rights reserved.
Written 2000,2001 by Maxim Krasnyansky <[email protected]>
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
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 e543b53..347aaa5 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;
@@ -3141,8 +3143,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;
@@ -3255,8 +3255,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);
@@ -4589,8 +4587,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
Structure members used by ERTM or streaming mode need to be
initialized when an ERTM or streaming mode link is configured. Some
duplicate code is also eliminated by moving in to the ERTM init
function.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f19f7bc..78a334b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2315,17 +2315,30 @@ static inline int l2cap_ertm_init(struct l2cap_chan *chan)
{
int err;
+ chan->next_tx_seq = 0;
+ chan->expected_tx_seq = 0;
chan->expected_ack_seq = 0;
chan->unacked_frames = 0;
chan->buffer_seq = 0;
chan->num_acked = 0;
chan->frames_sent = 0;
+ chan->last_acked_seq = 0;
+ chan->sdu = NULL;
+ chan->sdu_last_frag = NULL;
+ chan->sdu_len = 0;
+
+ if (chan->mode != L2CAP_MODE_ERTM)
+ return 0;
+
+ chan->rx_state = L2CAP_RX_STATE_RECV;
+ chan->tx_state = L2CAP_TX_STATE_XMIT;
INIT_DELAYED_WORK(&chan->retrans_timer, l2cap_retrans_timeout);
INIT_DELAYED_WORK(&chan->monitor_timer, l2cap_monitor_timeout);
INIT_DELAYED_WORK(&chan->ack_timer, l2cap_ack_timeout);
skb_queue_head_init(&chan->srej_q);
+ skb_queue_head_init(&chan->tx_q);
INIT_LIST_HEAD(&chan->srej_l);
err = l2cap_seq_list_init(&chan->srej_list, chan->tx_win);
@@ -3193,10 +3206,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
l2cap_state_change(chan, BT_CONNECTED);
- chan->next_tx_seq = 0;
- chan->expected_tx_seq = 0;
- skb_queue_head_init(&chan->tx_q);
- if (chan->mode == L2CAP_MODE_ERTM)
+ if (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_STREAMING)
err = l2cap_ertm_init(chan);
if (err < 0)
@@ -3328,10 +3339,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
set_default_fcs(chan);
l2cap_state_change(chan, BT_CONNECTED);
- chan->next_tx_seq = 0;
- chan->expected_tx_seq = 0;
- skb_queue_head_init(&chan->tx_q);
- if (chan->mode == L2CAP_MODE_ERTM)
+ if (chan->mode == L2CAP_MODE_ERTM ||
+ chan->mode == L2CAP_MODE_STREAMING)
err = l2cap_ertm_init(chan);
if (err < 0)
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
The L2CAP MTU for incoming data is verified differently depending on
the L2CAP mode, so the check is best performed in a mode-specific
context. Checking the incoming MTU before HCI fragment reassembly is
a layer violation and assumes all bytes after the standard L2CAP
header are L2CAP data.
This approadch causes issues with unsegmented ERTM or streaming mode
frames, where there are additional enhanced or extended headers before
the data payload and possible FCS bytes after the data payload. A
valid frame could be as many as 10 bytes larger than the MTU.
Removing this code is the best fix, because the MTU is checked later
on for all L2CAP data frames (connectionless, basic, ERTM, and
streaming). This also gets rid of outdated locking (socket instead of
l2cap_chan) and an extra lookup of the channel ID.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5963cd2..e543b53 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4953,8 +4953,6 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
if (!(flags & ACL_CONT)) {
struct l2cap_hdr *hdr;
- struct l2cap_chan *chan;
- u16 cid;
int len;
if (conn->rx_len) {
@@ -4974,7 +4972,6 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
hdr = (struct l2cap_hdr *) skb->data;
len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
- cid = __le16_to_cpu(hdr->cid);
if (len == skb->len) {
/* Complete frame received */
@@ -4991,23 +4988,6 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
goto drop;
}
- chan = l2cap_get_chan_by_scid(conn, cid);
-
- 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, "
- "MTU %d)", len,
- chan->imtu);
- release_sock(sk);
- l2cap_conn_unreliable(conn, ECOMM);
- goto drop;
- }
- release_sock(sk);
- }
-
/* Allocate skb for the complete frame (with header) */
conn->rx_skb = bt_skb_alloc(len, GFP_ATOMIC);
if (!conn->rx_skb)
--
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 | 15 ++++++++-------
2 files changed, 8 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 2fb268f..90678a9 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -256,12 +256,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 82b6368..ac8ce10 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -716,16 +716,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;
}
@@ -936,9 +933,13 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
struct sk_buff *skb;
int err;
+ l2cap_chan_unlock(chan);
+
skb = bt_skb_send_alloc(chan->sk, len, nb, &err);
if (!skb)
- return (ERR_PTR(err));
+ skb = ERR_PTR(err);
+
+ l2cap_chan_lock(chan);
return skb;
}
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
L2CAP sockets contain a pointer to l2cap_chan that needs to be
reference counted in order to prevent a possible dangling pointer when
the channel is freed.
There were a few other cases where an l2cap_chan pointer on the stack
was dereferenced after a call to l2cap_chan_del. Those pointers are
also now reference counted.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 6 ++++++
net/bluetooth/l2cap_sock.c | 3 +++
2 files changed, 9 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c3d3cfc..5963cd2 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1257,6 +1257,7 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
/* Kill channels */
list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
+ l2cap_chan_hold(chan);
l2cap_chan_lock(chan);
l2cap_chan_del(chan, err);
@@ -1264,6 +1265,7 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
l2cap_chan_unlock(chan);
chan->ops->close(chan->data);
+ l2cap_chan_put(chan);
}
mutex_unlock(&conn->chan_lock);
@@ -3376,11 +3378,13 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
sk->sk_shutdown = SHUTDOWN_MASK;
release_sock(sk);
+ l2cap_chan_hold(chan);
l2cap_chan_del(chan, ECONNRESET);
l2cap_chan_unlock(chan);
chan->ops->close(chan->data);
+ l2cap_chan_put(chan);
mutex_unlock(&conn->chan_lock);
@@ -3408,11 +3412,13 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
l2cap_chan_lock(chan);
+ l2cap_chan_hold(chan);
l2cap_chan_del(chan, 0);
l2cap_chan_unlock(chan);
chan->ops->close(chan->data);
+ l2cap_chan_put(chan);
mutex_unlock(&conn->chan_lock);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0f30785..82b6368 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -956,6 +956,7 @@ static void l2cap_sock_destruct(struct sock *sk)
{
BT_DBG("sk %p", sk);
+ l2cap_chan_put(l2cap_pi(sk)->chan);
if (l2cap_pi(sk)->rx_busy_skb) {
kfree_skb(l2cap_pi(sk)->rx_busy_skb);
l2cap_pi(sk)->rx_busy_skb = NULL;
@@ -1057,6 +1058,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock, int p
return NULL;
}
+ l2cap_chan_hold(chan);
+
chan->sk = sk;
l2cap_pi(sk)->chan = chan;
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
l2cap_get_chan_by_ident was not used, but didn't generate a compiler
warning because it was an inline function.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/l2cap_core.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 78a334b..c3d3cfc 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -121,17 +121,6 @@ static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8
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);
- mutex_unlock(&conn->chan_lock);
-
- return c;
-}
-
static struct l2cap_chan *__l2cap_global_chan_by_addr(__le16 psm, bdaddr_t *src)
{
struct l2cap_chan *c;
--
1.7.10
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum