[PATCH v2 net-next 0/5] net: Increase inputs to flow_keys hashing

[PATCH v2 net-next 0/5] net: Increase inputs to flow_keys hashing

[Tom Herbert]

This patch set adds new fields to the flow_keys structure and hashes
over these fields to get a better flow hash. In particular, these
patches now include hashing over the full IPv6 addresses in order
to defend against address spoofing that always results in the
same hash. The new input also includes the Ethertype, L4 protocol,
VLAN, flow label, GRE keyid, and MPLS entropy label.

In order to increase hash inputs, we switch to using jhash2
which operates an an array of u32's. jhash2 operates on multiples of
three words. The data in the hash is constructed for that, and there
are are two variants for IPv4 and Ipv6 addressing. For IPv4 addresses,
jhash is performed over six u32's and for IPv6 it is done over twelve.

flow_keys can store either IPv4 or IPv6 addresses (addr_proto field
is a selector). ipv6_addr_hash is no longer used to convert addresses
for setting in flow table. For legacy uses of flow keys outside of
flow_dissector the flow_get_u32_src and flow_get_u32_dst functions
have been added to get u32 representation representations of addresses
in flow_keys.

For flow lables we also eliminate the short circuit in flow_dissector
for non-zero flow label. The flow label is now considered additional
input to ports.

Testing: Ran netperf TCP_RR for 200 flows using IPv4 and IPv6 comparing
before the patches and with the patches. Did not detect nay performance
degradation.

v2:
  - Took out MPLS entropy label. Will add this later.

Tom Herbert (5):
  net: Get skb hash over flow_keys structure
  net: Add full IPv6 addresses to flow_keys
  net: Add VLAN ID to flow_keys
  net: Add IPv6 flow label to flow_keys
  net: Add GRE keyid in flow_keys

 drivers/net/bonding/bond_main.c                |   9 +-
 drivers/net/ethernet/cisco/enic/enic_clsf.c    |   8 +-
 drivers/net/ethernet/cisco/enic/enic_ethtool.c |   4 +-
 include/net/flow_keys.h                        |  44 ++++++-
 include/net/ip.h                               |  21 +++-
 include/net/ipv6.h                             |  21 +++-
 net/core/flow_dissector.c                      | 159 +++++++++++++++++--------
 net/sched/cls_flow.c                           |  14 ++-
 8 files changed, 208 insertions(+), 72 deletions(-)

-- 
1.8.1

[PATCH v2 net-next 1/5] net: Get skb hash over flow_keys structure

[Tom Herbert]

This patch changes flow hashing to use jhash2 over the flow_keys
structure instead just doing jhash_3words over src, dst, and ports.
This method will allow us take more input into the hashing function
so that we can include full IPv6 addresses, VLAN, flow labels etc.
without needing to resort to xor'ing which makes for a poor hash.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/flow_keys.h   | 12 +++++++++---
 net/core/flow_dissector.c | 20 ++++++++++++++------
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 6d6ef62..5907472 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -15,6 +15,12 @@
  * All the members, except thoff, are in network byte order.
  */
 struct flow_keys {
+	u16	thoff;
+#define FLOW_KEYS_HASH_START_FIELD	n_proto
+	__be16	n_proto;
+	u8	ip_proto;
+	u8	padding;
+
 	/* (src,dst) must be grouped, in the same way than in IP header */
 	__be32 src;
 	__be32 dst;
@@ -22,11 +28,11 @@ struct flow_keys {
 		__be32 ports;
 		__be16 port16[2];
 	};
-	u16	thoff;
-	__be16	n_proto;
-	u8	ip_proto;
 };
 
+#define FLOW_KEYS_HASH_OFFSET		\
+	offsetof(struct flow_keys, FLOW_KEYS_HASH_START_FIELD)
+
 bool __skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow,
 			void *data, __be16 proto, int nhoff, int hlen);
 static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow)
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index d3acc4d..02c5104 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -267,9 +267,19 @@ static __always_inline void __flow_hash_secret_init(void)
 	net_get_random_once(&hashrnd, sizeof(hashrnd));
 }
 
-static __always_inline u32 __flow_hash_3words(u32 a, u32 b, u32 c, u32 keyval)
+static __always_inline u32 __flow_hash_words(u32 *words, u32 length, u32 keyval)
 {
-	return jhash_3words(a, b, c, keyval);
+	return jhash2(words, length, keyval);
+}
+
+static inline void *flow_keys_hash_start(struct flow_keys *flow)
+{
+	return (void *)flow + FLOW_KEYS_HASH_OFFSET;
+}
+
+static inline size_t flow_keys_hash_length(struct flow_keys *flow)
+{
+	return (sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) / sizeof(u32);
 }
 
 static inline u32 __flow_hash_from_keys(struct flow_keys *keys, u32 keyval)
@@ -284,10 +294,8 @@ static inline u32 __flow_hash_from_keys(struct flow_keys *keys, u32 keyval)
 		swap(keys->port16[0], keys->port16[1]);
 	}
 
-	hash = __flow_hash_3words((__force u32)keys->dst,
-				  (__force u32)keys->src,
-				  (__force u32)keys->ports,
-				  keyval);
+	hash = __flow_hash_words((u32 *)flow_keys_hash_start(keys),
+				 flow_keys_hash_length(keys), keyval);
 	if (!hash)
 		hash = 1;
 
-- 
1.8.1

[PATCH v2 net-next 2/5] net: Add full IPv6 addresses to flow_keys

[Tom Herbert]

This patch adds full IPv6 addresses into flow_keys and uses them as
input to the flow hash function. The implementation supports either
IPv4 or IPv6 addresses in a union, and selector is used to determine
how may words to input to jhash2.

We also add flow_get_u32_dst and flow_get_u32_src functions which are
used to get a u32 representation of the source and destination
addresses. For IPv6, ipv6_addr_hash is called. These functions retain
getting the legacy values of src and dst in flow_keys.

With this patch, Ethertype and IP protocol are now included in the
flow hash input.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 drivers/net/bonding/bond_main.c                |   9 +--
 drivers/net/ethernet/cisco/enic/enic_clsf.c    |   8 +-
 drivers/net/ethernet/cisco/enic/enic_ethtool.c |   4 +-
 include/net/flow_keys.h                        |  29 ++++++-
 include/net/ip.h                               |  21 ++++-
 include/net/ipv6.h                             |  21 ++++-
 net/core/flow_dissector.c                      | 108 +++++++++++++++++++------
 net/sched/cls_flow.c                           |  14 +++-
 8 files changed, 166 insertions(+), 48 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2ee13be..134c3f3 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3062,8 +3062,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 		if (unlikely(!pskb_may_pull(skb, noff + sizeof(*iph))))
 			return false;
 		iph = ip_hdr(skb);
-		fk->src = iph->saddr;
-		fk->dst = iph->daddr;
+		iph_to_flow_copy_v4addrs(fk, iph);
 		noff += iph->ihl << 2;
 		if (!ip_is_fragment(iph))
 			proto = iph->protocol;
@@ -3071,8 +3070,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 		if (unlikely(!pskb_may_pull(skb, noff + sizeof(*iph6))))
 			return false;
 		iph6 = ipv6_hdr(skb);
