BUG: TCPDUMP invalid cksum persists after disabling TCP cksum offload

BUG: TCPDUMP invalid cksum persists after disabling TCP cksum offload

[Jamie Gloudon]

Hello,
   I am seeing that tx checksum offload appears to be still running after disabling the feature with ethtool. I'm using kernel 3.6.0-rc6 and the latest ethtool from the git repo. 
 
The default settings on my e1000e NIC:
# ethtool -k eth1 | grep ': on'
 rx-checksumming: on
 tx-checksumming: on
        tx-checksum-ip-generic: on
 scatter-gather: on
         tx-scatter-gather: on
 tcp-segmentation-offload: on
         tx-tcp-segmentation: on
         tx-tcp6-segmentation: on
 generic-segmentation-offload: on
 generic-receive-offload: on
 rx-vlan-offload: on
 tx-vlan-offload: on
 receive-hashing: on
 highdma: on [fixed]
 rx-vlan-filter: on [fixed]
 tx-nocache-copy: on
 
The results after disabling tcp cksum offload feature:
# ethtool -K eth1 tx off
Actual changes:
 tx-checksumming: off
         tx-checksum-ip-generic: off
 scatter-gather: off
         tx-scatter-gather: off [requested on]
 tcp-segmentation-offload: off
         tx-tcp-segmentation: off [requested on]
         tx-tcp6-segmentation: off [requested on]
 generic-segmentation-offload: off [requested on]
 
However, in tcpdump, I'm still observing incorrect tcp checksum:
14:44:38.838711 IP (tos 0x10, ttl 64, id 45798, offset 0, flags [DF], proto TCP
(6), length 60)
     1.1.1.2.59748 > 1.1.1.1.23: Flags [S], cksum 0x0433 (incorrect -> 0x4137), seq 318222122, win 14600, options [mss 1460,sackOK,TS val 5447116 ecr 0,nop,wscale 7], length 0
 
Is this behaviour valid? I'm quite baffled.

Regards,
Jamie Gloudon

Re: BUG: TCPDUMP invalid cksum persists after disabling TCP cksum offload

[Vijay Subramanian]

> The results after disabling tcp cksum offload feature:
> # ethtool -K eth1 tx off

Instead of this, can you try
# ethtool -K eth1 tso off


Vijay

Re: BUG: TCPDUMP invalid cksum persists after disabling TCP cksum offload

[Rick Jones]

On 09/18/2012 02:46 PM, Vijay Subramanian wrote:
>> The results after disabling tcp cksum offload feature:
>> # ethtool -K eth1 tx off
>
> Instead of this, can you try
> # ethtool -K eth1 tso off

Doesn't TSO depend on TX CKO being enabled?  That is, if TX CKO were 
disabled, shouldn't TSO have been implicitly disabled at the same time?

rick jones

Re: BUG: TCPDUMP invalid cksum persists after disabling TCP cksum offload

[Jamie Gloudon]

Rick is correct. Disabling TX CKO also disable TSO. You can see that in
my ethtool output.

On Tue, Sep 18, 2012 at 02:54:38PM -0700, Rick Jones wrote:
> On 09/18/2012 02:46 PM, Vijay Subramanian wrote:
> >>The results after disabling tcp cksum offload feature:
> >># ethtool -K eth1 tx off
> >
> >Instead of this, can you try
> ># ethtool -K eth1 tso off
> 
> Doesn't TSO depend on TX CKO being enabled?  That is, if TX CKO were
> disabled, shouldn't TSO have been implicitly disabled at the same time?
> 
> rick jones

Re: BUG: TCPDUMP invalid cksum persists after disabling TCP cksum offload

[Eric Dumazet]

