datapath: Update hardware computed checksum on VLAN change.

The checksum computed by hardware on receive stored in skb->csum
when skb->ip_summed == CHECKSUM_COMPLETE is supposed to reflect
the contents of the packet starting at skb->data, which includes
the VLAN tag if there is one.  However, when we manipulate the
VLAN tag we don't update the checksum.  This leads to all kinds
of nasty warnings about broken hardware, not to mention we can't
take advantage of the checksum that was already computed.

This also fixes some issues with our private checksum type value
on some different kernels and after GSO.

diff --git a/datapath/actions.c b/datapath/actions.c
index b20873bf293592c3a0fa89d143f10043433bdd9d..b6d5488ce8a67f89e6dca193b38d556a9937b3fd 100644 (file)
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -65,11 +65,14 @@ vlan_pull_tag(struct sk_buff *skb)
        struct vlan_ethhdr *vh = vlan_eth_hdr(skb);
        struct ethhdr *eh;
 
-
        /* Verify we were given a vlan packet */
        if (vh->h_vlan_proto != htons(ETH_P_8021Q))
                return skb;
 
+       if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE)
+               skb->csum = csum_sub(skb->csum, csum_partial(skb->data
+                                       + ETH_HLEN, VLAN_HLEN, 0));
+
        memmove(skb->data + VLAN_HLEN, skb->data, 2 * VLAN_ETH_ALEN);
 
        eh = (struct ethhdr *)skb_pull(skb, VLAN_HLEN);
@@ -104,7 +107,16 @@ modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
        if (skb->protocol == htons(ETH_P_8021Q)) {
                /* Modify vlan id, but maintain other TCI values */
                struct vlan_ethhdr *vh = vlan_eth_hdr(skb);
+               __be16 old_tci = vh->h_vlan_TCI;
+
                vh->h_vlan_TCI = htons((ntohs(vh->h_vlan_TCI) & ~mask) | tci);
+
+               if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE) {
+                       __be16 diff[] = { ~old_tci, vh->h_vlan_TCI };
+
+                       skb->csum = ~csum_partial((char *)diff, sizeof(diff),
+                                               ~skb->csum);
+               }
        } else {
                /* Add vlan header */
 
@@ -144,6 +156,9 @@ modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
 
                                segs->next = NULL;
 
+                               /* GSO can change the checksum type so update.*/
+                               compute_ip_summed(segs, true);
+
                                segs = __vlan_put_tag(segs, tci);
                                err = -ENOMEM;
                                if (segs) {
@@ -167,6 +182,7 @@ modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
                        } while (segs->next);
 
                        skb = segs;
+                       compute_ip_summed(skb, true);
                }
 
                /* The hardware-accelerated version of vlan_put_tag() works
@@ -177,6 +193,12 @@ modify_vlan_tci(struct datapath *dp, struct sk_buff *skb,
                skb = __vlan_put_tag(skb, tci);
                if (!skb)
                        return ERR_PTR(-ENOMEM);
+
+               /* GSO doesn't fix up the hardware computed checksum so this
+                * will only be hit in the non-GSO case. */
+               if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE)
+                       skb->csum = csum_add(skb->csum, csum_partial(skb->data
+                                               + ETH_HLEN, VLAN_HLEN, 0));
        }
 
        return skb;
