Message ID | 4EAB5455.5080004@8192.net |
---|---|
State | Rejected, archived |
Delegated to: | David Miller |
Headers | show |
2011/10/29 John <linux@8192.net>: > --- a/drivers/net/bonding/bond_main.c 2011-04-19 11:18:48.000000000 -0700 > +++ b/drivers/net/bonding/bond_main.c 2011-10-27 11:26:20.000000000 -0700 [...] > + v6hash = (v6hash >> 16) ^ (v6hash >> 8) ^ v6hash; This XORs in only 3 of 4 bytes, and assuming count > 256 (its unlikely you have more than 256 slaves in bond) the most significant byte is wasted. It should be: v6hash ^= v6hash >> 16; v6hash ^= v6hash >> 8; Same for IPv4 part. OTOH, if that was your intention, then the description in your mail was wrong. Best Regards, Michał Mirosław -- 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
Le samedi 29 octobre 2011 à 09:55 +0200, Michał Mirosław a écrit : > 2011/10/29 John <linux@8192.net>: > > --- a/drivers/net/bonding/bond_main.c 2011-04-19 11:18:48.000000000 -0700 > > +++ b/drivers/net/bonding/bond_main.c 2011-10-27 11:26:20.000000000 -0700 > [...] > > + v6hash = (v6hash >> 16) ^ (v6hash >> 8) ^ v6hash; > > This XORs in only 3 of 4 bytes, and assuming count > 256 (its unlikely > you have more than 256 slaves in bond) the most significant byte is > wasted. It should be: > > v6hash ^= v6hash >> 16; > v6hash ^= v6hash >> 8; > > Same for IPv4 part. > > OTOH, if that was your intention, then the description in your mail was wrong. > Same end result :) -- 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
Le samedi 29 octobre 2011 à 10:49 +0200, Eric Dumazet a écrit : > Le samedi 29 octobre 2011 à 09:55 +0200, Michał Mirosław a écrit : > > 2011/10/29 John <linux@8192.net>: > > > --- a/drivers/net/bonding/bond_main.c 2011-04-19 11:18:48.000000000 -0700 > > > +++ b/drivers/net/bonding/bond_main.c 2011-10-27 11:26:20.000000000 -0700 > > [...] > > > + v6hash = (v6hash >> 16) ^ (v6hash >> 8) ^ v6hash; > > > > This XORs in only 3 of 4 bytes, and assuming count > 256 (its unlikely > > you have more than 256 slaves in bond) the most significant byte is > > wasted. It should be: > > > > v6hash ^= v6hash >> 16; > > v6hash ^= v6hash >> 8; > > > > Same for IPv4 part. > > > > OTOH, if that was your intention, then the description in your mail was wrong. > > > > Same end result :) > Oh well, not exactly, there was a reason for using two different C instructions ;) ALso the modulo is expensive... -- 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
--- a/drivers/net/bonding/bond_main.c 2011-04-19 11:18:48.000000000 -0700 +++ b/drivers/net/bonding/bond_main.c 2011-10-27 11:26:20.000000000 -0700 @@ -3533,14 +3533,26 @@ static int bond_xmit_hash_policy_l23(struct sk_buff *skb, int count) { struct ethhdr *data = (struct ethhdr *)skb->data; - struct iphdr *iph = ip_hdr(skb); - if (skb->protocol == htons(ETH_P_IP)) { + if (skb->protocol == htons(ETH_P_IP) && + skb_network_header_len(skb) >= sizeof(struct iphdr)) { + struct iphdr *iph = ip_hdr(skb); return ((ntohl(iph->saddr ^ iph->daddr) & 0xffff) ^ (data->h_dest[5] ^ data->h_source[5])) % count; + } else if (skb->protocol == htons(ETH_P_IPV6) && + skb_network_header_len(skb) >= sizeof(struct ipv6hdr)) { + struct ipv6hdr *ipv6h = ipv6_hdr(skb); + u32 v6hash = + (ipv6h->saddr.s6_addr32[1] ^ ipv6h->daddr.s6_addr32[1]) ^ + (ipv6h->saddr.s6_addr32[2] ^ ipv6h->daddr.s6_addr32[2]) ^ + (ipv6h->saddr.s6_addr32[3] ^ ipv6h->daddr.s6_addr32[3]); + v6hash = (v6hash >> 16) ^ (v6hash >> 8) ^ v6hash; + return (v6hash ^ data->h_dest[5] ^ data->h_source[5]) % count; } - return (data->h_dest[5] ^ data->h_source[5]) % count; + if (skb_headlen(skb) >= 6) + return (data->h_dest[5] ^ data->h_source[5]) % count; + return 0; } /* @@ -3551,22 +3563,39 @@ static int bond_xmit_hash_policy_l34(struct sk_buff *skb, int count) { struct ethhdr *data = (struct ethhdr *)skb->data; - struct iphdr *iph = ip_hdr(skb); - __be16 *layer4hdr = (__be16 *)((u32 *)iph + iph->ihl); - int layer4_xor = 0; + u32 layer4_xor = 0; if (skb->protocol == htons(ETH_P_IP)) { + struct iphdr *iph = ip_hdr(skb); + __be16 *layer4hdr = (__be16 *)((u32 *)iph + iph->ihl); + if (iph->ihl * sizeof(u32) + sizeof(__be16) * 2 > + skb_headlen(skb) - skb_network_offset(skb)) goto SHORT_HEADER; if (!(iph->frag_off & htons(IP_MF|IP_OFFSET)) && - (iph->protocol == IPPROTO_TCP || - iph->protocol == IPPROTO_UDP)) { + (iph->protocol == IPPROTO_TCP || + iph->protocol == IPPROTO_UDP)) { layer4_xor = ntohs((*layer4hdr ^ *(layer4hdr + 1))); } return (layer4_xor ^ ((ntohl(iph->saddr ^ iph->daddr)) & 0xffff)) % count; - - } - - return (data->h_dest[5] ^ data->h_source[5]) % count; + } else if (skb->protocol == htons(ETH_P_IPV6)) { + struct ipv6hdr *ipv6h = ipv6_hdr(skb); + __be16 *layer4hdrv6 = (__be16 *)((u8 *)ipv6h + sizeof(*ipv6h)); + if (sizeof(struct ipv6hdr) + sizeof(__be16) * 2 > + skb_headlen(skb) - skb_network_offset(skb)) goto SHORT_HEADER; + if (ipv6h->nexthdr == IPPROTO_TCP || ipv6h->nexthdr == IPPROTO_UDP) { + layer4_xor = (*layer4hdrv6 ^ *(layer4hdrv6 + 1)); + } + layer4_xor ^= + (ipv6h->saddr.s6_addr32[1] ^ ipv6h->daddr.s6_addr32[1]) ^ + (ipv6h->saddr.s6_addr32[2] ^ ipv6h->daddr.s6_addr32[2]) ^ + (ipv6h->saddr.s6_addr32[3] ^ ipv6h->daddr.s6_addr32[3]); + return ((layer4_xor >> 16) ^ (layer4_xor >> 8) ^ layer4_xor) % count; + } + + SHORT_HEADER: + if (skb_headlen(skb) >= 6) + return (data->h_dest[5] ^ data->h_source[5]) % count; + return 0; } /*