-		fk->src = (__force __be32)ipv6_addr_hash(&iph6->saddr);
-		fk->dst = (__force __be32)ipv6_addr_hash(&iph6->daddr);
+		iph_to_flow_copy_v6addrs(fk, iph6);
 		noff += sizeof(*iph6);
 		proto = iph6->nexthdr;
 	} else {
@@ -3106,7 +3104,8 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
 		hash = bond_eth_hash(skb);
 	else
 		hash = (__force u32)flow.ports;
-	hash ^= (__force u32)flow.dst ^ (__force u32)flow.src;
+	hash ^= (__force u32)flow_get_u32_dst(&flow) ^
+		(__force u32)flow_get_u32_src(&flow);
 	hash ^= (hash >> 16);
 	hash ^= (hash >> 8);
 
diff --git a/drivers/net/ethernet/cisco/enic/enic_clsf.c b/drivers/net/ethernet/cisco/enic/enic_clsf.c
index 0be6850..d8cbea1 100644
--- a/drivers/net/ethernet/cisco/enic/enic_clsf.c
+++ b/drivers/net/ethernet/cisco/enic/enic_clsf.c
@@ -33,8 +33,8 @@ int enic_addfltr_5t(struct enic *enic, struct flow_keys *keys, u16 rq)
 		return -EPROTONOSUPPORT;
 	};
 	data.type = FILTER_IPV4_5TUPLE;
-	data.u.ipv4.src_addr = ntohl(keys->src);
-	data.u.ipv4.dst_addr = ntohl(keys->dst);
+	data.u.ipv4.src_addr = ntohl(keys->v4addrs.src);
+	data.u.ipv4.dst_addr = ntohl(keys->v4addrs.dst);
 	data.u.ipv4.src_port = ntohs(keys->port16[0]);
 	data.u.ipv4.dst_port = ntohs(keys->port16[1]);
 	data.u.ipv4.flags = FILTER_FIELDS_IPV4_5TUPLE;
@@ -158,8 +158,8 @@ static struct enic_rfs_fltr_node *htbl_key_search(struct hlist_head *h,
 	struct enic_rfs_fltr_node *tpos;
 
 	hlist_for_each_entry(tpos, h, node)
-		if (tpos->keys.src == k->src &&
-		    tpos->keys.dst == k->dst &&
+		if (tpos->keys.v4addrs.src == k->v4addrs.src &&
+		    tpos->keys.v4addrs.dst == k->v4addrs.dst &&
 		    tpos->keys.ports == k->ports &&
 		    tpos->keys.ip_proto == k->ip_proto &&
 		    tpos->keys.n_proto == k->n_proto)
diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index 28d9ca6..6596c98 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -346,10 +346,10 @@ static int enic_grxclsrule(struct enic *enic, struct ethtool_rxnfc *cmd)
 		break;
 	}
 
-	fsp->h_u.tcp_ip4_spec.ip4src = n->keys.src;
+	fsp->h_u.tcp_ip4_spec.ip4src = flow_get_u32_src(&n->keys);
 	fsp->m_u.tcp_ip4_spec.ip4src = (__u32)~0;
 
-	fsp->h_u.tcp_ip4_spec.ip4dst = n->keys.dst;
+	fsp->h_u.tcp_ip4_spec.ip4dst = flow_get_u32_dst(&n->keys);
 	fsp->m_u.tcp_ip4_spec.ip4dst = (__u32)~0;
 
 	fsp->h_u.tcp_ip4_spec.psrc = n->keys.port16[0];
diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 5907472..c8bc6aa 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -16,23 +16,46 @@
  */
 struct flow_keys {
 	u16	thoff;
+	u16	addr_proto;
+#define	FLOW_KEYS_ADDR_NONE	0
+#define	FLOW_KEYS_ADDR_IPV4	1
+#define	FLOW_KEYS_ADDR_IPV6	2
+
+	/* Fields below this point are taken as input to skb_hash */
 #define FLOW_KEYS_HASH_START_FIELD	n_proto
 	__be16	n_proto;
 	u8	ip_proto;
 	u8	padding;
 
-	/* (src,dst) must be grouped, in the same way than in IP header */
-	__be32 src;
-	__be32 dst;
 	union {
 		__be32 ports;
 		__be16 port16[2];
 	};
+
+	/* (src,dst) must be grouped, in the same way than in IP header */
+	union {
+		u32	addrs;
+#define FLOW_KEYS_HASH_ADDR_START_FIELD	addrs
+		struct {
+			__be32 src;
+			__be32 dst;
+		} v4addrs;
+		struct {
+			__be32 src[4];
+			__be32 dst[4];
+		} v6addrs;
+	};
 };
 
 #define FLOW_KEYS_HASH_OFFSET		\
 	offsetof(struct flow_keys, FLOW_KEYS_HASH_START_FIELD)
 
+#define FLOW_KEYS_HASH_ADDRS_OFFSET	\
+	offsetof(struct flow_keys, FLOW_KEYS_HASH_ADDR_START_FIELD)
+
+__be32 flow_get_u32_src(const struct flow_keys *flow);
+__be32 flow_get_u32_dst(const struct flow_keys *flow);
+
 bool __skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow,
 			void *data, __be16 proto, int nhoff, int hlen);
 static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow)
diff --git a/include/net/ip.h b/include/net/ip.h
index d14af7e..fa89279 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -355,13 +355,30 @@ static inline __wsum inet_compute_pseudo(struct sk_buff *skb, int proto)
 				  skb->len, proto, 0);
 }
 