@@ -215,10 +237,10 @@ static void update_csum(__sum16 *sum, struct sk_buff *skb,
 {
        __be32 diff[] = { ~from, to };
 
-       if (OVS_CB(skb)->ip_summed != CSUM_PARTIAL) {
+       if (OVS_CB(skb)->ip_summed != OVS_CSUM_PARTIAL) {
                *sum = csum_fold(csum_partial((char *)diff, sizeof(diff),
                                ~csum_unfold(*sum)));
-               if (OVS_CB(skb)->ip_summed == CSUM_COMPLETE && pseudohdr)
+               if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE && pseudohdr)
                        skb->csum = ~csum_partial((char *)diff, sizeof(diff),
                                                ~skb->csum);
        } else if (pseudohdr)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index b6aefe892156fb3cbc887bd3d897c78140574bb4..76569f504072d039dd0f36cc7871e61c5490930b 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -71,7 +71,6 @@ static DEFINE_MUTEX(dp_mutex);
 #define MAINT_SLEEP_MSECS 1000
 
 static int new_nbp(struct datapath *, struct net_device *, int port_no);
-static void compute_ip_summed(struct sk_buff *skb);
 
 /* Must be called with rcu_read_lock or dp_mutex. */
 struct datapath *get_dp(int dp_idx)
@@ -530,7 +529,7 @@ void dp_process_received_packet(struct sk_buff *skb, struct net_bridge_port *p)
 
        WARN_ON_ONCE(skb_shared(skb));
 
-       compute_ip_summed(skb);
+       compute_ip_summed(skb, false);
 
        /* BHs are off so we don't have to use get_cpu()/put_cpu() here. */
        stats = percpu_ptr(dp->stats_percpu, smp_processor_id());
@@ -699,25 +698,57 @@ out:
  * skb_forward_csum().  It is slightly different because we are only concerned
  * with bridging and not other types of forwarding and can get away with
  * slightly more optimal behavior.*/
-static void
-compute_ip_summed(struct sk_buff *skb)
+void
+compute_ip_summed(struct sk_buff *skb, bool xmit)
 {
-       OVS_CB(skb)->ip_summed = skb->ip_summed;
-
+       /* For our convenience these defines change repeatedly between kernel
+        * versions, so we can't just copy them over... */
+       switch (skb->ip_summed) {
+       case CHECKSUM_NONE:
+               OVS_CB(skb)->ip_summed = OVS_CSUM_NONE;
+               break;
+       case CHECKSUM_UNNECESSARY:
+               OVS_CB(skb)->ip_summed = OVS_CSUM_UNNECESSARY;
+               break;
 #ifdef CHECKSUM_HW
        /* In theory this could be either CHECKSUM_PARTIAL or CHECKSUM_COMPLETE.
         * However, we should only get CHECKSUM_PARTIAL packets from Xen, which
         * uses some special fields to represent this (see below).  Since we
         * can only make one type work, pick the one that actually happens in
-        * practice. */
-       if (skb->ip_summed == CHECKSUM_HW)
-               OVS_CB(skb)->ip_summed = CSUM_COMPLETE;
+        * practice.
+        *
+        * The one exception to this is if we are on the transmit path
+        * (basically after skb_checksum_setup() has been run) the type has
+        * already been converted, so we should stay with that. */
+       case CHECKSUM_HW:
+               if (!xmit)
+                       OVS_CB(skb)->ip_summed = OVS_CSUM_COMPLETE;
+               else
+                       OVS_CB(skb)->ip_summed = OVS_CSUM_PARTIAL;
+
+               break;
+#else
+       case CHECKSUM_COMPLETE:
+               OVS_CB(skb)->ip_summed = OVS_CSUM_COMPLETE;
+               break;
+       case CHECKSUM_PARTIAL:
+               OVS_CB(skb)->ip_summed = OVS_CSUM_PARTIAL;
+               break;
 #endif
+       default:
+               printk(KERN_ERR "openvswitch: unknown checksum type %d\n",
+                      skb->ip_summed);
+               /* None seems the safest... */
+               OVS_CB(skb)->ip_summed = OVS_CSUM_NONE;
+       }       
+
 #if defined(CONFIG_XEN) && defined(HAVE_PROTO_DATA_VALID)
        /* Xen has a special way of representing CHECKSUM_PARTIAL on older
-        * kernels. */
+        * kernels. It should not be set on the transmit path though. */
        if (skb->proto_csum_blank)
-               OVS_CB(skb)->ip_summed = CSUM_PARTIAL;
+               OVS_CB(skb)->ip_summed = OVS_CSUM_PARTIAL;
+
+       WARN_ON_ONCE(skb->proto_csum_blank && xmit);
 #endif
 }
 
@@ -725,7 +756,7 @@ void
 forward_ip_summed(struct sk_buff *skb)
 {
 #ifdef CHECKSUM_HW
-       if (OVS_CB(skb)->ip_summed == CSUM_COMPLETE)
+       if (OVS_CB(skb)->ip_summed == OVS_CSUM_COMPLETE)
                skb->ip_summed = CHECKSUM_NONE;
 #endif
 }

diff --git a/datapath/datapath.h b/datapath/datapath.h
index d9fe4b2fb5d2b8951a852b4257be925e2b63e51a..38c84756f04a64b8bcc95b167428f9afb629ad6c 100644 (file)
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -183,10 +183,10 @@ struct net_bridge_port {
 };
 
 enum csum_type {
-       CSUM_NONE = 0,
-       CSUM_UNNECESSARY = 1,
-       CSUM_COMPLETE = 2,
-       CSUM_PARTIAL = 3,
+       OVS_CSUM_NONE = 0,
+       OVS_CSUM_UNNECESSARY = 1,
+       OVS_CSUM_COMPLETE = 2,
+       OVS_CSUM_PARTIAL = 3,
 };
 
 /**
@@ -236,6 +236,7 @@ static inline int vswitch_skb_checksum_setup(struct sk_buff *skb)
 }
 #endif
 
+void compute_ip_summed(struct sk_buff *skb, bool xmit);
 void forward_ip_summed(struct sk_buff *skb);
 
 #endif /* datapath.h */