On Tue, 2012-09-18 at 17:14 -0400, Jamie Gloudon wrote:
> Hello,
>    I am seeing that tx checksum offload appears to be still running after disabling the feature with ethtool. I'm using kernel 3.6.0-rc6 and the latest ethtool from the git repo. 
>  
> The default settings on my e1000e NIC:
> # ethtool -k eth1 | grep ': on'
>  rx-checksumming: on
>  tx-checksumming: on
>         tx-checksum-ip-generic: on
>  scatter-gather: on
>          tx-scatter-gather: on
>  tcp-segmentation-offload: on
>          tx-tcp-segmentation: on
>          tx-tcp6-segmentation: on
>  generic-segmentation-offload: on
>  generic-receive-offload: on
>  rx-vlan-offload: on
>  tx-vlan-offload: on
>  receive-hashing: on
>  highdma: on [fixed]
>  rx-vlan-filter: on [fixed]
>  tx-nocache-copy: on
>  
> The results after disabling tcp cksum offload feature:
> # ethtool -K eth1 tx off
> Actual changes:
>  tx-checksumming: off
>          tx-checksum-ip-generic: off
>  scatter-gather: off
>          tx-scatter-gather: off [requested on]
>  tcp-segmentation-offload: off
>          tx-tcp-segmentation: off [requested on]
>          tx-tcp6-segmentation: off [requested on]
>  generic-segmentation-offload: off [requested on]
>  
> However, in tcpdump, I'm still observing incorrect tcp checksum:
> 14:44:38.838711 IP (tos 0x10, ttl 64, id 45798, offset 0, flags [DF], proto TCP
> (6), length 60)
>      1.1.1.2.59748 > 1.1.1.1.23: Flags [S], cksum 0x0433 (incorrect -> 0x4137), seq 318222122, win 14600, options [mss 1460,sackOK,TS val 5447116 ecr 0,nop,wscale 7], length 0
>  
> Is this behaviour valid? I'm quite baffled.

Thats because dev_hard_start_xmit() calls dev_queue_xmit_nit() before
doing the features tests : 

tcpdump gets a copy of the packet before all mangling done
(skb_checksum_help() in your case)

                if (!list_empty(&ptype_all))
                        dev_queue_xmit_nit(skb, dev);

                features = netif_skb_features(skb);

                if (vlan_tx_tag_present(skb) &&
                    !(features & NETIF_F_HW_VLAN_TX)) {
                        skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb));
                        if (unlikely(!skb))
                                goto out;

                        skb->vlan_tci = 0;
                }

                if (netif_needs_gso(skb, features)) {
                        if (unlikely(dev_gso_segment(skb, features)))
                                goto out_kfree_skb;
                        if (skb->next)
                                goto gso;
                } else {
                        if (skb_needs_linearize(skb, features) &&
                            __skb_linearize(skb))
                                goto out_kfree_skb;

                        /* If packet is not checksummed and device does not
                         * support checksumming for this protocol, complete
                         * checksumming here.
                         */
                        if (skb->ip_summed == CHECKSUM_PARTIAL) {
                                skb_set_transport_header(skb,
                                        skb_checksum_start_offset(skb));
                                if (!(features & NETIF_F_ALL_CSUM) &&
                                     skb_checksum_help(skb))
                                        goto out_kfree_skb;
                        }
                }

                skb_len = skb->len;
                rc = ops->ndo_start_xmit(skb, dev);


I guess we could move dev_queue_xmit_nit(skb, dev) calls right before the
ndo_start_xmit() calls...

[PATCH net-next] net: more accurate network taps in transmit path

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

dev_queue_xmit_nit() should be called right before ndo_start_xmit()
calls or we might give wrong packet contents to taps users :

Packet checksum can be changed, or packet can be linearized or
segmented, and segments partially sent for the later case.

Also a memory allocation can fail and packet never really hit the
driver entry point.

Reported-by: Jamie Gloudon <jamie.gloudon@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/dev.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index dcc673d..52cd1d7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2213,9 +2213,6 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(skb);
 
-		if (!list_empty(&ptype_all))
-			dev_queue_xmit_nit(skb, dev);
-
 		features = netif_skb_features(skb);
 
 		if (vlan_tx_tag_present(skb) &&
@@ -2250,6 +2247,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 			}
 		}
 
+		if (!list_empty(&ptype_all))
+			dev_queue_xmit_nit(skb, dev);
+
 		skb_len = skb->len;
 		rc = ops->ndo_start_xmit(skb, dev);
 		trace_net_dev_xmit(skb, rc, dev, skb_len);
