This patch series changes SDU reassembly to link together PDU skbs
using a frag list instead of copying all data bytes to a single,
linear skb. The reduction in overhead is especially helpful with high
speed AMP links.
Mat Martineau (3):
Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM
Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg()
Bluetooth: Perform L2CAP SDU reassembly without copying data
include/net/bluetooth/l2cap.h | 3 +-
net/bluetooth/af_bluetooth.c | 30 +++++-
net/bluetooth/bnep/core.c | 5 +-
net/bluetooth/cmtp/core.c | 5 +-
net/bluetooth/hidp/core.c | 10 ++-
net/bluetooth/l2cap_core.c | 239 ++++++++++++++---------------------------
net/bluetooth/rfcomm/core.c | 5 +-
7 files changed, 128 insertions(+), 169 deletions(-)
--
1.7.6
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Gustavo -
On Fri, 22 Jul 2011, Gustavo Padovan wrote:
> * Mat Martineau <[email protected]> [2011-07-13 10:58:38 -0700]:
>
>> ERTM reassembly will be more efficient when skbs are linked together
>> rather than copying every incoming data byte. The existing stream recv
>> function assumes skbs are linear, so it needs to know how to handle
>> fragments before reassembly is changed.
>>
>> bt_sock_recvmsg() already handles fragmented skbs.
>>
>> Signed-off-by: Mat Martineau <[email protected]>
>> ---
>> net/bluetooth/af_bluetooth.c | 30 ++++++++++++++++++++++++++++--
>> 1 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
>> index 8add9b4..9a43520 100644
>> --- a/net/bluetooth/af_bluetooth.c
>> +++ b/net/bluetooth/af_bluetooth.c
>> @@ -349,7 +349,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>> }
>>
>> chunk = min_t(unsigned int, skb->len, size);
>> - if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
>> + if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) {
>> skb_queue_head(&sk->sk_receive_queue, skb);
>> if (!copied)
>> copied = -EFAULT;
>> @@ -361,7 +361,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
>> sock_recv_ts_and_drops(msg, sk, skb);
>>
>> if (!(flags & MSG_PEEK)) {
>> - skb_pull(skb, chunk);
>> + int skb_len = skb_headlen(skb);
>> +
>> + if (chunk <= skb_len) {
>> + __skb_pull(skb, chunk);
>> + } else {
>> + struct sk_buff *frag;
>> +
>> + __skb_pull(skb, skb_len);
>> + chunk -= skb_len;
>> +
>> + skb_walk_frags(skb, frag) {
>> + if (chunk <= frag->len) {
>> + /* Pulling partial data */
>> + skb->len -= chunk;
>> + skb->data_len -= chunk;
>> + __skb_pull(frag, chunk);
>> + break;
>> + } else if (frag->len) {
>> + /* Pulling all frag data */
>> + chunk -= frag->len;
>> + skb->len -= frag->len;
>> + skb->data_len -= frag->len;
>> + __skb_pull(frag, frag->len);
>> + }
>> + }
>> + }
>> +
>
> ERTM and Streaming mode can also use SOCK_SEQPACKET, I think you also need to
> handle this for SOCK_SEQPACKET.
bt_sock_recvmsg() is used with SOCK_SEQPACKET, and it already handles
fragmented skbs without any changes. See skb_copy_datagram_iovec(),
which is called by bt_sock_recvmsg() to copy the data out of the skb.
--
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]> [2011-07-13 10:58:39 -0700]:
> Use sk_buff fragment capabilities to link together incoming skbs
> instead of allocating a new skb for reassembly and copying.
>
> The new reassembly code works equally well for ERTM and streaming
> mode, so there is now one reassembly function instead of two.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> include/net/bluetooth/l2cap.h | 3 +-
> net/bluetooth/l2cap_core.c | 239 ++++++++++++++---------------------------
> 2 files changed, 80 insertions(+), 162 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 4f34ad2..6fa1140 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -354,8 +354,8 @@ struct l2cap_chan {
> __u8 retry_count;
> __u8 num_acked;
> __u16 sdu_len;
> - __u16 partial_sdu_len;
> struct sk_buff *sdu;
> + struct sk_buff *sdu_last_frag;
>
> __u8 remote_tx_win;
> __u8 remote_max_tx;
> @@ -454,7 +454,6 @@ enum {
> #define L2CAP_CONF_MAX_CONF_RSP 2
>
> enum {
> - CONN_SAR_SDU,
> CONN_SREJ_SENT,
> CONN_WAIT_F,
> CONN_SREJ_ACT,
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 52c791e..e82fff1 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3120,102 +3120,104 @@ static int l2cap_add_to_srej_queue(struct l2cap_chan *chan, struct sk_buff *skb,
> return 0;
> }
>
> -static int l2cap_ertm_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control)
> +static void append_skb_frag(struct sk_buff *skb,
> + struct sk_buff *new_frag, struct sk_buff **last_frag)
> {
> - struct sk_buff *_skb;
> - int err;
> + /* skb->len reflects data in skb as well as all fragments
> + * skb->data_len reflects only data in fragments
> + */
> + if (!skb_has_frag_list(skb))
> + skb_shinfo(skb)->frag_list = new_frag;
> +
> + new_frag->next = NULL;
> +
> + (*last_frag)->next = new_frag;
> + *last_frag = new_frag;
> +
> + skb->len += new_frag->len;
> + skb->data_len += new_frag->len;
> + skb->truesize += new_frag->truesize;
> +}
> +
> +static int l2cap_reassemble_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control)
> +{
> + int err = -EINVAL;
>
> switch (control & L2CAP_CTRL_SAR) {
> case L2CAP_SDU_UNSEGMENTED:
> - if (test_bit(CONN_SAR_SDU, &chan->conn_state))
> - goto drop;
> + if (chan->sdu)
> + break;
>
> - return chan->ops->recv(chan->data, skb);
> + err = chan->ops->recv(chan->data, skb);
> + break;
>
> case L2CAP_SDU_START:
> - if (test_bit(CONN_SAR_SDU, &chan->conn_state))
> - goto drop;
> + if (chan->sdu)
> + break;
>
> chan->sdu_len = get_unaligned_le16(skb->data);
> + skb_pull(skb, 2);
>
> - if (chan->sdu_len > chan->imtu)
> - goto disconnect;
> -
> - chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC);
> - if (!chan->sdu)
> - return -ENOMEM;
> + if (chan->sdu_len > chan->imtu) {
> + err = -EMSGSIZE;
> + break;
> + }
>
> - /* pull sdu_len bytes only after alloc, because of Local Busy
> - * condition we have to be sure that this will be executed
> - * only once, i.e., when alloc does not fail */
> - skb_pull(skb, 2);
> + if (skb->len >= chan->sdu_len)
> + break;
>
> - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
> + chan->sdu = skb;
> + chan->sdu_last_frag = skb;
>
> - set_bit(CONN_SAR_SDU, &chan->conn_state);
> - chan->partial_sdu_len = skb->len;
> + skb = NULL;
> + err = 0;
> break;
>
> case L2CAP_SDU_CONTINUE:
> - if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
> - goto disconnect;
> -
> if (!chan->sdu)
> - goto disconnect;
> + break;
>
> - chan->partial_sdu_len += skb->len;
> - if (chan->partial_sdu_len > chan->sdu_len)
> - goto drop;
> + append_skb_frag(chan->sdu, skb,
> + &chan->sdu_last_frag);
> + skb = NULL;
>
> - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
> + if (chan->sdu->len >= chan->sdu_len)
> + break;
>
> + err = 0;
> break;
>
> case L2CAP_SDU_END:
> - if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
> - goto disconnect;
> -
> if (!chan->sdu)
> - goto disconnect;
> -
> - chan->partial_sdu_len += skb->len;
> -
> - if (chan->partial_sdu_len > chan->imtu)
> - goto drop;
> + break;
>
> - if (chan->partial_sdu_len != chan->sdu_len)
> - goto drop;
> + append_skb_frag(chan->sdu, skb,
> + &chan->sdu_last_frag);
> + skb = NULL;
>
> - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
> + if (chan->sdu->len != chan->sdu_len)
> + break;
>
> - _skb = skb_clone(chan->sdu, GFP_ATOMIC);
> - if (!_skb) {
> - return -ENOMEM;
> - }
> + err = chan->ops->recv(chan->data, chan->sdu);
>
> - err = chan->ops->recv(chan->data, _skb);
> - if (err < 0) {
> - kfree_skb(_skb);
> - return err;
> + if (!err) {
> + /* Reassembly complete */
> + chan->sdu = NULL;
> + chan->sdu_last_frag = NULL;
> + chan->sdu_len = 0;
> }
> -
> - clear_bit(CONN_SAR_SDU, &chan->conn_state);
> -
> - kfree_skb(chan->sdu);
> break;
> }
>
> - kfree_skb(skb);
> - return 0;
> -
> -drop:
> - kfree_skb(chan->sdu);
> - chan->sdu = NULL;
> + if (err) {
> + kfree_skb(skb);
> + kfree_skb(chan->sdu);
> + chan->sdu = NULL;
> + chan->sdu_last_frag = NULL;
> + chan->sdu_len = 0;
> + }
>
> -disconnect:
> - l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
> - kfree_skb(skb);
> - return 0;
> + return err;
> }
>
> static void l2cap_ertm_enter_local_busy(struct l2cap_chan *chan)
> @@ -3269,99 +3271,6 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
> }
> }
>
> -static int l2cap_streaming_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control)
> -{
> - struct sk_buff *_skb;
> - int err = -EINVAL;
> -
> - /*
> - * TODO: We have to notify the userland if some data is lost with the
> - * Streaming Mode.
> - */
> -
> - switch (control & L2CAP_CTRL_SAR) {
> - case L2CAP_SDU_UNSEGMENTED:
> - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) {
> - kfree_skb(chan->sdu);
> - break;
> - }
> -
> - err = chan->ops->recv(chan->data, skb);
> - if (!err)
> - return 0;
> -
> - break;
> -
> - case L2CAP_SDU_START:
> - if (test_bit(CONN_SAR_SDU, &chan->conn_state)) {
> - kfree_skb(chan->sdu);
> - break;
> - }
> -
> - chan->sdu_len = get_unaligned_le16(skb->data);
> - skb_pull(skb, 2);
> -
> - if (chan->sdu_len > chan->imtu) {
> - err = -EMSGSIZE;
> - break;
> - }
> -
> - chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC);
> - if (!chan->sdu) {
> - err = -ENOMEM;
> - break;
> - }
> -
> - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
> -
> - set_bit(CONN_SAR_SDU, &chan->conn_state);
> - chan->partial_sdu_len = skb->len;
> - err = 0;
> - break;
> -
> - case L2CAP_SDU_CONTINUE:
> - if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
> - break;
> -
> - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
> -
> - chan->partial_sdu_len += skb->len;
> - if (chan->partial_sdu_len > chan->sdu_len)
> - kfree_skb(chan->sdu);
> - else
> - err = 0;
> -
> - break;
> -
> - case L2CAP_SDU_END:
> - if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
> - break;
> -
> - memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
> -
> - clear_bit(CONN_SAR_SDU, &chan->conn_state);
> - chan->partial_sdu_len += skb->len;
> -
> - if (chan->partial_sdu_len > chan->imtu)
> - goto drop;
> -
> - if (chan->partial_sdu_len == chan->sdu_len) {
> - _skb = skb_clone(chan->sdu, GFP_ATOMIC);
> - err = chan->ops->recv(chan->data, _skb);
> - if (err < 0)
> - kfree_skb(_skb);
> - }
> - err = 0;
> -
> -drop:
> - kfree_skb(chan->sdu);
> - break;
> - }
> -
> - kfree_skb(skb);
> - return err;
> -}
> -
> static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq)
> {
> struct sk_buff *skb;
> @@ -3376,7 +3285,7 @@ static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq)
>
> skb = skb_dequeue(&chan->srej_q);
> control = bt_cb(skb)->sar << L2CAP_CTRL_SAR_SHIFT;
> - err = l2cap_ertm_reassembly_sdu(chan, skb, control);
> + err = l2cap_reassemble_sdu(chan, skb, control);
>
> if (err < 0) {
> l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
> @@ -3536,7 +3445,7 @@ expected:
> return 0;
> }
>
> - err = l2cap_ertm_reassembly_sdu(chan, skb, rx_control);
> + err = l2cap_reassemble_sdu(chan, skb, rx_control);
> chan->buffer_seq = (chan->buffer_seq + 1) % 64;
> if (err < 0) {
> l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
> @@ -3854,10 +3763,20 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
>
> if (chan->expected_tx_seq == tx_seq)
> chan->expected_tx_seq = (chan->expected_tx_seq + 1) % 64;
Coding style issue: you also need {} in the 'if'. If you need it on 'else'
then also add it to 'if.'
> - else
> + else {
Gustavo
* Mat Martineau <[email protected]> [2011-07-13 10:58:38 -0700]:
> ERTM reassembly will be more efficient when skbs are linked together
> rather than copying every incoming data byte. The existing stream recv
> function assumes skbs are linear, so it needs to know how to handle
> fragments before reassembly is changed.
>
> bt_sock_recvmsg() already handles fragmented skbs.
>
> Signed-off-by: Mat Martineau <[email protected]>
> ---
> net/bluetooth/af_bluetooth.c | 30 ++++++++++++++++++++++++++++--
> 1 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
> index 8add9b4..9a43520 100644
> --- a/net/bluetooth/af_bluetooth.c
> +++ b/net/bluetooth/af_bluetooth.c
> @@ -349,7 +349,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> }
>
> chunk = min_t(unsigned int, skb->len, size);
> - if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
> + if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) {
> skb_queue_head(&sk->sk_receive_queue, skb);
> if (!copied)
> copied = -EFAULT;
> @@ -361,7 +361,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> sock_recv_ts_and_drops(msg, sk, skb);
>
> if (!(flags & MSG_PEEK)) {
> - skb_pull(skb, chunk);
> + int skb_len = skb_headlen(skb);
> +
> + if (chunk <= skb_len) {
> + __skb_pull(skb, chunk);
> + } else {
> + struct sk_buff *frag;
> +
> + __skb_pull(skb, skb_len);
> + chunk -= skb_len;
> +
> + skb_walk_frags(skb, frag) {
> + if (chunk <= frag->len) {
> + /* Pulling partial data */
> + skb->len -= chunk;
> + skb->data_len -= chunk;
> + __skb_pull(frag, chunk);
> + break;
> + } else if (frag->len) {
> + /* Pulling all frag data */
> + chunk -= frag->len;
> + skb->len -= frag->len;
> + skb->data_len -= frag->len;
> + __skb_pull(frag, frag->len);
> + }
> + }
> + }
> +
ERTM and Streaming mode can also use SOCK_SEQPACKET, I think you also need to
handle this for SOCK_SEQPACKET.
Gustavo
Hi Mat,
> >>> This patch series changes SDU reassembly to link together PDU skbs
> >>> using a frag list instead of copying all data bytes to a single,
> >>> linear skb. The reduction in overhead is especially helpful with high
> >>> speed AMP links.
> >
> > I have not looked at these at all, but one thing that came up in a
> > discussion between Johan and myself with OBEX zero-copy was that we like
> > to also see MSG_MORE support for L2CAP and RFCOMM sockets. Something to
> > keep in mind ;)
>
> Noted! It would be nice to build up OBEX packets in the kernel,
> rather than having to assemble entire packets in an application
> buffer.
hence the request for MSG_MORE support. That would allow us to build
frames properly. Could also become a pretty nice enhancement for A2DP.
Regards
Marcel
Hi Marcel -
On Mon, 18 Jul 2011, Marcel Holtmann wrote:
> Hi Mat,
>
>>> This patch series changes SDU reassembly to link together PDU skbs
>>> using a frag list instead of copying all data bytes to a single,
>>> linear skb. The reduction in overhead is especially helpful with high
>>> speed AMP links.
>
> I have not looked at these at all, but one thing that came up in a
> discussion between Johan and myself with OBEX zero-copy was that we like
> to also see MSG_MORE support for L2CAP and RFCOMM sockets. Something to
> keep in mind ;)
Noted! It would be nice to build up OBEX packets in the kernel,
rather than having to assemble entire packets in an application
buffer.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Mat,
> > This patch series changes SDU reassembly to link together PDU skbs
> > using a frag list instead of copying all data bytes to a single,
> > linear skb. The reduction in overhead is especially helpful with high
> > speed AMP links.
I have not looked at these at all, but one thing that came up in a
discussion between Johan and myself with OBEX zero-copy was that we like
to also see MSG_MORE support for L2CAP and RFCOMM sockets. Something to
keep in mind ;)
Regards
Marcel
Gustavo -
On Wed, 13 Jul 2011, Mat Martineau wrote:
> This patch series changes SDU reassembly to link together PDU skbs
> using a frag list instead of copying all data bytes to a single,
> linear skb. The reduction in overhead is especially helpful with high
> speed AMP links.
>
>
> Mat Martineau (3):
> Bluetooth: Linearize skbs for use in BNEP, CMTP, HIDP, and RFCOMM
> Bluetooth: Handle fragmented skbs in bt_sock_stream_recvmsg()
> Bluetooth: Perform L2CAP SDU reassembly without copying data
>
> include/net/bluetooth/l2cap.h | 3 +-
> net/bluetooth/af_bluetooth.c | 30 +++++-
> net/bluetooth/bnep/core.c | 5 +-
> net/bluetooth/cmtp/core.c | 5 +-
> net/bluetooth/hidp/core.c | 10 ++-
> net/bluetooth/l2cap_core.c | 239 ++++++++++++++---------------------------
> net/bluetooth/rfcomm/core.c | 5 +-
> 7 files changed, 128 insertions(+), 169 deletions(-)
>
> --
> 1.7.6
Have you had a chance to look at these patches?
Thanks,
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Use sk_buff fragment capabilities to link together incoming skbs
instead of allocating a new skb for reassembly and copying.
The new reassembly code works equally well for ERTM and streaming
mode, so there is now one reassembly function instead of two.
Signed-off-by: Mat Martineau <[email protected]>
---
include/net/bluetooth/l2cap.h | 3 +-
net/bluetooth/l2cap_core.c | 239 ++++++++++++++---------------------------
2 files changed, 80 insertions(+), 162 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 4f34ad2..6fa1140 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -354,8 +354,8 @@ struct l2cap_chan {
__u8 retry_count;
__u8 num_acked;
__u16 sdu_len;
- __u16 partial_sdu_len;
struct sk_buff *sdu;
+ struct sk_buff *sdu_last_frag;
__u8 remote_tx_win;
__u8 remote_max_tx;
@@ -454,7 +454,6 @@ enum {
#define L2CAP_CONF_MAX_CONF_RSP 2
enum {
- CONN_SAR_SDU,
CONN_SREJ_SENT,
CONN_WAIT_F,
CONN_SREJ_ACT,
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 52c791e..e82fff1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3120,102 +3120,104 @@ static int l2cap_add_to_srej_queue(struct l2cap_chan *chan, struct sk_buff *skb,
return 0;
}
-static int l2cap_ertm_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control)
+static void append_skb_frag(struct sk_buff *skb,
+ struct sk_buff *new_frag, struct sk_buff **last_frag)
{
- struct sk_buff *_skb;
- int err;
+ /* skb->len reflects data in skb as well as all fragments
+ * skb->data_len reflects only data in fragments
+ */
+ if (!skb_has_frag_list(skb))
+ skb_shinfo(skb)->frag_list = new_frag;
+
+ new_frag->next = NULL;
+
+ (*last_frag)->next = new_frag;
+ *last_frag = new_frag;
+
+ skb->len += new_frag->len;
+ skb->data_len += new_frag->len;
+ skb->truesize += new_frag->truesize;
+}
+
+static int l2cap_reassemble_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control)
+{
+ int err = -EINVAL;
switch (control & L2CAP_CTRL_SAR) {
case L2CAP_SDU_UNSEGMENTED:
- if (test_bit(CONN_SAR_SDU, &chan->conn_state))
- goto drop;
+ if (chan->sdu)
+ break;
- return chan->ops->recv(chan->data, skb);
+ err = chan->ops->recv(chan->data, skb);
+ break;
case L2CAP_SDU_START:
- if (test_bit(CONN_SAR_SDU, &chan->conn_state))
- goto drop;
+ if (chan->sdu)
+ break;
chan->sdu_len = get_unaligned_le16(skb->data);
+ skb_pull(skb, 2);
- if (chan->sdu_len > chan->imtu)
- goto disconnect;
-
- chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC);
- if (!chan->sdu)
- return -ENOMEM;
+ if (chan->sdu_len > chan->imtu) {
+ err = -EMSGSIZE;
+ break;
+ }
- /* pull sdu_len bytes only after alloc, because of Local Busy
- * condition we have to be sure that this will be executed
- * only once, i.e., when alloc does not fail */
- skb_pull(skb, 2);
+ if (skb->len >= chan->sdu_len)
+ break;
- memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
+ chan->sdu = skb;
+ chan->sdu_last_frag = skb;
- set_bit(CONN_SAR_SDU, &chan->conn_state);
- chan->partial_sdu_len = skb->len;
+ skb = NULL;
+ err = 0;
break;
case L2CAP_SDU_CONTINUE:
- if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
- goto disconnect;
-
if (!chan->sdu)
- goto disconnect;
+ break;
- chan->partial_sdu_len += skb->len;
- if (chan->partial_sdu_len > chan->sdu_len)
- goto drop;
+ append_skb_frag(chan->sdu, skb,
+ &chan->sdu_last_frag);
+ skb = NULL;
- memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
+ if (chan->sdu->len >= chan->sdu_len)
+ break;
+ err = 0;
break;
case L2CAP_SDU_END:
- if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
- goto disconnect;
-
if (!chan->sdu)
- goto disconnect;
-
- chan->partial_sdu_len += skb->len;
-
- if (chan->partial_sdu_len > chan->imtu)
- goto drop;
+ break;
- if (chan->partial_sdu_len != chan->sdu_len)
- goto drop;
+ append_skb_frag(chan->sdu, skb,
+ &chan->sdu_last_frag);
+ skb = NULL;
- memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
+ if (chan->sdu->len != chan->sdu_len)
+ break;
- _skb = skb_clone(chan->sdu, GFP_ATOMIC);
- if (!_skb) {
- return -ENOMEM;
- }
+ err = chan->ops->recv(chan->data, chan->sdu);
- err = chan->ops->recv(chan->data, _skb);
- if (err < 0) {
- kfree_skb(_skb);
- return err;
+ if (!err) {
+ /* Reassembly complete */
+ chan->sdu = NULL;
+ chan->sdu_last_frag = NULL;
+ chan->sdu_len = 0;
}
-
- clear_bit(CONN_SAR_SDU, &chan->conn_state);
-
- kfree_skb(chan->sdu);
break;
}
- kfree_skb(skb);
- return 0;
-
-drop:
- kfree_skb(chan->sdu);
- chan->sdu = NULL;
+ if (err) {
+ kfree_skb(skb);
+ kfree_skb(chan->sdu);
+ chan->sdu = NULL;
+ chan->sdu_last_frag = NULL;
+ chan->sdu_len = 0;
+ }
-disconnect:
- l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
- kfree_skb(skb);
- return 0;
+ return err;
}
static void l2cap_ertm_enter_local_busy(struct l2cap_chan *chan)
@@ -3269,99 +3271,6 @@ void l2cap_chan_busy(struct l2cap_chan *chan, int busy)
}
}
-static int l2cap_streaming_reassembly_sdu(struct l2cap_chan *chan, struct sk_buff *skb, u16 control)
-{
- struct sk_buff *_skb;
- int err = -EINVAL;
-
- /*
- * TODO: We have to notify the userland if some data is lost with the
- * Streaming Mode.
- */
-
- switch (control & L2CAP_CTRL_SAR) {
- case L2CAP_SDU_UNSEGMENTED:
- if (test_bit(CONN_SAR_SDU, &chan->conn_state)) {
- kfree_skb(chan->sdu);
- break;
- }
-
- err = chan->ops->recv(chan->data, skb);
- if (!err)
- return 0;
-
- break;
-
- case L2CAP_SDU_START:
- if (test_bit(CONN_SAR_SDU, &chan->conn_state)) {
- kfree_skb(chan->sdu);
- break;
- }
-
- chan->sdu_len = get_unaligned_le16(skb->data);
- skb_pull(skb, 2);
-
- if (chan->sdu_len > chan->imtu) {
- err = -EMSGSIZE;
- break;
- }
-
- chan->sdu = bt_skb_alloc(chan->sdu_len, GFP_ATOMIC);
- if (!chan->sdu) {
- err = -ENOMEM;
- break;
- }
-
- memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
-
- set_bit(CONN_SAR_SDU, &chan->conn_state);
- chan->partial_sdu_len = skb->len;
- err = 0;
- break;
-
- case L2CAP_SDU_CONTINUE:
- if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
- break;
-
- memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
-
- chan->partial_sdu_len += skb->len;
- if (chan->partial_sdu_len > chan->sdu_len)
- kfree_skb(chan->sdu);
- else
- err = 0;
-
- break;
-
- case L2CAP_SDU_END:
- if (!test_bit(CONN_SAR_SDU, &chan->conn_state))
- break;
-
- memcpy(skb_put(chan->sdu, skb->len), skb->data, skb->len);
-
- clear_bit(CONN_SAR_SDU, &chan->conn_state);
- chan->partial_sdu_len += skb->len;
-
- if (chan->partial_sdu_len > chan->imtu)
- goto drop;
-
- if (chan->partial_sdu_len == chan->sdu_len) {
- _skb = skb_clone(chan->sdu, GFP_ATOMIC);
- err = chan->ops->recv(chan->data, _skb);
- if (err < 0)
- kfree_skb(_skb);
- }
- err = 0;
-
-drop:
- kfree_skb(chan->sdu);
- break;
- }
-
- kfree_skb(skb);
- return err;
-}
-
static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq)
{
struct sk_buff *skb;
@@ -3376,7 +3285,7 @@ static void l2cap_check_srej_gap(struct l2cap_chan *chan, u8 tx_seq)
skb = skb_dequeue(&chan->srej_q);
control = bt_cb(skb)->sar << L2CAP_CTRL_SAR_SHIFT;
- err = l2cap_ertm_reassembly_sdu(chan, skb, control);
+ err = l2cap_reassemble_sdu(chan, skb, control);
if (err < 0) {
l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
@@ -3536,7 +3445,7 @@ expected:
return 0;
}
- err = l2cap_ertm_reassembly_sdu(chan, skb, rx_control);
+ err = l2cap_reassemble_sdu(chan, skb, rx_control);
chan->buffer_seq = (chan->buffer_seq + 1) % 64;
if (err < 0) {
l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
@@ -3854,10 +3763,20 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
if (chan->expected_tx_seq == tx_seq)
chan->expected_tx_seq = (chan->expected_tx_seq + 1) % 64;
- else
+ else {
chan->expected_tx_seq = (tx_seq + 1) % 64;
- l2cap_streaming_reassembly_sdu(chan, skb, control);
+ /* Frame(s) missing - must discard partial SDU */
+ kfree_skb(chan->sdu);
+ chan->sdu = NULL;
+ chan->sdu_last_frag = NULL;
+ chan->sdu_len = 0;
+
+ /* TODO: Notify userland of missing data */
+ }
+
+ if (l2cap_reassemble_sdu(chan, skb, control) == -EMSGSIZE)
+ l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
goto done;
--
1.7.6
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Fragmented skbs are only encountered when receiving ERTM or streaming
mode L2CAP data. BNEP, CMTP, HIDP, and RFCOMM generally use basic
mode, but they need to handle fragments without crashing.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/bnep/core.c | 5 ++++-
net/bluetooth/cmtp/core.c | 5 ++++-
net/bluetooth/hidp/core.c | 10 ++++++++--
net/bluetooth/rfcomm/core.c | 5 ++++-
4 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index ca39fcf..dfadb65 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -490,7 +490,10 @@ static int bnep_session(void *arg)
/* RX */
while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
skb_orphan(skb);
- bnep_rx_frame(s, skb);
+ if (!skb_linearize(skb))
+ bnep_rx_frame(s, skb);
+ else
+ kfree_skb(skb);
}
if (sk->sk_state != BT_CONNECTED)
diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index c5b11af..2487b84 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -300,7 +300,10 @@ static int cmtp_session(void *arg)
while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
skb_orphan(skb);
- cmtp_recv_frame(session, skb);
+ if (!skb_linearize(skb))
+ cmtp_recv_frame(session, skb);
+ else
+ kfree_skb(skb);
}
cmtp_process_transmit(session);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index c405a95..2506509 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -715,12 +715,18 @@ static int hidp_session(void *arg)
while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
skb_orphan(skb);
- hidp_recv_ctrl_frame(session, skb);
+ if (!skb_linearize(skb))
+ hidp_recv_ctrl_frame(session, skb);
+ else
+ kfree_skb(skb);
}
while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
skb_orphan(skb);
- hidp_recv_intr_frame(session, skb);
+ if (!skb_linearize(skb))
+ hidp_recv_intr_frame(session, skb);
+ else
+ kfree_skb(skb);
}
hidp_process_transmit(session);
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 5759bb7..6bbd317 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -1855,7 +1855,10 @@ static inline void rfcomm_process_rx(struct rfcomm_session *s)
/* Get data directly from socket receive queue without copying it. */
while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
skb_orphan(skb);
- rfcomm_recv_frame(s, skb);
+ if (!skb_linearize(skb))
+ rfcomm_recv_frame(s, skb);
+ else
+ kfree_skb(skb);
}
if (sk->sk_state == BT_CLOSED) {
--
1.7.6
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
ERTM reassembly will be more efficient when skbs are linked together
rather than copying every incoming data byte. The existing stream recv
function assumes skbs are linear, so it needs to know how to handle
fragments before reassembly is changed.
bt_sock_recvmsg() already handles fragmented skbs.
Signed-off-by: Mat Martineau <[email protected]>
---
net/bluetooth/af_bluetooth.c | 30 ++++++++++++++++++++++++++++--
1 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 8add9b4..9a43520 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -349,7 +349,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
}
chunk = min_t(unsigned int, skb->len, size);
- if (memcpy_toiovec(msg->msg_iov, skb->data, chunk)) {
+ if (skb_copy_datagram_iovec(skb, 0, msg->msg_iov, chunk)) {
skb_queue_head(&sk->sk_receive_queue, skb);
if (!copied)
copied = -EFAULT;
@@ -361,7 +361,33 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
sock_recv_ts_and_drops(msg, sk, skb);
if (!(flags & MSG_PEEK)) {
- skb_pull(skb, chunk);
+ int skb_len = skb_headlen(skb);
+
+ if (chunk <= skb_len) {
+ __skb_pull(skb, chunk);
+ } else {
+ struct sk_buff *frag;
+
+ __skb_pull(skb, skb_len);
+ chunk -= skb_len;
+
+ skb_walk_frags(skb, frag) {
+ if (chunk <= frag->len) {
+ /* Pulling partial data */
+ skb->len -= chunk;
+ skb->data_len -= chunk;
+ __skb_pull(frag, chunk);
+ break;
+ } else if (frag->len) {
+ /* Pulling all frag data */
+ chunk -= frag->len;
+ skb->len -= frag->len;
+ skb->data_len -= frag->len;
+ __skb_pull(frag, frag->len);
+ }
+ }
+ }
+
if (skb->len) {
skb_queue_head(&sk->sk_receive_queue, skb);
break;
--
1.7.6
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum