David Miller <da...@davemloft.net> wrote:
>> David Miller <da...@davemloft.net> wrote:
>>>> + /* split line is in frag list */
>>>> + if (k == 0 && pskb_carve_frag_list(skb, shinfo, off - pos, gfp_mask)) {
>>>> + /* skb_frag_unref() is not needed here as shinfo->nr_frags = 0.
>>>> */
>>>> + if (skb_has_frag_list(skb))
>>>> + kfree_skb_list(skb_shinfo(skb)->frag_list);
>>>> + kfree(data);
>>>> + return -ENOMEM;
>>>
>>>On error, the caller is going to kfree_skb(skb) which will take care of the
>>>frag list.
>>>
>>
>> I'am sorry for my careless. The caller will take care of the frag list and
>> kfree(data) is enough here.
>> Many thanks for review, will send v2 soon.
>
>Actually, reading this again, what about the skb_clone_fraglist() done a few
>lines up? Who will release that reference to the fraglist items?
>
>Maybe the kfree_skb_list() is necessary after all?
Yep, it looks really confusing here. On error, the caller calls kfree_skb(skb)
but only atomic_sub the skb_shared_info->dataref indeed because skb is cloned
here and it shares the fraglist with origin skbuff. But the
skb_clone_fraglist() done a few lines up hold the extra reference to the
fraglist for coming new skb->data.
As there is no new skb->data anymore, that reference to the fraglist items
won't be release unless we take care of it here.
It seems this patch exactly do the right things already. :)
