Re: [PATCH] net: handle the return value of pskb_carve_frag_list() correctly

David Miller [off-list ref] wrote:

quoted

David Miller [off-list ref] wrote:

quoted

quoted

+	/* 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. :)