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

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

[Miaohe Lin]

pskb_carve_frag_list() may return -ENOMEM in pskb_carve_inside_nonlinear().
we should handle this correctly or we would get wrong sk_buff.

Fixes: 6fa01ccd8830 ("skbuff: Add pskb_extract() helper function")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 net/core/skbuff.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index bca0d3c7f114..afbc1a79dc8a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5987,9 +5987,13 @@ static int pskb_carve_inside_nonlinear(struct sk_buff *skb, const u32 off,
 	if (skb_has_frag_list(skb))
 		skb_clone_fraglist(skb);
 
-	if (k == 0) {
-		/* split line is in frag list */
-		pskb_carve_frag_list(skb, shinfo, off - pos, gfp_mask);
+	/* 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;
 	}
 	skb_release_data(skb);
 
-- 
2.19.1

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

[David Miller]

From: Miaohe Lin <linmiaohe@huawei.com>
Date: Sat, 15 Aug 2020 04:46:41 -0400

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

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

[David Miller]

From: Miaohe Lin <linmiaohe@huawei.com>
Date: Sat, 15 Aug 2020 04:46:41 -0400

> pskb_carve_frag_list() may return -ENOMEM in pskb_carve_inside_nonlinear().
> we should handle this correctly or we would get wrong sk_buff.
> 
> Fixes: 6fa01ccd8830 ("skbuff: Add pskb_extract() helper function")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Applied, thanks.

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

[linmiaohe]

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.

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

[David Miller]

From: linmiaohe <linmiaohe@huawei.com>
Date: Mon, 17 Aug 2020 02:27:23 +0000

> 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?

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

[linmiaohe]

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. :)