Re: [PATCH 2/2] Bluetooth: Fix skb length calculation

[Mat Martineau <mathewm@codeaurora.org>]

Hi Gustavo -

On Wed, 9 May 2012, Gustavo Padovan wrote:

> When we add a fragment to a skb, len, data_len and truesize fields needs
> to be updated.
>
> Signed-off-by: Gustavo Padovan <gustavo@padovan.org>
> ---
> net/bluetooth/l2cap_core.c |    4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 66a1a55..8d7c6ba 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1851,6 +1851,10 @@ static inline int l2cap_skbuff_fromiovec(struct l2cap_chan *chan,
> 		sent += count;
> 		len  -= count;
>
> +		skb->len += (*frag)->len;
> +		skb->data_len += (*frag)->len;
> +		skb->truesize += (*frag)->truesize;
> +
> 		frag = &(*frag)->next;
> 	}
>
> -- 
> 1.7.10.1

Good to hear that MSG_MORE support is in progress!  Is this change 
necessary to support MSG_MORE, or is it only something you noticed 
while working on that feature?


I think this patch breaks SO_SNDBUF accounting, which uses 
skb->truesize in the sock_wfree() destructor.

For outgoing packets, L2CAP and HCI currently use skb fragments in a 
non-standard way.  The &skb_shinfo(skb)->frag_list and skb->next 
pointers are used to group HCI fragments so they are placed in the HCI 
send queue all at once.  But they are *not* intended to represent a 
"normal" fragmented skb.

Once the list of skbs is passed to hci_send_acl(), hci_queue_acl() 
separates all of the linked skbs before being placing them in the HCI 
chan->data_q.  Since the driver sees packets coming out of 
chan->data_q, it only sees unfragmented skbs.  If you change 
skb->truesize on the first HCI fragment, sk->sk_wmem_alloc will be 
adjusted for the bytes used by that first fragment plus the bytes used 
by all of the continuation fragments.  However, all of the later 
continuation fragments will adjust sk->sk_wmem_alloc too!


There are other problems due to the non-standard use of skb fragments 
by HCI and L2CAP.  One is that it is confusing, and makes changes that 
*should* work (like this one) instead cause breakage.

The big problem is that the skb->next pointer is used for both skb 
queuing and skb fragment lists.  On top of that, skb clones share the 
&skb_shinfo(skb)->frag_list pointer - so the continuation fragments 
and their 'next' pointers are also shared between clones!  When 
hci_queue_acl() puts each fragment in the chan->data_q, the skb->next 
pointers of the fragments are overwritten.  If there is a clone of the 
head skb in another queue (like the ERTM tx queue), its fragments get 
corrupted.  This is why ERTM PDUs must fit in a single HCI fragment, 
so that no fragment lists are included in the ERTM tx queue.  Some 
devices (especially USB) have very short HCI MTUs, which makes ERTM 
much less efficient on those devices.


I see these options:

  * Add comments to explain non-standard use of skb fragments

  * Keep your change above, but also modify hci_queue_acl() to rewrite 
the len, data_len, and truesize values of the head skb before queueing 
it.

  * Try to find a different way to represent HCI fragments that fits 
with standard skb usage and interacts better with ERTM tx queues.

What sounds good to you?

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum