From 635c9298b91d0942aca39ba1f0d7ea5805ab618e Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Thu, 4 Mar 2010 16:39:57 -0500 Subject: [PATCH] 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. --- datapath/actions.c | 28 ++++++++++++++++++++--- datapath/datapath.c | 55 +++++++++++++++++++++++++++++++++++---------- datapath/datapath.h | 9 ++++---- 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/datapath/actions.c b/datapath/actions.c index b20873bf..b6d5488c 100644 --- 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 b6aefe89..76569f50 100644 --- 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 d9fe4b2f..38c84756 100644 --- 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 */ -- 2.30.2