+/* copy IPv4 saddr & daddr to flow_keys, possibly using 64bit load/store
+ * Equivalent to :	flow->v4addrs.src = iph->saddr;
+ *			flow->v4addrs.dst = iph->daddr;
+ */
+static inline void iph_to_flow_copy_v4addrs(struct flow_keys *flow,
+					    const struct iphdr *iph)
+{
+	BUILD_BUG_ON(offsetof(typeof(*flow), v4addrs.dst) !=
+		     offsetof(typeof(*flow), v4addrs.src) +
+			      sizeof(flow->v4addrs.src));
+	memcpy(&flow->v4addrs, &iph->saddr, sizeof(flow->v4addrs));
+	flow->addr_proto = FLOW_KEYS_ADDR_IPV4;
+}
+
 static inline void inet_set_txhash(struct sock *sk)
 {
 	struct inet_sock *inet = inet_sk(sk);
 	struct flow_keys keys;
 
-	keys.src = inet->inet_saddr;
-	keys.dst = inet->inet_daddr;
+	memset(&keys, 0, sizeof(keys));
+
+	keys.v4addrs.src = inet->inet_saddr;
+	keys.v4addrs.dst = inet->inet_daddr;
+	keys.addr_proto = FLOW_KEYS_ADDR_IPV4;
 	keys.port16[0] = inet->inet_sport;
 	keys.port16[1] = inet->inet_dport;
 
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 53d25ef..5e2b8dc 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -691,6 +691,20 @@ static inline int ip6_sk_dst_hoplimit(struct ipv6_pinfo *np, struct flowi6 *fl6,
 	return hlimit;
 }
 
+/* copy IPv6 saddr & daddr to flow_keys, possibly using 64bit load/store
+ * Equivalent to :	flow->v6addrs.src = iph->saddr;
+ *			flow->v6addrs.dst = iph->daddr;
+ */
+static inline void iph_to_flow_copy_v6addrs(struct flow_keys *flow,
+					    const struct ipv6hdr *iph)
+{
+	BUILD_BUG_ON(offsetof(typeof(*flow), v6addrs.dst) !=
+		     offsetof(typeof(*flow), v6addrs.src) +
+			      sizeof(flow->v6addrs.src));
+	memcpy(&flow->v6addrs, &iph->saddr, sizeof(flow->v6addrs));
+	flow->addr_proto = FLOW_KEYS_ADDR_IPV6;
+}
+
 #if IS_ENABLED(CONFIG_IPV6)
 static inline void ip6_set_txhash(struct sock *sk)
 {
@@ -698,8 +712,11 @@ static inline void ip6_set_txhash(struct sock *sk)
 	struct ipv6_pinfo *np = inet6_sk(sk);
 	struct flow_keys keys;
 
-	keys.src = (__force __be32)ipv6_addr_hash(&np->saddr);
-	keys.dst = (__force __be32)ipv6_addr_hash(&sk->sk_v6_daddr);
+	memset(&keys, 0, sizeof(keys));
+
+	memcpy(&keys.v6addrs.src, &np->saddr, sizeof(keys.v6addrs.src));
+	memcpy(&keys.v6addrs.dst, &sk->sk_v6_daddr, sizeof(keys.v6addrs.dst));
+	keys.addr_proto = FLOW_KEYS_ADDR_IPV6;
 	keys.port16[0] = inet->inet_sport;
 	keys.port16[1] = inet->inet_dport;
 
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 02c5104..69fbaf9 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -15,17 +15,6 @@
 #include <net/flow_keys.h>
 #include <scsi/fc/fc_fcoe.h>
 
-/* copy saddr & daddr, possibly using 64bit load/store
- * Equivalent to :	flow->src = iph->saddr;
- *			flow->dst = iph->daddr;
- */
-static void iph_to_flow_copy_addrs(struct flow_keys *flow, const struct iphdr *iph)
-{
-	BUILD_BUG_ON(offsetof(typeof(*flow), dst) !=
-		     offsetof(typeof(*flow), src) + sizeof(flow->src));
-	memcpy(&flow->src, &iph->saddr, sizeof(flow->src) + sizeof(flow->dst));
-}
-
 /**
  * __skb_flow_get_ports - extract the upper layer ports and return them
  * @skb: sk_buff to extract the ports from
@@ -107,7 +96,7 @@ ip:
 		if (!skb)
 			break;
 
-		iph_to_flow_copy_addrs(flow, iph);
+		iph_to_flow_copy_v4addrs(flow, iph);
 		break;
 	}
 	case htons(ETH_P_IPV6): {
@@ -127,8 +116,7 @@ ipv6:
 		if (!skb)
 			break;
 
-		flow->src = (__force __be32)ipv6_addr_hash(&iph->saddr);
-		flow->dst = (__force __be32)ipv6_addr_hash(&iph->daddr);
+		iph_to_flow_copy_v6addrs(flow, iph);
 
 		flow_label = ip6_flowlabel(iph);
 		if (flow_label) {
@@ -186,8 +174,9 @@ ipv6:
 		hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen, &_hdr);
 		if (!hdr)
 			return false;
-		flow->src = hdr->srcnode;
-		flow->dst = 0;
+		flow->v4addrs.src = hdr->srcnode;
+		flow->v4addrs.dst = 0;
+		flow->addr_proto = FLOW_KEYS_ADDR_IPV4;
 		flow->n_proto = proto;
 		flow->thoff = (u16)nhoff;
 		return true;
@@ -279,20 +268,87 @@ static inline void *flow_keys_hash_start(struct flow_keys *flow)
 
 static inline size_t flow_keys_hash_length(struct flow_keys *flow)
 {
-	return (sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) / sizeof(u32);
+	size_t len = (FLOW_KEYS_HASH_ADDRS_OFFSET -
+		       FLOW_KEYS_HASH_OFFSET) / sizeof(u32);
+
+	switch (flow->addr_proto) {
+	case FLOW_KEYS_ADDR_IPV4:
+		len += sizeof(flow->v4addrs) / sizeof(u32);
+		break;
+	case FLOW_KEYS_ADDR_IPV6:
+		len += sizeof(flow->v6addrs) / sizeof(u32);
+		break;
+	}
+
+	return len;
 }
 
-static inline u32 __flow_hash_from_keys(struct flow_keys *keys, u32 keyval)
+__be32 flow_get_u32_src(const struct flow_keys *flow)
 {
-	u32 hash;
+	switch (flow->addr_proto) {
+	case FLOW_KEYS_ADDR_IPV4:
+		return flow->v4addrs.src;
+	case FLOW_KEYS_ADDR_IPV6:
+		return (__force __be32)ipv6_addr_hash(
+				(struct in6_addr *)flow->v6addrs.src);
+	default:
+		return 0;
+	}
+}
+EXPORT_SYMBOL(flow_get_u32_src);
+
+__be32 flow_get_u32_dst(const struct flow_keys *flow)
+{
+	switch (flow->addr_proto) {
+	case FLOW_KEYS_ADDR_IPV4:
+		return flow->v4addrs.dst;
+	case FLOW_KEYS_ADDR_IPV6:
+		return (__force __be32)ipv6_addr_hash(
+				(struct in6_addr *)flow->v6addrs.dst);
+	default:
+		return 0;
+	}
+}
+EXPORT_SYMBOL(flow_get_u32_dst);
+
+static inline void __flow_hash_consistentify(struct flow_keys *keys)
+{
+	int addr_diff, i;
 
 	/* get a consistent hash (same value on both flow directions) */
-	if (((__force u32)keys->dst < (__force u32)keys->src) ||
-	    (((__force u32)keys->dst == (__force u32)keys->src) &&
-	     ((__force u16)keys->port16[1] < (__force u16)keys->port16[0]))) {
-		swap(keys->dst, keys->src);
-		swap(keys->port16[0], keys->port16[1]);
+	switch (keys->addr_proto) {
+	case FLOW_KEYS_ADDR_IPV4:
+		addr_diff = (__force u32)keys->v4addrs.dst -
+			    (__force u32)keys->v4addrs.src;
+		if ((addr_diff < 0) ||
+		    (addr_diff == 0 &&
+		     ((__force u16)keys->port16[1] <
+		      (__force u16)keys->port16[0]))) {
+			swap(keys->v4addrs.dst, keys->v4addrs.src);
+			swap(keys->port16[0], keys->port16[1]);
+		}
+		break;
+	case FLOW_KEYS_ADDR_IPV6:
+		addr_diff = memcmp(keys->v6addrs.dst, keys->v6addrs.src,
+				   sizeof(keys->v6addrs.dst));
+		if ((addr_diff < 0) ||
+		    (addr_diff == 0 &&
+		     ((__force u16)keys->port16[1] <
+		      (__force u16)keys->port16[0]))) {
+			for (i = 0; i < 4; i++)
+				swap(keys->v6addrs.dst[i],
+				     keys->v6addrs.src[i]);
+			swap(keys->port16[0], keys->port16[1]);
+		}
+		break;
 	}
+}
+
+static inline u32 __flow_hash_from_keys(struct flow_keys *keys, u32 keyval)
+{
+	u32 hash;
+
+	__flow_hash_consistentify(keys);
 
 	hash = __flow_hash_words((u32 *)flow_keys_hash_start(keys),
 				 flow_keys_hash_length(keys), keyval);
@@ -340,8 +396,8 @@ void make_flow_keys_digest(struct flow_keys_digest *digest,
 	data->n_proto = flow->n_proto;
 	data->ip_proto = flow->ip_proto;
 	data->ports = flow->ports;
-	data->src = flow->src;
-	data->dst = flow->dst;
+	data->src = flow_get_u32_src(flow);
+	data->dst = flow_get_u32_dst(flow);
 }
 EXPORT_SYMBOL(make_flow_keys_digest);
 
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index a620c4e..3788b929 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -68,15 +68,21 @@ static inline u32 addr_fold(void *addr)
 
 static u32 flow_get_src(const struct sk_buff *skb, const struct flow_keys *flow)
 {
-	if (flow->src)
-		return ntohl(flow->src);
+	__be32 src = flow_get_u32_src(flow);
+
+	if (src)
+		return ntohl(src);
+
 	return addr_fold(skb->sk);
 }
 
 static u32 flow_get_dst(const struct sk_buff *skb, const struct flow_keys *flow)
 {
-	if (flow->dst)
-		return ntohl(flow->dst);
+	__be32 dst = flow_get_u32_dst(flow);
+
+	if (dst)
+		return ntohl(dst);
+
 	return addr_fold(skb_dst(skb)) ^ (__force u16) tc_skb_protocol(skb);
 }
 
-- 
1.8.1

[PATCH v2 net-next 3/5] net: Add VLAN ID to flow_keys

[Tom Herbert]

In flow_dissector set vlan_id in flow_keys when VLAN is found.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/flow_keys.h   | 1 +
 net/core/flow_dissector.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index c8bc6aa..14298e2 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -26,6 +26,7 @@ struct flow_keys {
 	__be16	n_proto;
 	u8	ip_proto;
 	u8	padding;
+	u32	vlan_id:12;
 
 	union {
 		__be32 ports;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 69fbaf9..148b989 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -143,6 +143,7 @@ ipv6:
 		if (!vlan)
 			return false;
 
+		flow->vlan_id = skb_vlan_tag_get_id(skb);
 		proto = vlan->h_vlan_encapsulated_proto;
 		nhoff += sizeof(*vlan);
 		goto again;
-- 
1.8.1

[PATCH v2 net-next 4/5] net: Add IPv6 flow label to flow_keys

[Tom Herbert]

In flow_dissector set the flow label in flow_keys for IPv6. This also
removes the shortcircuiting of flow dissection when a non-zero label
is present, the flow label can be considered to provide additional
entropy for a hash.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/flow_keys.h   |  3 ++-
 net/core/flow_dissector.c | 15 +--------------
 2 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 14298e2..906d47a 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -26,7 +26,8 @@ struct flow_keys {
 	__be16	n_proto;
 	u8	ip_proto;
 	u8	padding;
-	u32	vlan_id:12;
+	u32	vlan_id:12,
+		flow_label:20;
 
 	union {
 		__be32 ports;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 148b989..de71e42 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -102,7 +102,6 @@ ip:
 	case htons(ETH_P_IPV6): {
 		const struct ipv6hdr *iph;
 		struct ipv6hdr _iph;
-		__be32 flow_label;
 
 ipv6:
 		iph = __skb_header_pointer(skb, nhoff, sizeof(_iph), data, hlen, &_iph);
@@ -118,19 +117,7 @@ ipv6:
 
 		iph_to_flow_copy_v6addrs(flow, iph);
 
-		flow_label = ip6_flowlabel(iph);
-		if (flow_label) {
-			/* Awesome, IPv6 packet has a flow label so we can
-			 * use that to represent the ports without any
-			 * further dissection.
-			 */
-			flow->n_proto = proto;
-			flow->ip_proto = ip_proto;
-			flow->ports = flow_label;
-			flow->thoff = (u16)nhoff;
-
-			return true;
-		}
+		flow->flow_label = ntohl(ip6_flowlabel(iph));
 
 		break;
 	}
-- 
1.8.1

[PATCH v2 net-next 5/5] net: Add GRE keyid in flow_keys

[Tom Herbert]

In flow dissector if a GRE header contains a keyid this is saved in the
new extra entropy field of flow_keys. The GRE keyid is then represented
in the flow hash function input.

Signed-off-by: Tom Herbert <tom@herbertland.com>
---
 include/net/flow_keys.h   |  1 +
 net/core/flow_dissector.c | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
index 906d47a..779e42f 100644
--- a/include/net/flow_keys.h
+++ b/include/net/flow_keys.h
@@ -28,6 +28,7 @@ struct flow_keys {
 	u8	padding;
 	u32	vlan_id:12,
 		flow_label:20;
+	u32	extra_entropy;
 
 	union {
 		__be32 ports;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index de71e42..025b91f 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -195,8 +195,21 @@ ipv6:
 			nhoff += 4;
 			if (hdr->flags & GRE_CSUM)
 				nhoff += 4;
-			if (hdr->flags & GRE_KEY)
+			if (hdr->flags & GRE_KEY) {
+				const __be32 *keyid;
+				__be32 _keyid;
+
+				keyid = __skb_header_pointer(skb, nhoff,
+							     sizeof(_keyid),
+							     data, hlen,
+							     &_keyid);
+				if (!keyid)
+					return false;
+
+				flow->extra_entropy ^= *keyid;
+
 				nhoff += 4;
+			}
 			if (hdr->flags & GRE_SEQ)
 				nhoff += 4;
 			if (proto == htons(ETH_P_TEB)) {
-- 
1.8.1

Re: [PATCH v2 net-next 5/5] net: Add GRE keyid in flow_keys

[Eric Dumazet]

On Mon, 2015-05-11 at 20:26 -0700, Tom Herbert wrote:
> In flow dissector if a GRE header contains a keyid this is saved in the
> new extra entropy field of flow_keys. The GRE keyid is then represented
> in the flow hash function input.
> 
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> ---
>  include/net/flow_keys.h   |  1 +
>  net/core/flow_dissector.c | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
> index 906d47a..779e42f 100644
> --- a/include/net/flow_keys.h
> +++ b/include/net/flow_keys.h
> @@ -28,6 +28,7 @@ struct flow_keys {
>  	u8	padding;
>  	u32	vlan_id:12,
>  		flow_label:20;
> +	u32	extra_entropy;
>  
>  	union {
>  		__be32 ports;
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index de71e42..025b91f 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -195,8 +195,21 @@ ipv6:
>  			nhoff += 4;
>  			if (hdr->flags & GRE_CSUM)
>  				nhoff += 4;
> -			if (hdr->flags & GRE_KEY)
> +			if (hdr->flags & GRE_KEY) {
> +				const __be32 *keyid;
> +				__be32 _keyid;
> +
> +				keyid = __skb_header_pointer(skb, nhoff,
> +							     sizeof(_keyid),
> +							     data, hlen,
> +							     &_keyid);
> +				if (!keyid)
> +					return false;
> +
> +				flow->extra_entropy ^= *keyid;


This probably should not please sparse ?

make C=2 CF=-D__CHECK_ENDIAN__  net/core/flow_dissector.o

Re: [PATCH v2 net-next 5/5] net: Add GRE keyid in flow_keys

[Eric Dumazet]

On Mon, 2015-05-11 at 21:33 -0700, Eric Dumazet wrote:
> On Mon, 2015-05-11 at 20:26 -0700, Tom Herbert wrote:
> > In flow dissector if a GRE header contains a keyid this is saved in the
> > new extra entropy field of flow_keys. The GRE keyid is then represented
> > in the flow hash function input.
> > 
> > Signed-off-by: Tom Herbert <tom@herbertland.com>
> > ---
> >  include/net/flow_keys.h   |  1 +
> >  net/core/flow_dissector.c | 15 ++++++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
> > index 906d47a..779e42f 100644
> > --- a/include/net/flow_keys.h
> > +++ b/include/net/flow_keys.h
> > @@ -28,6 +28,7 @@ struct flow_keys {
> >  	u8	padding;
> >  	u32	vlan_id:12,
> >  		flow_label:20;
> > +	u32	extra_entropy;
> >  
> >  	union {
> >  		__be32 ports;
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index de71e42..025b91f 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -195,8 +195,21 @@ ipv6:
> >  			nhoff += 4;
> >  			if (hdr->flags & GRE_CSUM)
> >  				nhoff += 4;
> > -			if (hdr->flags & GRE_KEY)
> > +			if (hdr->flags & GRE_KEY) {
> > +				const __be32 *keyid;
> > +				__be32 _keyid;
> > +
> > +				keyid = __skb_header_pointer(skb, nhoff,
> > +							     sizeof(_keyid),
> > +							     data, hlen,
> > +							     &_keyid);
> > +				if (!keyid)
> > +					return false;
> > +
> > +				flow->extra_entropy ^= *keyid;
> 
> 
> This probably should not please sparse ?
> 
> make C=2 CF=-D__CHECK_ENDIAN__  net/core/flow_dissector.o
> 

BTW, you never addressed prior feedback about sparse errors :

include/linux/skbuff.h:3054:24: warning: incorrect type in return expression (different base types)
include/linux/skbuff.h:3054:24:    expected restricted __sum16
include/linux/skbuff.h:3054:24:    got int
include/linux/skbuff.h:3054:24: warning: incorrect type in return expression (different base types)
include/linux/skbuff.h:3054:24:    expected restricted __sum16
include/linux/skbuff.h:3054:24:    got int
include/linux/skbuff.h:3362:14: warning: incorrect type in assignment (different base types)
include/linux/skbuff.h:3362:14:    expected unsigned short [unsigned] [usertype] csum
include/linux/skbuff.h:3362:14:    got restricted __sum16
include/linux/skbuff.h:3367:16: warning: incorrect type in return expression (different base types)
include/linux/skbuff.h:3367:16:    expected restricted __sum16
include/linux/skbuff.h:3367:16:    got unsigned short [unsigned] [usertype] csum

commit 7e2b10c1e52ca37f ("net: Support for multiple checksums with gso")

Thanks

Re: [PATCH v2 net-next 1/5] net: Get skb hash over flow_keys structure

[Eric Dumazet]

On Mon, 2015-05-11 at 20:26 -0700, Tom Herbert wrote:
> This patch changes flow hashing to use jhash2 over the flow_keys
> structure instead just doing jhash_3words over src, dst, and ports.
> This method will allow us take more input into the hashing function
> so that we can include full IPv6 addresses, VLAN, flow labels etc.
> without needing to resort to xor'ing which makes for a poor hash.
> 
> Signed-off-by: Tom Herbert <tom@herbertland.com>
> ---
>  include/net/flow_keys.h   | 12 +++++++++---
>  net/core/flow_dissector.c | 20 ++++++++++++++------
>  2 files changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
> index 6d6ef62..5907472 100644
> --- a/include/net/flow_keys.h
> +++ b/include/net/flow_keys.h
> @@ -15,6 +15,12 @@
>   * All the members, except thoff, are in network byte order.
>   */
>  struct flow_keys {
> +	u16	thoff;
> +#define FLOW_KEYS_HASH_START_FIELD	n_proto
> +	__be16	n_proto;
> +	u8	ip_proto;
> +	u8	padding;
> +
>  	/* (src,dst) must be grouped, in the same way than in IP header */
>  	__be32 src;
>  	__be32 dst;
> @@ -22,11 +28,11 @@ struct flow_keys {
>  		__be32 ports;
>  		__be16 port16[2];
>  	};
> -	u16	thoff;
> -	__be16	n_proto;
> -	u8	ip_proto;
>  };
>  
> +#define FLOW_KEYS_HASH_OFFSET		\
> +	offsetof(struct flow_keys, FLOW_KEYS_HASH_START_FIELD)
> +

I don't really understand : 

This (2) is not a multiple of 4, so some arches will have unaligned word
accesses ?

>  bool __skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow,
>  			void *data, __be16 proto, int nhoff, int hlen);
>  static inline bool skb_flow_dissect(const struct sk_buff *skb, struct flow_keys *flow)
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index d3acc4d..02c5104 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -267,9 +267,19 @@ static __always_inline void __flow_hash_secret_init(void)
>  	net_get_random_once(&hashrnd, sizeof(hashrnd));
>  }
>  
> -static __always_inline u32 __flow_hash_3words(u32 a, u32 b, u32 c, u32 keyval)
> +static __always_inline u32 __flow_hash_words(u32 *words, u32 length, u32 keyval)
>  {
> -	return jhash_3words(a, b, c, keyval);
> +	return jhash2(words, length, keyval);
> +}
> +

-> crash or very slow on MIPS.

Re: [PATCH v2 net-next 1/5] net: Get skb hash over flow_keys structure

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 May 2015 21:56:46 -0700

> On Mon, 2015-05-11 at 20:26 -0700, Tom Herbert wrote:
 ...
> This (2) is not a multiple of 4, so some arches will have unaligned word
> accesses ?
 ...
>> -static __always_inline u32 __flow_hash_3words(u32 a, u32 b, u32 c, u32 keyval)
>> +static __always_inline u32 __flow_hash_words(u32 *words, u32 length, u32 keyval)
>>  {
>> -	return jhash_3words(a, b, c, keyval);
>> +	return jhash2(words, length, keyval);
>> +}
>> +
> 
> -> crash or very slow on MIPS.

Yeah you really can't do this.  You must only pass 4-byte aligned items
into the hash function.

Re: [PATCH v2 net-next 5/5] net: Add GRE keyid in flow_keys

[Tom Herbert]

On Tue, May 12, 2015 at 12:50 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2015-05-11 at 21:33 -0700, Eric Dumazet wrote:
>> On Mon, 2015-05-11 at 20:26 -0700, Tom Herbert wrote:
>> > In flow dissector if a GRE header contains a keyid this is saved in the
>> > new extra entropy field of flow_keys. The GRE keyid is then represented
>> > in the flow hash function input.
>> >
>> > Signed-off-by: Tom Herbert <tom@herbertland.com>
>> > ---
>> >  include/net/flow_keys.h   |  1 +
>> >  net/core/flow_dissector.c | 15 ++++++++++++++-
>> >  2 files changed, 15 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/net/flow_keys.h b/include/net/flow_keys.h
>> > index 906d47a..779e42f 100644
>> > --- a/include/net/flow_keys.h
>> > +++ b/include/net/flow_keys.h
>> > @@ -28,6 +28,7 @@ struct flow_keys {
>> >     u8      padding;
>> >     u32     vlan_id:12,
>> >             flow_label:20;
>> > +   u32     extra_entropy;
>> >
>> >     union {
>> >             __be32 ports;
>> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> > index de71e42..025b91f 100644
>> > --- a/net/core/flow_dissector.c
>> > +++ b/net/core/flow_dissector.c
>> > @@ -195,8 +195,21 @@ ipv6:
>> >                     nhoff += 4;
>> >                     if (hdr->flags & GRE_CSUM)
>> >                             nhoff += 4;
>> > -                   if (hdr->flags & GRE_KEY)
>> > +                   if (hdr->flags & GRE_KEY) {
>> > +                           const __be32 *keyid;
>> > +                           __be32 _keyid;
>> > +
>> > +                           keyid = __skb_header_pointer(skb, nhoff,
>> > +                                                        sizeof(_keyid),
>> > +                                                        data, hlen,
>> > +                                                        &_keyid);
>> > +                           if (!keyid)
>> > +                                   return false;
>> > +
>> > +                           flow->extra_entropy ^= *keyid;
>>
>>
>> This probably should not please sparse ?
>>
>> make C=2 CF=-D__CHECK_ENDIAN__  net/core/flow_dissector.o
>>
>
> BTW, you never addressed prior feedback about sparse errors :
>
> include/linux/skbuff.h:3054:24: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3054:24:    expected restricted __sum16
> include/linux/skbuff.h:3054:24:    got int
> include/linux/skbuff.h:3054:24: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3054:24:    expected restricted __sum16
> include/linux/skbuff.h:3054:24:    got int
> include/linux/skbuff.h:3362:14: warning: incorrect type in assignment (different base types)
> include/linux/skbuff.h:3362:14:    expected unsigned short [unsigned] [usertype] csum
> include/linux/skbuff.h:3362:14:    got restricted __sum16
> include/linux/skbuff.h:3367:16: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3367:16:    expected restricted __sum16
> include/linux/skbuff.h:3367:16:    got unsigned short [unsigned] [usertype] csum
>
> commit 7e2b10c1e52ca37f ("net: Support for multiple checksums with gso")
>
Hi Eric,

I don't see these errors with sparse. How are you running it?

Thanks,
Tom

> Thanks
>
>

Re: [PATCH v2 net-next 5/5] net: Add GRE keyid in flow_keys

[Eric Dumazet]

On Tue, 2015-05-12 at 10:51 -0400, Tom Herbert wrote:

> 
> I don't see these errors with sparse. How are you running it?
> 
> Thanks,
> Tom

root@edumazet-glaptop2:/usr/src/net-next# sparse --version
0.4.5-rc1
root@edumazet-glaptop2:/usr/src/net-next# make C=2 CF=-D__CHECK_ENDIAN__
M=net/ipv4
  CHECK   net/ipv4/route.c
  CHECK   net/ipv4/inetpeer.c
  CHECK   net/ipv4/protocol.c
  CHECK   net/ipv4/ip_input.c
  CHECK   net/ipv4/ip_fragment.c
  CHECK   net/ipv4/ip_forward.c
  CHECK   net/ipv4/ip_options.c
  CHECK   net/ipv4/ip_output.c
...
  CHECK   net/ipv4/tcp_offload.c
net/ipv4/tcp_offload.c:139:60: warning: incorrect type in argument 2
(different base types)
net/ipv4/tcp_offload.c:139:60:    expected restricted __wsum [usertype]
res
net/ipv4/tcp_offload.c:139:60:    got fouled restricted __sum16
include/linux/skbuff.h:3362:14: warning: incorrect type in assignment
(different base types)
include/linux/skbuff.h:3362:14:    expected unsigned short [unsigned]
[usertype] csum
include/linux/skbuff.h:3362:14:    got restricted __sum16
include/linux/skbuff.h:3367:16: warning: incorrect type in return
expression (different base types)
include/linux/skbuff.h:3367:16:    expected restricted __sum16
include/linux/skbuff.h:3367:16:    got unsigned short [unsigned]
[usertype] csum
net/ipv4/tcp_offload.c:173:52: warning: incorrect type in argument 2
(different base types)
net/ipv4/tcp_offload.c:173:52:    expected restricted __wsum [usertype]
res
net/ipv4/tcp_offload.c:173:52:    got fouled restricted __sum16
include/linux/skbuff.h:3362:14: warning: incorrect type in assignment
(different base types)
include/linux/skbuff.h:3362:14:    expected unsigned short [unsigned]
[usertype] csum
include/linux/skbuff.h:3362:14:    got restricted __sum16
include/linux/skbuff.h:3367:16: warning: incorrect type in return
expression (different base types)
include/linux/skbuff.h:3367:16:    expected restricted __sum16
include/linux/skbuff.h:3367:16:    got unsigned short [unsigned]
[usertype] csum
  CHECK   net/ipv4/gre_offload.c
include/linux/skbuff.h:3362:14: warning: incorrect type in assignment
(different base types)
include/linux/skbuff.h:3362:14:    expected unsigned short [unsigned]
[usertype] csum
include/linux/skbuff.h:3362:14:    got restricted __sum16
include/linux/skbuff.h:3367:16: warning: incorrect type in return
expression (different base types)
include/linux/skbuff.h:3367:16:    expected restricted __sum16
include/linux/skbuff.h:3367:16:    got unsigned short [unsigned]
[usertype] csum

[PATCH net-next] net: fix two sparse errors

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

First one in __skb_checksum_validate_complete() fixes the following
(and other callers)

make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
  CHECK   net/ipv4/tcp_ipv4.c
include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
include/linux/skbuff.h:3052:24:    expected restricted __sum16
include/linux/skbuff.h:3052:24:    got int

Second is fixing gso_make_checksum() :

  CHECK   net/ipv4/gre_offload.c
include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
include/linux/skbuff.h:3360:14:    got restricted __sum16
include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
include/linux/skbuff.h:3365:16:    expected restricted __sum16
include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum


Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
Signed-off-by: Eric Dumazet <edumazet@google.com>
CC: Tom Herbert <tom@herbertland.com>
---
 include/linux/skbuff.h |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f83aa6568cbf5e1794fdb1a8da28642ea417cc76..b57eebfb67e06dc90addfe5f2fea9d913b2121cc 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3051,7 +3051,7 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
 		}
 	} else if (skb->csum_bad) {
 		/* ip_summed == CHECKSUM_NONE in this case */
-		return 1;
+		return (__force __sum16)1;
 	}
 
 	skb->csum = psum;
@@ -3353,15 +3353,14 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
 static inline __sum16 gso_make_checksum(struct sk_buff *skb, __wsum res)
 {
 	int plen = SKB_GSO_CB(skb)->csum_start - skb_headroom(skb) -
-	    skb_transport_offset(skb);
-	__u16 csum;
+		   skb_transport_offset(skb);
+	__wsum partial;
 
-	csum = csum_fold(csum_partial(skb_transport_header(skb),
-				      plen, skb->csum));
+	partial = csum_partial(skb_transport_header(skb), plen, skb->csum);
 	skb->csum = res;
 	SKB_GSO_CB(skb)->csum_start -= plen;
 
-	return csum;
+	return csum_fold(partial);
 }
 
 static inline bool skb_is_gso(const struct sk_buff *skb)

Re: [PATCH net-next] net: fix two sparse errors

[Sabrina Dubroca]

Hi Eric,

2015-05-15, 05:48:07 -0700, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> First one in __skb_checksum_validate_complete() fixes the following
> (and other callers)
> 
> make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
>   CHECK   net/ipv4/tcp_ipv4.c
> include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3052:24:    expected restricted __sum16
> include/linux/skbuff.h:3052:24:    got int
> 
> Second is fixing gso_make_checksum() :
> 
>   CHECK   net/ipv4/gre_offload.c
> include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
> include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
> include/linux/skbuff.h:3360:14:    got restricted __sum16
> include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3365:16:    expected restricted __sum16
> include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum
> 
> 
> Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
> Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> CC: Tom Herbert <tom@herbertland.com>
> ---

That's very similar to what I submitted in February:
https://patchwork.ozlabs.org/patch/437332/

I have a patch to make __skb_checksum_validate_complete return a bool,
since that's all the callers care about (it also needs modifications
at some call sites, not included here).

After that, maybe we should also reverse the logic so that "validate"
functions return true when the csum is valid.



diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f83aa6568cbf..23dc1ccb28f0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3034,24 +3034,23 @@ static inline void skb_checksum_complete_unset(struct sk_buff *skb)
 /* Validate (init) checksum based on checksum complete.
  *
  * Return values:
- *   0: checksum is validated or try to in skb_checksum_complete. In the latter
- *	case the ip_summed will not be CHECKSUM_UNNECESSARY and the pseudo
- *	checksum is stored in skb->csum for use in __skb_checksum_complete
- *   non-zero: value of invalid checksum
- *
+ *   false: checksum is validated or try to in skb_checksum_complete. In the
+ *     latter case the ip_summed will not be CHECKSUM_UNNECESSARY and the
+ *     pseudo checksum is stored in skb->csum for use in __skb_checksum_complete
+ *   true: invalid checksum
  */
-static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
-						       bool complete,
-						       __wsum psum)
+static inline bool __skb_checksum_validate_complete(struct sk_buff *skb,
+						    bool complete,
+						    __wsum psum)
 {
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
 		if (!csum_fold(csum_add(psum, skb->csum))) {
 			skb->csum_valid = 1;
-			return 0;
+			return false;
 		}
 	} else if (skb->csum_bad) {
 		/* ip_summed == CHECKSUM_NONE in this case */
-		return 1;
+		return true;
 	}
 
 	skb->csum = psum;
@@ -3061,10 +3060,10 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
 
 		csum = __skb_checksum_complete(skb);
 		skb->csum_valid = !csum;
-		return csum;
+		return !!csum;
 	}
 
-	return 0;
+	return false;
 }
 
 static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto)
@@ -3079,13 +3078,13 @@ static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto)
  * pseudo header.
  *
  * Return values:
- *   0: checksum is validated or try to in skb_checksum_complete
- *   non-zero: value of invalid checksum
+ *   false: checksum is validated or try to in skb_checksum_complete
+ *   true: invalid checksum
  */
 #define __skb_checksum_validate(skb, proto, complete,			\
 				zero_okay, check, compute_pseudo)	\
 ({									\
-	__sum16 __ret = 0;						\
+	bool __ret = false;						\
 	skb->csum_valid = 0;						\
 	if (__skb_checksum_validate_needed(skb, zero_okay, check))	\
 		__ret = __skb_checksum_validate_complete(skb,		\




-- 
Sabrina

Re: [PATCH net-next] net: fix two sparse errors

[Eric Dumazet]

On Fri, 2015-05-15 at 16:35 +0200, Sabrina Dubroca wrote:
> Hi Eric,
> 
> 2015-05-15, 05:48:07 -0700, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > First one in __skb_checksum_validate_complete() fixes the following
> > (and other callers)
> > 
> > make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
> >   CHECK   net/ipv4/tcp_ipv4.c
> > include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
> > include/linux/skbuff.h:3052:24:    expected restricted __sum16
> > include/linux/skbuff.h:3052:24:    got int
> > 
> > Second is fixing gso_make_checksum() :
> > 
> >   CHECK   net/ipv4/gre_offload.c
> > include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
> > include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
> > include/linux/skbuff.h:3360:14:    got restricted __sum16
> > include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
> > include/linux/skbuff.h:3365:16:    expected restricted __sum16
> > include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum
> > 
> > 
> > Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
> > Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > CC: Tom Herbert <tom@herbertland.com>
> > ---
> 
> That's very similar to what I submitted in February:
> https://patchwork.ozlabs.org/patch/437332/

Well, given you did not address David concerns, and Tom apparently does
not know how to use sparse, I finally gave up.

Re: [PATCH net-next] net: fix two sparse errors

[Tom Herbert]

On Fri, May 15, 2015 at 8:48 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> First one in __skb_checksum_validate_complete() fixes the following
> (and other callers)
>
> make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
>   CHECK   net/ipv4/tcp_ipv4.c
> include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3052:24:    expected restricted __sum16
> include/linux/skbuff.h:3052:24:    got int
>
> Second is fixing gso_make_checksum() :
>
>   CHECK   net/ipv4/gre_offload.c
> include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
> include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
> include/linux/skbuff.h:3360:14:    got restricted __sum16
> include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3365:16:    expected restricted __sum16
> include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum
>
>
> Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
> Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> CC: Tom Herbert <tom@herbertland.com>


Acked-by: Tom Herbert <tom@herbertland.com>

> ---
>  include/linux/skbuff.h |   11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f83aa6568cbf5e1794fdb1a8da28642ea417cc76..b57eebfb67e06dc90addfe5f2fea9d913b2121cc 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3051,7 +3051,7 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
>                 }
>         } else if (skb->csum_bad) {
>                 /* ip_summed == CHECKSUM_NONE in this case */
> -               return 1;
> +               return (__force __sum16)1;
>         }
>
>         skb->csum = psum;
> @@ -3353,15 +3353,14 @@ static inline int gso_pskb_expand_head(struct sk_buff *skb, int extra)
>  static inline __sum16 gso_make_checksum(struct sk_buff *skb, __wsum res)
>  {
>         int plen = SKB_GSO_CB(skb)->csum_start - skb_headroom(skb) -
> -           skb_transport_offset(skb);
> -       __u16 csum;
> +                  skb_transport_offset(skb);
> +       __wsum partial;
>
> -       csum = csum_fold(csum_partial(skb_transport_header(skb),
> -                                     plen, skb->csum));
> +       partial = csum_partial(skb_transport_header(skb), plen, skb->csum);
>         skb->csum = res;
>         SKB_GSO_CB(skb)->csum_start -= plen;
>
> -       return csum;
> +       return csum_fold(partial);
>  }
>
>  static inline bool skb_is_gso(const struct sk_buff *skb)
>
>

Re: [PATCH net-next] net: fix two sparse errors

[Sabrina Dubroca]

2015-05-15, 07:52:37 -0700, Eric Dumazet wrote:
> On Fri, 2015-05-15 at 16:35 +0200, Sabrina Dubroca wrote:
> > Hi Eric,
> > 
> > 2015-05-15, 05:48:07 -0700, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > > 
> > > First one in __skb_checksum_validate_complete() fixes the following
> > > (and other callers)
> > > 
> > > make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
> > >   CHECK   net/ipv4/tcp_ipv4.c
> > > include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
> > > include/linux/skbuff.h:3052:24:    expected restricted __sum16
> > > include/linux/skbuff.h:3052:24:    got int
> > > 
> > > Second is fixing gso_make_checksum() :
> > > 
> > >   CHECK   net/ipv4/gre_offload.c
> > > include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
> > > include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
> > > include/linux/skbuff.h:3360:14:    got restricted __sum16
> > > include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
> > > include/linux/skbuff.h:3365:16:    expected restricted __sum16
> > > include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum
> > > 
> > > 
> > > Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
> > > Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > CC: Tom Herbert <tom@herbertland.com>
> > > ---
> > 
> > That's very similar to what I submitted in February:
> > https://patchwork.ozlabs.org/patch/437332/
> 
> Well, given you did not address David concerns, and Tom apparently does
> not know how to use sparse, I finally gave up.

Sorry :(

But I don't see how doing the same thing for
__skb_checksum_validate_complete will address David's concerns.

I really have no problem with your submission, I just thought that it
might not satisfy David.

Re: [PATCH net-next] net: fix two sparse errors

[Tom Herbert]

On Fri, May 15, 2015 at 10:35 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> Hi Eric,
>
> 2015-05-15, 05:48:07 -0700, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> First one in __skb_checksum_validate_complete() fixes the following
>> (and other callers)
>>
>> make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
>>   CHECK   net/ipv4/tcp_ipv4.c
>> include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
>> include/linux/skbuff.h:3052:24:    expected restricted __sum16
>> include/linux/skbuff.h:3052:24:    got int
>>
>> Second is fixing gso_make_checksum() :
>>
>>   CHECK   net/ipv4/gre_offload.c
>> include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
>> include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
>> include/linux/skbuff.h:3360:14:    got restricted __sum16
>> include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
>> include/linux/skbuff.h:3365:16:    expected restricted __sum16
>> include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum
>>
>>
>> Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
>> Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> CC: Tom Herbert <tom@herbertland.com>
>> ---
>
> That's very similar to what I submitted in February:
> https://patchwork.ozlabs.org/patch/437332/
>
> I have a patch to make __skb_checksum_validate_complete return a bool,
> since that's all the callers care about (it also needs modifications
> at some call sites, not included here).
>
> After that, maybe we should also reverse the logic so that "validate"
> functions return true when the csum is valid.
>
>
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index f83aa6568cbf..23dc1ccb28f0 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3034,24 +3034,23 @@ static inline void skb_checksum_complete_unset(struct sk_buff *skb)
>  /* Validate (init) checksum based on checksum complete.
>   *
>   * Return values:
> - *   0: checksum is validated or try to in skb_checksum_complete. In the latter
> - *     case the ip_summed will not be CHECKSUM_UNNECESSARY and the pseudo
> - *     checksum is stored in skb->csum for use in __skb_checksum_complete
> - *   non-zero: value of invalid checksum
> - *
> + *   false: checksum is validated or try to in skb_checksum_complete. In the
> + *     latter case the ip_summed will not be CHECKSUM_UNNECESSARY and the
> + *     pseudo checksum is stored in skb->csum for use in __skb_checksum_complete
> + *   true: invalid checksum
>   */
> -static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
> -                                                      bool complete,
> -                                                      __wsum psum)
> +static inline bool __skb_checksum_validate_complete(struct sk_buff *skb,
> +                                                   bool complete,
> +                                                   __wsum psum)
>  {
>         if (skb->ip_summed == CHECKSUM_COMPLETE) {
>                 if (!csum_fold(csum_add(psum, skb->csum))) {
>                         skb->csum_valid = 1;
> -                       return 0;
> +                       return false;
>                 }
>         } else if (skb->csum_bad) {
>                 /* ip_summed == CHECKSUM_NONE in this case */
> -               return 1;
> +               return true;
>         }
>
>         skb->csum = psum;
> @@ -3061,10 +3060,10 @@ static inline __sum16 __skb_checksum_validate_complete(struct sk_buff *skb,
>
>                 csum = __skb_checksum_complete(skb);
>                 skb->csum_valid = !csum;
> -               return csum;
> +               return !!csum;
>         }
>
> -       return 0;
> +       return false;

Hi Sabrina,

We returned the checksum value since that may be of interest to the
caller at some point.


>  }
>
>  static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto)
> @@ -3079,13 +3078,13 @@ static inline __wsum null_compute_pseudo(struct sk_buff *skb, int proto)
>   * pseudo header.
>   *
>   * Return values:
> - *   0: checksum is validated or try to in skb_checksum_complete
> - *   non-zero: value of invalid checksum
> + *   false: checksum is validated or try to in skb_checksum_complete
> + *   true: invalid checksum
>   */
>  #define __skb_checksum_validate(skb, proto, complete,                  \
>                                 zero_okay, check, compute_pseudo)       \
>  ({                                                                     \
> -       __sum16 __ret = 0;                                              \
> +       bool __ret = false;                                             \
>         skb->csum_valid = 0;                                            \
>         if (__skb_checksum_validate_needed(skb, zero_okay, check))      \
>                 __ret = __skb_checksum_validate_complete(skb,           \
>
>
>
>
> --
> Sabrina

Re: [PATCH net-next] net: fix two sparse errors

[Sabrina Dubroca]

Hi Tom,

2015-05-15, 11:27:25 -0400, Tom Herbert wrote:
> Hi Sabrina,
> 
> We returned the checksum value since that may be of interest to the
> caller at some point.

Hmm, I thought "we may need that some day" was not a valid argument?


Thanks,
-- 
Sabrina

Re: [PATCH net-next] net: fix two sparse errors

[Eric Dumazet]

On Fri, 2015-05-15 at 17:23 +0200, Sabrina Dubroca wrote:

> Sorry :(

No big deal, we are dealing with false positives here.

> 
> But I don't see how doing the same thing for
> __skb_checksum_validate_complete will address David's concerns.
> 
> I really have no problem with your submission, I just thought that it
> might not satisfy David.

The real problem/bug is with the flow label thing.

This needs a real fix.

Re: [PATCH net-next] net: fix two sparse errors

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Fri, 15 May 2015 17:54:41 +0200

> Hi Tom,
> 
> 2015-05-15, 11:27:25 -0400, Tom Herbert wrote:
>> Hi Sabrina,
>> 
>> We returned the checksum value since that may be of interest to the
>> caller at some point.
> 
> Hmm, I thought "we may need that some day" was not a valid argument?

I think Tom's time frame for "some day" is really small and he
should be accomodated.

Re: [PATCH net-next] net: fix two sparse errors

[Sabrina Dubroca]

2015-05-15, 12:19:42 -0400, David Miller wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Fri, 15 May 2015 17:54:41 +0200
> 
> > Hi Tom,
> > 
> > 2015-05-15, 11:27:25 -0400, Tom Herbert wrote:
> >> Hi Sabrina,
> >> 
> >> We returned the checksum value since that may be of interest to the
> >> caller at some point.
> > 
> > Hmm, I thought "we may need that some day" was not a valid argument?
> 
> I think Tom's time frame for "some day" is really small and he
> should be accomodated.

Okay. Sorry again.

-- 
Sabrina

Re: [PATCH net-next] net: fix two sparse errors

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 May 2015 05:48:07 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> First one in __skb_checksum_validate_complete() fixes the following
> (and other callers)
> 
> make C=2 CF=-D__CHECK_ENDIAN__ net/ipv4/tcp_ipv4.o
>   CHECK   net/ipv4/tcp_ipv4.c
> include/linux/skbuff.h:3052:24: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3052:24:    expected restricted __sum16
> include/linux/skbuff.h:3052:24:    got int
> 
> Second is fixing gso_make_checksum() :
> 
>   CHECK   net/ipv4/gre_offload.c
> include/linux/skbuff.h:3360:14: warning: incorrect type in assignment (different base types)
> include/linux/skbuff.h:3360:14:    expected unsigned short [unsigned] [usertype] csum
> include/linux/skbuff.h:3360:14:    got restricted __sum16
> include/linux/skbuff.h:3365:16: warning: incorrect type in return expression (different base types)
> include/linux/skbuff.h:3365:16:    expected restricted __sum16
> include/linux/skbuff.h:3365:16:    got unsigned short [unsigned] [usertype] csum
> 
> 
> Fixes: 5a21232983aa7 ("net: Support for csum_bad in skbuff")
> Fixes: 7e2b10c1e52ca ("net: Support for multiple checksums with gso")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.