Messages in this thread | | | From | linmiaohe <> | Subject | Re: [PATCH] net: handle the return value of pskb_carve_frag_list() correctly | Date | Mon, 17 Aug 2020 11:58:05 +0000 |
| |
David Miller <davem@davemloft.net> wrote: >> David Miller <davem@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. :)
| |