[net-next] net: keep original skb which only needs header checking during software GSO
diff mbox series

Message ID 1411108686-20602-1-git-send-email-jasowang@redhat.com
State New, archived
Headers show
Series
  • [net-next] net: keep original skb which only needs header checking during software GSO
Related show

Commit Message

Jason Wang Sept. 19, 2014, 6:38 a.m. UTC
Commit ce93718fb7cdbc064c3000ff59e4d3200bdfa744 ("net: Don't keep
around original SKB when we software segment GSO frames") frees the
original skb after software GSO even for dodgy gso skbs. This breaks
the stream throughput from untrusted sources, since only header
checking was done during software GSO instead of a true
segmentation. This patch fixes this by freeing the original gso skb
only when it was really segmented by software.

Fixes ce93718fb7cdbc064c3000ff59e4d3200bdfa744 ("net: Don't keep
around original SKB when we software segment GSO frames.")

CC: David S. Miller <davem@davemloft.net>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/core/dev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Eric Dumazet Sept. 19, 2014, 7:04 a.m. UTC | #1
On Fri, 2014-09-19 at 14:38 +0800, Jason Wang wrote:
> Commit ce93718fb7cdbc064c3000ff59e4d3200bdfa744 ("net: Don't keep
> around original SKB when we software segment GSO frames") frees the
> original skb after software GSO even for dodgy gso skbs. This breaks
> the stream throughput from untrusted sources, since only header
> checking was done during software GSO instead of a true
> segmentation. This patch fixes this by freeing the original gso skb
> only when it was really segmented by software.
> 
> Fixes ce93718fb7cdbc064c3000ff59e4d3200bdfa744 ("net: Don't keep
> around original SKB when we software segment GSO frames.")
> 
> CC: David S. Miller <davem@davemloft.net>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/core/dev.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e916ba8..b7a0e1d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2694,10 +2694,12 @@ struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
>  		struct sk_buff *segs;
>  
>  		segs = skb_gso_segment(skb, features);
> -		kfree_skb(skb);
>  		if (IS_ERR(segs))
>  			
> -		skb = segs;
> +		else if (segs) {
> +			kfree_skb(skb);
> +			skb = segs;
> +		}
>  	} else {
>  		if (skb_needs_linearize(skb, features) &&
>  		    __skb_linearize(skb))

Good catch !

While we are at it, could you use consume_skb() instead of kfree_skb(),
and add missing {} (CodingStyle) ?

if (IS_ERR(segs)) {
	segs = NULL;
} else if (segs) {
	consume_skb(skb);
	skb = segs;
}


Thanks !


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Jason Wang Sept. 19, 2014, 7:49 a.m. UTC | #2
On 09/19/2014 03:04 PM, Eric Dumazet wrote:
> On Fri, 2014-09-19 at 14:38 +0800, Jason Wang wrote:
>> Commit ce93718fb7cdbc064c3000ff59e4d3200bdfa744 ("net: Don't keep
>> around original SKB when we software segment GSO frames") frees the
>> original skb after software GSO even for dodgy gso skbs. This breaks
>> the stream throughput from untrusted sources, since only header
>> checking was done during software GSO instead of a true
>> segmentation. This patch fixes this by freeing the original gso skb
>> only when it was really segmented by software.
>>
>> Fixes ce93718fb7cdbc064c3000ff59e4d3200bdfa744 ("net: Don't keep
>> around original SKB when we software segment GSO frames.")
>>
>> CC: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>  net/core/dev.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index e916ba8..b7a0e1d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2694,10 +2694,12 @@ struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
>>  		struct sk_buff *segs;
>>  
>>  		segs = skb_gso_segment(skb, features);
>> -		kfree_skb(skb);
>>  		if (IS_ERR(segs))
>>  			
>> -		skb = segs;
>> +		else if (segs) {
>> +			kfree_skb(skb);
>> +			skb = segs;
>> +		}
>>  	} else {
>>  		if (skb_needs_linearize(skb, features) &&
>>  		    __skb_linearize(skb))
> Good catch !
>
> While we are at it, could you use consume_skb() instead of kfree_skb(),
> and add missing {} (CodingStyle) ?
>
> if (IS_ERR(segs)) {
> 	segs = NULL;
> } else if (segs) {
> 	consume_skb(skb);
> 	skb = segs;
> }
>
>
> Thanks !
>
>

Yes, it's better. Will do them in V2.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch
diff mbox series

diff --git a/net/core/dev.c b/net/core/dev.c
index e916ba8..b7a0e1d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2694,10 +2694,12 @@  struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
 		struct sk_buff *segs;
 
 		segs = skb_gso_segment(skb, features);
-		kfree_skb(skb);
 		if (IS_ERR(segs))
 			segs = NULL;
-		skb = segs;
+		else if (segs) {
+			kfree_skb(skb);
+			skb = segs;
+		}
 	} else {
 		if (skb_needs_linearize(skb, features) &&
 		    __skb_linearize(skb))