Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH] net: handle the return value of pskb_carve_frag_list() correctly
@ 2020-08-17 11:58 linmiaohe
  0 siblings, 0 replies; 6+ messages in thread
From: linmiaohe @ 2020-08-17 11:58 UTC (permalink / raw)
  To: David Miller
  Cc: kuba, martin.varghese, fw, pshelar, dcaratti, edumazet,
	steffen.klassert, pabeni, shmulik, kyk.segfault,
	sowmini.varadhan, netdev, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] net: handle the return value of pskb_carve_frag_list() correctly
@ 2020-08-17  2:27 linmiaohe
  2020-08-17  4:01 ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: linmiaohe @ 2020-08-17  2:27 UTC (permalink / raw)
  To: David Miller
  Cc: kuba, martin.varghese, fw, pshelar, dcaratti, edumazet,
	steffen.klassert, pabeni, shmulik, kyk.segfault,
	sowmini.varadhan, netdev, linux-kernel

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.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] net: handle the return value of pskb_carve_frag_list() correctly
@ 2020-08-15  8:46 Miaohe Lin
  2020-08-16 23:01 ` David Miller
  2020-08-18 22:57 ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Miaohe Lin @ 2020-08-15  8:46 UTC (permalink / raw)
  To: davem, kuba, martin.varghese, fw, pshelar, dcaratti, edumazet,
	steffen.klassert, pabeni, shmulik, kyk.segfault,
	sowmini.varadhan
  Cc: netdev, linux-kernel, linmiaohe

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-08-18 22:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-17 11:58 [PATCH] net: handle the return value of pskb_carve_frag_list() correctly linmiaohe
  -- strict thread matches above, loose matches on Subject: below --
2020-08-17  2:27 linmiaohe
2020-08-17  4:01 ` David Miller
2020-08-15  8:46 Miaohe Lin
2020-08-16 23:01 ` David Miller
2020-08-18 22:57 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).