Re: [PATCH net-next] net: take care of cloned skbs in tcp_try_coalesce()

[Alexander Duyck <alexander.duyck@gmail.com>]

On 5/2/2012 8:14 PM, Eric Dumazet wrote:
> On Wed, 2012-05-02 at 20:00 -0700, Alexander Duyck wrote:
>> This is exactly my point.  The way your current code works the check in
>> pskb_expand_head will not detect the header is cloned.
>>
>> So for example lets say you have one of your skbs that goes through and
>> is merged.
>>
>> 1.  You start with a cloned skb.  dataref = 2, head_frag page count = 1;
>> 2.  You go through tcp_try_coalesce.  dataref = 2, head_frag page count = 2;

4592         if (from->head_frag) {
4593                 struct page *page;
4594                 unsigned int offset;
4595
4596                 if (skb_shinfo(to)->nr_frags + skb_shinfo(from)->nr_frags>= MAX_SKB_FRAGS)
4597                         return false;
4598                 page = virt_to_head_page(from->head);
4599                 offset = from->data - (unsigned char *)page_address(page);
4600                 skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
4601                                    page, offset, skb_headlen(from));
4602
4603                 if (skb_cloned(from))
4604                         get_page(page);
4605                 else
4606                         *fragstolen = true;
4607
4608                 delta = len; /* we dont know real truesize... */
4609                 goto copyfrags;
4610         }


>> 3.  You call __kfree_skb on the skb.  dataref = 1, head_frag page count = 2;
4614 static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)
4615 {
4616         if (head_stolen)
4617                 kmem_cache_free(skbuff_head_cache, skb);
4618         else
4619                 __kfree_skb(skb);
4620 }
4621

>
> If page count was incremented in 2., this is because we stole the head.
>
> But we could not stole the head since dataref = 2
I don't see how that is the case.  It looks pretty clear to me that if 
dataref ==2, as represented by skb_cloned(from) being true above, we are 
calling get_page which will bump the page count and we are still 
stealing the head and then calling __kfree_skb which will decrement dataref.
> So try to find a real example, based on current state, not on the first
> patch I sent, since we made progress since this time.
>
> Thanks

I included a couple of grabs from blobs on git.kernel.org for Dave's 
current tree.  I should have patches out in about 10 minutes and then we 
can discuss those.

Thanks,

Alex