diff mbox

[net-next] net: Check frag_lists first to prevent data out of order

Message ID 1440636965-11552-1-git-send-email-chenweilong@huawei.com
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

chenweilong Aug. 27, 2015, 12:56 a.m. UTC 
From: Weilong Chen <chenweilong@huawei.com>

When try to merge several skbs to prior one, if the frag_list is
used and the the last one is a small packet, once the condition
"len <= skb_tailroom(to)" is satisfied, we will get a wrong
packet!
This patch just check frag_lists before the condtion to prevent
this from happening.

Signed-off-by: Weilong Chen <chenweilong@huawei.com>
---
 net/core/skbuff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Eric Dumazet Aug. 27, 2015, 2:12 a.m. UTC | #1 
On Thu, 2015-08-27 at 08:56 +0800, chenweilong@huawei.com wrote:
> From: Weilong Chen <chenweilong@huawei.com>
> 
> When try to merge several skbs to prior one, if the frag_list is
> used and the the last one is a small packet, once the condition
> "len <= skb_tailroom(to)" is satisfied, we will get a wrong
> packet!
> This patch just check frag_lists before the condtion to prevent
> this from happening.
> 
> Signed-off-by: Weilong Chen <chenweilong@huawei.com>
> ---
>  net/core/skbuff.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 8a725cc..d08edcb 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4133,6 +4133,9 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>  	if (skb_cloned(to))
>  		return false;
>  
> +	if (skb_has_frag_list(to) || skb_has_frag_list(from))
> +		return false;
> +
>  	if (len <= skb_tailroom(to)) {
>  		if (len)
>  			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
> @@ -4140,9 +4143,6 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>  		return true;
>  	}
>  
> -	if (skb_has_frag_list(to) || skb_has_frag_list(from))
> -		return false;
> -
>  	if (skb_headlen(from) != 0) {
>  		struct page *page;
>  		unsigned int offset;

Sigh.

No idea what problem you tried to solve.

This patch is not needed.

If (len <= skb_tailroom()), then it is obviously correct to copy_bits()
the bytes.

Hints :

- If @to has a fraglist, then skb_tailroom(to) is 0 so the copy can not
be done.

- If @from has a fraglist, it is not relevant as we copy it into @to and
will free @from.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Aug. 27, 2015, 2:23 a.m. UTC | #2 
On Wed, 2015-08-26 at 19:12 -0700, Eric Dumazet wrote:
> On Thu, 2015-08-27 at 08:56 +0800, chenweilong@huawei.com wrote:
> > From: Weilong Chen <chenweilong@huawei.com>
> > 
> > When try to merge several skbs to prior one, if the frag_list is
> > used and the the last one is a small packet, once the condition
> > "len <= skb_tailroom(to)" is satisfied, we will get a wrong
> > packet!
> > This patch just check frag_lists before the condtion to prevent
> > this from happening.
> > 
> > Signed-off-by: Weilong Chen <chenweilong@huawei.com>
> > ---
> >  net/core/skbuff.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 8a725cc..d08edcb 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -4133,6 +4133,9 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
> >  	if (skb_cloned(to))
> >  		return false;
> >  
> > +	if (skb_has_frag_list(to) || skb_has_frag_list(from))
> > +		return false;
> > +
> >  	if (len <= skb_tailroom(to)) {
> >  		if (len)
> >  			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
> > @@ -4140,9 +4143,6 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
> >  		return true;
> >  	}
> >  
> > -	if (skb_has_frag_list(to) || skb_has_frag_list(from))
> > -		return false;
> > -
> >  	if (skb_headlen(from) != 0) {
> >  		struct page *page;
> >  		unsigned int offset;
> 
> Sigh.
> 
> No idea what problem you tried to solve.
> 
> This patch is not needed.
> 
> If (len <= skb_tailroom()), then it is obviously correct to copy_bits()
> the bytes.
> 
> Hints :
> 
> - If @to has a fraglist, then skb_tailroom(to) is 0 so the copy can not
> be done.
> 
> - If @from has a fraglist, it is not relevant as we copy it into @to and
> will free @from.

This is going to be a FAQ 

http://www.spinics.net/lists/netdev/msg315090.html




--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
chenweilong Aug. 28, 2015, 3:35 a.m. UTC | #3 
Thanks for reply.

> On Wed, 2015-08-26 at 19:12 -0700, Eric Dumazet wrote:
>> On Thu, 2015-08-27 at 08:56 +0800, chenweilong@huawei.com wrote:
>>> From: Weilong Chen <chenweilong@huawei.com>
>>>
>>> When try to merge several skbs to prior one, if the frag_list is
>>> used and the the last one is a small packet, once the condition
>>> "len <= skb_tailroom(to)" is satisfied, we will get a wrong
>>> packet!
>>> This patch just check frag_lists before the condtion to prevent
>>> this from happening.
>>>
>>> Signed-off-by: Weilong Chen <chenweilong@huawei.com>
>>> ---
>>>   net/core/skbuff.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index 8a725cc..d08edcb 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -4133,6 +4133,9 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>>>   	if (skb_cloned(to))
>>>   		return false;
>>>
>>> +	if (skb_has_frag_list(to) || skb_has_frag_list(from))
>>> +		return false;
>>> +
>>>   	if (len <= skb_tailroom(to)) {
>>>   		if (len)
>>>   			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
>>> @@ -4140,9 +4143,6 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>>>   		return true;
>>>   	}
>>>
>>> -	if (skb_has_frag_list(to) || skb_has_frag_list(from))
>>> -		return false;
>>> -
>>>   	if (skb_headlen(from) != 0) {
>>>   		struct page *page;
>>>   		unsigned int offset;
>>
>> Sigh.
>>
>> No idea what problem you tried to solve.
>>
>> This patch is not needed.
>>
>> If (len <= skb_tailroom()), then it is obviously correct to copy_bits()
>> the bytes.
>>
>> Hints :
>>
>> - If @to has a fraglist, then skb_tailroom(to) is 0 so the copy can not
>> be done.

How to make sure it?
In my test, @to has a fraglist, but skb_tailroom(to) is not 0!
The test is about tipc, the function tipc_buf_append will merge 3 skbs 
to one:
packet 1: len = 1420 skb_tailroom = 190
packet 2: len = 1420
	packet 2 will be add to 1' fraglist, but not update skb_tailroom
packet 3: len = 60
	Here's the error!

>>
>> - If @from has a fraglist, it is not relevant as we copy it into @to and
>> will free @from.
>
> This is going to be a FAQ
>
> http://www.spinics.net/lists/netdev/msg315090.html
>
>
>
>
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 28, 2015, 4:26 a.m. UTC | #4 
From: Weilong Chen <chenweilong@huawei.com>
Date: Fri, 28 Aug 2015 11:35:40 +0800

> In my test, @to has a fraglist, but skb_tailroom(to) is not 0!
> The test is about tipc, the function tipc_buf_append will merge 3 skbs
> to one:
> packet 1: len = 1420 skb_tailroom = 190
> packet 2: len = 1420
> 	packet 2 will be add to 1' fraglist, but not update skb_tailroom

This operation is illegal.

Once there are frags, tailroom is not permitted.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Aug. 28, 2015, 4:37 a.m. UTC | #5 
On Fri, 2015-08-28 at 11:35 +0800, Weilong Chen wrote:
> Thanks for reply.
> 
> > On Wed, 2015-08-26 at 19:12 -0700, Eric Dumazet wrote:
> >> On Thu, 2015-08-27 at 08:56 +0800, chenweilong@huawei.com wrote:
> >>> From: Weilong Chen <chenweilong@huawei.com>
> >>>
> >>> When try to merge several skbs to prior one, if the frag_list is
> >>> used and the the last one is a small packet, once the condition
> >>> "len <= skb_tailroom(to)" is satisfied, we will get a wrong
> >>> packet!
> >>> This patch just check frag_lists before the condtion to prevent
> >>> this from happening.
> >>>
> >>> Signed-off-by: Weilong Chen <chenweilong@huawei.com>
> >>> ---
> >>>   net/core/skbuff.c | 6 +++---
> >>>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >>> index 8a725cc..d08edcb 100644
> >>> --- a/net/core/skbuff.c
> >>> +++ b/net/core/skbuff.c
> >>> @@ -4133,6 +4133,9 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
> >>>   	if (skb_cloned(to))
> >>>   		return false;
> >>>
> >>> +	if (skb_has_frag_list(to) || skb_has_frag_list(from))
> >>> +		return false;
> >>> +
> >>>   	if (len <= skb_tailroom(to)) {
> >>>   		if (len)
> >>>   			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
> >>> @@ -4140,9 +4143,6 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
> >>>   		return true;
> >>>   	}
> >>>
> >>> -	if (skb_has_frag_list(to) || skb_has_frag_list(from))
> >>> -		return false;
> >>> -
> >>>   	if (skb_headlen(from) != 0) {
> >>>   		struct page *page;
> >>>   		unsigned int offset;
> >>
> >> Sigh.
> >>
> >> No idea what problem you tried to solve.
> >>
> >> This patch is not needed.
> >>
> >> If (len <= skb_tailroom()), then it is obviously correct to copy_bits()
> >> the bytes.
> >>
> >> Hints :
> >>
> >> - If @to has a fraglist, then skb_tailroom(to) is 0 so the copy can not
> >> be done.
> 
> How to make sure it?
> In my test, @to has a fraglist, but skb_tailroom(to) is not 0!
> The test is about tipc, the function tipc_buf_append will merge 3 skbs 
> to one:
> packet 1: len = 1420 skb_tailroom = 190
> packet 2: len = 1420
> 	packet 2 will be add to 1' fraglist, but not update skb_tailroom
> packet 3: len = 60
> 	Here's the error!

Then fix tipc, or make sure you read and understood the code.

By definition, if one skb has something in fraglist, it is non linear.

If you read skb_tailroom(), you'll see this function returns 0 if skb is
non linear.

include/linux/skbuff.h-/**
include/linux/skbuff.h: *       skb_tailroom - bytes at buffer end
include/linux/skbuff.h- *       @skb: buffer to check
include/linux/skbuff.h- *
include/linux/skbuff.h- *       Return the number of bytes of free space at the tail of an sk_buff
include/linux/skbuff.h- */
include/linux/skbuff.h:static inline int skb_tailroom(const struct sk_buff *skb)
include/linux/skbuff.h-{
include/linux/skbuff.h- return skb_is_nonlinear(skb) ? 0 : skb->end - skb->tail;
include/linux/skbuff.h-}



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
chenweilong Aug. 28, 2015, 5:33 a.m. UTC | #6 
在 2015/8/28 12:37, Eric Dumazet 写道:
> On Fri, 2015-08-28 at 11:35 +0800, Weilong Chen wrote:
>> Thanks for reply.
>>
>>> On Wed, 2015-08-26 at 19:12 -0700, Eric Dumazet wrote:
>>>> On Thu, 2015-08-27 at 08:56 +0800, chenweilong@huawei.com wrote:
>>>>> From: Weilong Chen <chenweilong@huawei.com>
>>>>>
>>>>> When try to merge several skbs to prior one, if the frag_list is
>>>>> used and the the last one is a small packet, once the condition
>>>>> "len <= skb_tailroom(to)" is satisfied, we will get a wrong
>>>>> packet!
>>>>> This patch just check frag_lists before the condtion to prevent
>>>>> this from happening.
>>>>>
>>>>> Signed-off-by: Weilong Chen <chenweilong@huawei.com>
>>>>> ---
>>>>>    net/core/skbuff.c | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>>> index 8a725cc..d08edcb 100644
>>>>> --- a/net/core/skbuff.c
>>>>> +++ b/net/core/skbuff.c
>>>>> @@ -4133,6 +4133,9 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>>>>>    	if (skb_cloned(to))
>>>>>    		return false;
>>>>>
>>>>> +	if (skb_has_frag_list(to) || skb_has_frag_list(from))
>>>>> +		return false;
>>>>> +
>>>>>    	if (len <= skb_tailroom(to)) {
>>>>>    		if (len)
>>>>>    			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
>>>>> @@ -4140,9 +4143,6 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
>>>>>    		return true;
>>>>>    	}
>>>>>
>>>>> -	if (skb_has_frag_list(to) || skb_has_frag_list(from))
>>>>> -		return false;
>>>>> -
>>>>>    	if (skb_headlen(from) != 0) {
>>>>>    		struct page *page;
>>>>>    		unsigned int offset;
>>>>
>>>> Sigh.
>>>>
>>>> No idea what problem you tried to solve.
>>>>
>>>> This patch is not needed.
>>>>
>>>> If (len <= skb_tailroom()), then it is obviously correct to copy_bits()
>>>> the bytes.
>>>>
>>>> Hints :
>>>>
>>>> - If @to has a fraglist, then skb_tailroom(to) is 0 so the copy can not
>>>> be done.
>>
>> How to make sure it?
>> In my test, @to has a fraglist, but skb_tailroom(to) is not 0!
>> The test is about tipc, the function tipc_buf_append will merge 3 skbs
>> to one:
>> packet 1: len = 1420 skb_tailroom = 190
>> packet 2: len = 1420
>> 	packet 2 will be add to 1' fraglist, but not update skb_tailroom
>> packet 3: len = 60
>> 	Here's the error!
>
> Then fix tipc, or make sure you read and understood the code.
>
> By definition, if one skb has something in fraglist, it is non linear.
>
> If you read skb_tailroom(), you'll see this function returns 0 if skb is
> non linear.

Yes you'er right, when packet 2 is add to 1' fraglist, tipc should

update the packet 1' 'len' and 'data_len'. In my kernel(v3.10), it

doesn't do that, which was corrected in commit 5074a (v3.16).

Thanks for your patience

>
> include/linux/skbuff.h-/**
> include/linux/skbuff.h: *       skb_tailroom - bytes at buffer end
> include/linux/skbuff.h- *       @skb: buffer to check
> include/linux/skbuff.h- *
> include/linux/skbuff.h- *       Return the number of bytes of free space at the tail of an sk_buff
> include/linux/skbuff.h- */
> include/linux/skbuff.h:static inline int skb_tailroom(const struct sk_buff *skb)
> include/linux/skbuff.h-{
> include/linux/skbuff.h- return skb_is_nonlinear(skb) ? 0 : skb->end - skb->tail;
> include/linux/skbuff.h-}
>
>
>
>
> .
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
chenweilong Aug. 28, 2015, 5:34 a.m. UTC | #7 
> From: Weilong Chen <chenweilong@huawei.com>
> Date: Fri, 28 Aug 2015 11:35:40 +0800
>
>> In my test, @to has a fraglist, but skb_tailroom(to) is not 0!
>> The test is about tipc, the function tipc_buf_append will merge 3 skbs
>> to one:
>> packet 1: len = 1420 skb_tailroom = 190
>> packet 2: len = 1420
>> 	packet 2 will be add to 1' fraglist, but not update skb_tailroom
>
> This operation is illegal.
>
> Once there are frags, tailroom is not permitted.
>
> .
>

Agree

Thanks

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 8a725cc..d08edcb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4133,6 +4133,9 @@  bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 	if (skb_cloned(to))
 		return false;
 
+	if (skb_has_frag_list(to) || skb_has_frag_list(from))
+		return false;
+
 	if (len <= skb_tailroom(to)) {
 		if (len)
 			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
@@ -4140,9 +4143,6 @@  bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 		return true;
 	}
 
-	if (skb_has_frag_list(to) || skb_has_frag_list(from))
-		return false;
-
 	if (skb_headlen(from) != 0) {
 		struct page *page;
 		unsigned int offset;