@@ -2272,6 +2272,9 @@ gso:
 		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
 			skb_dst_drop(nskb);
 
+		if (!list_empty(&ptype_all))
+			dev_queue_xmit_nit(nskb, dev);
+
 		skb_len = nskb->len;
 		rc = ops->ndo_start_xmit(nskb, dev);
 		trace_net_dev_xmit(nskb, rc, dev, skb_len);

Re: [PATCH net-next] net: more accurate network taps in transmit path

[Jamie Gloudon]

Just to report. This patch fixed the invalid tcp tx checksum issue via tap for me. Thanks!

On Wed, Sep 19, 2012 at 08:44:49AM +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> dev_queue_xmit_nit() should be called right before ndo_start_xmit()
> calls or we might give wrong packet contents to taps users :
> 
> Packet checksum can be changed, or packet can be linearized or
> segmented, and segments partially sent for the later case.
> 
> Also a memory allocation can fail and packet never really hit the
> driver entry point.
> 
> Reported-by: Jamie Gloudon <jamie.gloudon@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/dev.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index dcc673d..52cd1d7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2213,9 +2213,6 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>  		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
>  			skb_dst_drop(skb);
>  
> -		if (!list_empty(&ptype_all))
> -			dev_queue_xmit_nit(skb, dev);
> -
>  		features = netif_skb_features(skb);
>  
>  		if (vlan_tx_tag_present(skb) &&
> @@ -2250,6 +2247,9 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
>  			}
>  		}
>  
> +		if (!list_empty(&ptype_all))
> +			dev_queue_xmit_nit(skb, dev);
> +
>  		skb_len = skb->len;
>  		rc = ops->ndo_start_xmit(skb, dev);
>  		trace_net_dev_xmit(skb, rc, dev, skb_len);
> @@ -2272,6 +2272,9 @@ gso:
>  		if (dev->priv_flags & IFF_XMIT_DST_RELEASE)
>  			skb_dst_drop(nskb);
>  
> +		if (!list_empty(&ptype_all))
> +			dev_queue_xmit_nit(nskb, dev);
> +
>  		skb_len = nskb->len;
>  		rc = ops->ndo_start_xmit(nskb, dev);
>  		trace_net_dev_xmit(nskb, rc, dev, skb_len);
> 
> 

Re: [PATCH net-next] net: more accurate network taps in transmit path

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 19 Sep 2012 08:44:49 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> dev_queue_xmit_nit() should be called right before ndo_start_xmit()
> calls or we might give wrong packet contents to taps users :
> 
> Packet checksum can be changed, or packet can be linearized or
> segmented, and segments partially sent for the later case.
> 
> Also a memory allocation can fail and packet never really hit the
> driver entry point.
> 
> Reported-by: Jamie Gloudon <jamie.gloudon@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Are you really sure that all the network tap implementations can
handle software GSO segmented skbs using skb->next linkage?

Because that is what they can potentially see after this change.

Re: [PATCH net-next] net: more accurate network taps in transmit path

[Eric Dumazet]

On Wed, 2012-09-19 at 14:16 -0400, David Miller wrote:

> Are you really sure that all the network tap implementations can
> handle software GSO segmented skbs using skb->next linkage?
> 
> Because that is what they can potentially see after this change.

I dont think so, because skb->next is NULL at the points I call the
network tap.

Or did I miss something ?

Re: [PATCH net-next] net: more accurate network taps in transmit path

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 19 Sep 2012 20:21:12 +0200

> On Wed, 2012-09-19 at 14:16 -0400, David Miller wrote:
> 
>> Are you really sure that all the network tap implementations can
>> handle software GSO segmented skbs using skb->next linkage?
>> 
>> Because that is what they can potentially see after this change.
> 
> I dont think so, because skb->next is NULL at the points I call the
> network tap.
> 
> Or did I miss something ?

Indeed, you're right.  I misread the control flow here.

Applied, thanks a lot Eric.