[PATCH v2 net-next 0/2] net/sched: cls_flower: Support matching on ICMP

[PATCH v2 net-next 0/2] net/sched: cls_flower: Support matching on ICMP

[Simon Horman]

Hi,

this series add supports for matching on ICMP type and code to cls_flower.
This is modeled on existing support for matching on L4 ports. The updates
to the dissector are intended to allow for code and storage re-use.

Changes v1->v2:
* Add all flow dissector helpers in first patch

Change RFC->v1:
* Drop RFC designation after positive review from Jiri

Simon Horman (2):
  flow dissector: ICMP support
  net/sched: cls_flower: Support matching on ICMP type and code

 drivers/net/bonding/bond_main.c |  6 +++--
 include/linux/skbuff.h          |  5 +++++
 include/net/flow_dissector.h    | 50 ++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/pkt_cls.h    | 10 +++++++++
 net/core/flow_dissector.c       | 34 +++++++++++++++++++++++++---
 net/sched/cls_flow.c            |  4 ++--
 net/sched/cls_flower.c          | 42 ++++++++++++++++++++++++++++++++++
 7 files changed, 141 insertions(+), 10 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

[PATCH v2 net-next 1/2] flow dissector: ICMP support

[Simon Horman]

Allow dissection of ICMP(V6) type and code. This re-uses transport layer
port dissection code as although ICMP is not a transport protocol and their
type and code are not ports this allows sharing of both code and storage.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 drivers/net/bonding/bond_main.c |  6 +++--
 include/linux/skbuff.h          |  5 +++++
 include/net/flow_dissector.h    | 50 ++++++++++++++++++++++++++++++++++++++---
 net/core/flow_dissector.c       | 34 +++++++++++++++++++++++++---
 net/sched/cls_flow.c            |  4 ++--
 5 files changed, 89 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8029dd4912b6..a6f75cfb2bf7 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3181,7 +3181,8 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 	} else {
 		return false;
 	}
-	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
+	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 &&
+	    proto >= 0 && !skb_flow_is_icmp_any(skb, proto))
 		fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
 
 	return true;
@@ -3209,7 +3210,8 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
 		return bond_eth_hash(skb);
 
 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
-	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23)
+	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23 ||
+	    flow_keys_are_icmp_any(&flow))
 		hash = bond_eth_hash(skb);
 	else
 		hash = (__force u32)flow.ports.ports;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 9c535fbccf2c..44a8f69a9198 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1094,6 +1094,11 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
 __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 			    void *data, int hlen_proto);
 
+static inline bool skb_flow_is_icmp_any(const struct sk_buff *skb, u8 ip_proto)
+{
+	return flow_protos_are_icmp_any(skb->protocol, ip_proto);
+}
+
 static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
 					int thoff, u8 ip_proto)
 {
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index c4f31666afd2..5540dfa18872 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -2,6 +2,7 @@
 #define _NET_FLOW_DISSECTOR_H
 
 #include <linux/types.h>
+#include <linux/in.h>
 #include <linux/in6.h>
 #include <uapi/linux/if_ether.h>
 
@@ -89,10 +90,15 @@ struct flow_dissector_key_addrs {
 };
 
 /**
- * flow_dissector_key_tp_ports:
- *	@ports: port numbers of Transport header
+ * flow_dissector_key_ports:
+ *	@ports: port numbers of Transport header or
+ *		type and code of ICMP header
+ *		ports: source (high) and destination (low) port numbers
  *		src: source port number
  *		dst: destination port number
+ *		icmp: ICMP type (high) and code (low)
+ *		type: ICMP type
+ *		type: ICMP code
  */
 struct flow_dissector_key_ports {
 	union {
@@ -101,6 +107,11 @@ struct flow_dissector_key_ports {
 			__be16 src;
 			__be16 dst;
 		};
+		__be16 icmp;
+		struct {
+			u8 type;
+			u8 code;
+		};
 	};
 };
 
@@ -188,9 +199,42 @@ struct flow_keys_digest {
 void make_flow_keys_digest(struct flow_keys_digest *digest,
 			   const struct flow_keys *flow);
 
+static inline bool flow_protos_are_icmpv4(__be16 n_proto, u8 ip_proto)
+{
+	return n_proto == htons(ETH_P_IP) && ip_proto == IPPROTO_ICMP;
+}
+
+static inline bool flow_protos_are_icmpv6(__be16 n_proto, u8 ip_proto)
+{
+	return n_proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_ICMPV6;
+}
+
+static inline bool flow_protos_are_icmp_any(__be16 n_proto, u8 ip_proto)
+{
+	return flow_protos_are_icmpv4(n_proto, ip_proto) ||
+		flow_protos_are_icmpv6(n_proto, ip_proto);
+}
+
+static inline bool flow_basic_key_is_icmpv4(const struct flow_dissector_key_basic *basic)
+{
+	return flow_protos_are_icmpv4(basic->n_proto, basic->ip_proto);
+}
+
+static inline bool flow_basic_key_is_icmpv6(const struct flow_dissector_key_basic *basic)
+{
+	return flow_protos_are_icmpv6(basic->n_proto, basic->ip_proto);
+}
+
+static inline bool flow_keys_are_icmp_any(const struct flow_keys *keys)
+{
+	return flow_protos_are_icmp_any(keys->basic.n_proto,
+					keys->basic.ip_proto);
+}
+
 static inline bool flow_keys_have_l4(const struct flow_keys *keys)
 {
-	return (keys->ports.ports || keys->tags.flow_label);
+	return (!flow_keys_are_icmp_any(keys) && keys->ports.ports) ||
+		keys->tags.flow_label;
 }
 
 u32 flow_hash_from_keys(struct flow_keys *keys);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 1eb6f949e5b2..0584b4bb4390 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -58,6 +58,28 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
 EXPORT_SYMBOL(skb_flow_dissector_init);
 
 /**
+ * skb_flow_get_be16 - extract be16 entity
+ * @skb: sk_buff to extract from
+ * @poff: offset to extract at
+ * @data: raw buffer pointer to the packet
+ * @hlen: packet header length
+ *
+ * The function will try to retrieve a be32 entity at
+ * offset poff
+ */
+__be16 skb_flow_get_be16(const struct sk_buff *skb, int poff, void *data,
+			 int hlen)
+{
+	__be16 *u, _u;
+
+	u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u);
+	if (u)
+		return *u;
+
+	return 0;
+}
+
+/**
  * __skb_flow_get_ports - extract the upper layer ports and return them
  * @skb: sk_buff to extract the ports from
  * @thoff: transport header offset
@@ -542,8 +564,13 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		key_ports = skb_flow_dissector_target(flow_dissector,
 						      FLOW_DISSECTOR_KEY_PORTS,
 						      target_container);
-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
-							data, hlen);
+		if (flow_protos_are_icmp_any(proto, ip_proto))
+			key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
+							    hlen);
+		else
+			key_ports->ports = __skb_flow_get_ports(skb, nhoff,
+								ip_proto, data,
+								hlen);
 	}
 
 out_good:
@@ -718,7 +745,8 @@ void make_flow_keys_digest(struct flow_keys_digest *digest,
 
 	data->n_proto = flow->basic.n_proto;
 	data->ip_proto = flow->basic.ip_proto;
-	data->ports = flow->ports.ports;
+	if (flow_keys_have_l4(flow))
+		data->ports = flow->ports.ports;
 	data->src = flow->addrs.v4addrs.src;
 	data->dst = flow->addrs.v4addrs.dst;
 }
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index e39672394c7b..a1a7ae71aa62 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -96,7 +96,7 @@ static u32 flow_get_proto(const struct sk_buff *skb,
 static u32 flow_get_proto_src(const struct sk_buff *skb,
 			      const struct flow_keys *flow)
 {
-	if (flow->ports.ports)
+	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
 		return ntohs(flow->ports.src);
 
 	return addr_fold(skb->sk);
@@ -105,7 +105,7 @@ static u32 flow_get_proto_src(const struct sk_buff *skb,
 static u32 flow_get_proto_dst(const struct sk_buff *skb,
 			      const struct flow_keys *flow)
 {
-	if (flow->ports.ports)
+	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
 		return ntohs(flow->ports.dst);
 
 	return addr_fold(skb_dst(skb)) ^ (__force u16) tc_skb_protocol(skb);
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Jiri Pirko]

Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
>port dissection code as although ICMP is not a transport protocol and their
>type and code are not ports this allows sharing of both code and storage.
>
>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>---
> drivers/net/bonding/bond_main.c |  6 +++--
> include/linux/skbuff.h          |  5 +++++
> include/net/flow_dissector.h    | 50 ++++++++++++++++++++++++++++++++++++++---
> net/core/flow_dissector.c       | 34 +++++++++++++++++++++++++---
> net/sched/cls_flow.c            |  4 ++--
> 5 files changed, 89 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 8029dd4912b6..a6f75cfb2bf7 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3181,7 +3181,8 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
> 	} else {
> 		return false;
> 	}
>-	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
>+	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 &&
>+	    proto >= 0 && !skb_flow_is_icmp_any(skb, proto))
> 		fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
> 
> 	return true;
>@@ -3209,7 +3210,8 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> 		return bond_eth_hash(skb);
> 
> 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
>-	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23)
>+	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23 ||
>+	    flow_keys_are_icmp_any(&flow))
> 		hash = bond_eth_hash(skb);
> 	else
> 		hash = (__force u32)flow.ports.ports;
>diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>index 9c535fbccf2c..44a8f69a9198 100644
>--- a/include/linux/skbuff.h
>+++ b/include/linux/skbuff.h
>@@ -1094,6 +1094,11 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
> __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
> 			    void *data, int hlen_proto);
> 
>+static inline bool skb_flow_is_icmp_any(const struct sk_buff *skb, u8 ip_proto)
>+{
>+	return flow_protos_are_icmp_any(skb->protocol, ip_proto);
>+}
>+
> static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
> 					int thoff, u8 ip_proto)
> {
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index c4f31666afd2..5540dfa18872 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -2,6 +2,7 @@
> #define _NET_FLOW_DISSECTOR_H
> 
> #include <linux/types.h>
>+#include <linux/in.h>
> #include <linux/in6.h>
> #include <uapi/linux/if_ether.h>
> 
>@@ -89,10 +90,15 @@ struct flow_dissector_key_addrs {
> };
> 
> /**
>- * flow_dissector_key_tp_ports:
>- *	@ports: port numbers of Transport header
>+ * flow_dissector_key_ports:
>+ *	@ports: port numbers of Transport header or
>+ *		type and code of ICMP header
>+ *		ports: source (high) and destination (low) port numbers
>  *		src: source port number
>  *		dst: destination port number
>+ *		icmp: ICMP type (high) and code (low)
>+ *		type: ICMP type
>+ *		type: ICMP code
>  */
> struct flow_dissector_key_ports {
> 	union {
>@@ -101,6 +107,11 @@ struct flow_dissector_key_ports {
> 			__be16 src;
> 			__be16 dst;
> 		};
>+		__be16 icmp;
>+		struct {
>+			u8 type;
>+			u8 code;
>+		};

Digging into this a bit more. I think it would be much nice not to mix
up l4 ports and icmp stuff.

How about to have FLOW_DISSECTOR_KEY_ICMP
and
struct flow_dissector_key_icmp {
	u8 type;
	u8 code;
};

The you can make this structure and struct flow_dissector_key_ports into
an union in struct flow_keys.

Looks much cleaner to me.



> 	};
> };
> 
>@@ -188,9 +199,42 @@ struct flow_keys_digest {
> void make_flow_keys_digest(struct flow_keys_digest *digest,
> 			   const struct flow_keys *flow);
> 
>+static inline bool flow_protos_are_icmpv4(__be16 n_proto, u8 ip_proto)
>+{
>+	return n_proto == htons(ETH_P_IP) && ip_proto == IPPROTO_ICMP;
>+}
>+
>+static inline bool flow_protos_are_icmpv6(__be16 n_proto, u8 ip_proto)
>+{
>+	return n_proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_ICMPV6;
>+}
>+
>+static inline bool flow_protos_are_icmp_any(__be16 n_proto, u8 ip_proto)
>+{
>+	return flow_protos_are_icmpv4(n_proto, ip_proto) ||
>+		flow_protos_are_icmpv6(n_proto, ip_proto);
>+}
>+
>+static inline bool flow_basic_key_is_icmpv4(const struct flow_dissector_key_basic *basic)
>+{
>+	return flow_protos_are_icmpv4(basic->n_proto, basic->ip_proto);
>+}
>+
>+static inline bool flow_basic_key_is_icmpv6(const struct flow_dissector_key_basic *basic)
>+{
>+	return flow_protos_are_icmpv6(basic->n_proto, basic->ip_proto);
>+}
>+
>+static inline bool flow_keys_are_icmp_any(const struct flow_keys *keys)
>+{
>+	return flow_protos_are_icmp_any(keys->basic.n_proto,
>+					keys->basic.ip_proto);
>+}
>+
> static inline bool flow_keys_have_l4(const struct flow_keys *keys)
> {
>-	return (keys->ports.ports || keys->tags.flow_label);
>+	return (!flow_keys_are_icmp_any(keys) && keys->ports.ports) ||
>+		keys->tags.flow_label;
> }
> 
> u32 flow_hash_from_keys(struct flow_keys *keys);
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 1eb6f949e5b2..0584b4bb4390 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -58,6 +58,28 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
> EXPORT_SYMBOL(skb_flow_dissector_init);
> 
> /**
>+ * skb_flow_get_be16 - extract be16 entity
>+ * @skb: sk_buff to extract from
>+ * @poff: offset to extract at
>+ * @data: raw buffer pointer to the packet
>+ * @hlen: packet header length
>+ *
>+ * The function will try to retrieve a be32 entity at
>+ * offset poff
>+ */
>+__be16 skb_flow_get_be16(const struct sk_buff *skb, int poff, void *data,
>+			 int hlen)
>+{
>+	__be16 *u, _u;
>+
>+	u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u);
>+	if (u)
>+		return *u;
>+
>+	return 0;
>+}
>+
>+/**
>  * __skb_flow_get_ports - extract the upper layer ports and return them
>  * @skb: sk_buff to extract the ports from
>  * @thoff: transport header offset
>@@ -542,8 +564,13 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 		key_ports = skb_flow_dissector_target(flow_dissector,
> 						      FLOW_DISSECTOR_KEY_PORTS,
> 						      target_container);
>-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
>-							data, hlen);
>+		if (flow_protos_are_icmp_any(proto, ip_proto))
>+			key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
>+							    hlen);
>+		else
>+			key_ports->ports = __skb_flow_get_ports(skb, nhoff,
>+								ip_proto, data,
>+								hlen);
> 	}
> 
> out_good:
>@@ -718,7 +745,8 @@ void make_flow_keys_digest(struct flow_keys_digest *digest,
> 
> 	data->n_proto = flow->basic.n_proto;
> 	data->ip_proto = flow->basic.ip_proto;
>-	data->ports = flow->ports.ports;
>+	if (flow_keys_have_l4(flow))
>+		data->ports = flow->ports.ports;
> 	data->src = flow->addrs.v4addrs.src;
> 	data->dst = flow->addrs.v4addrs.dst;
> }
>diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
>index e39672394c7b..a1a7ae71aa62 100644
>--- a/net/sched/cls_flow.c
>+++ b/net/sched/cls_flow.c
>@@ -96,7 +96,7 @@ static u32 flow_get_proto(const struct sk_buff *skb,
> static u32 flow_get_proto_src(const struct sk_buff *skb,
> 			      const struct flow_keys *flow)
> {
>-	if (flow->ports.ports)
>+	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
> 		return ntohs(flow->ports.src);
> 
> 	return addr_fold(skb->sk);
>@@ -105,7 +105,7 @@ static u32 flow_get_proto_src(const struct sk_buff *skb,
> static u32 flow_get_proto_dst(const struct sk_buff *skb,
> 			      const struct flow_keys *flow)
> {
>-	if (flow->ports.ports)
>+	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
> 		return ntohs(flow->ports.dst);
> 
> 	return addr_fold(skb_dst(skb)) ^ (__force u16) tc_skb_protocol(skb);
>-- 
>2.7.0.rc3.207.g0ac5344
>

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Tom Herbert]

On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
>>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
>>port dissection code as although ICMP is not a transport protocol and their
>>type and code are not ports this allows sharing of both code and storage.
>>
>>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>>---
>> drivers/net/bonding/bond_main.c |  6 +++--
>> include/linux/skbuff.h          |  5 +++++
>> include/net/flow_dissector.h    | 50 ++++++++++++++++++++++++++++++++++++++---
>> net/core/flow_dissector.c       | 34 +++++++++++++++++++++++++---
>> net/sched/cls_flow.c            |  4 ++--
>> 5 files changed, 89 insertions(+), 10 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 8029dd4912b6..a6f75cfb2bf7 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -3181,7 +3181,8 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
>>       } else {
>>               return false;
>>       }
>>-      if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
>>+      if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 &&
>>+          proto >= 0 && !skb_flow_is_icmp_any(skb, proto))
>>               fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
>>
>>       return true;
>>@@ -3209,7 +3210,8 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>>               return bond_eth_hash(skb);
>>
>>       if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
>>-          bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23)
>>+          bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23 ||
>>+          flow_keys_are_icmp_any(&flow))
>>               hash = bond_eth_hash(skb);
>>       else
>>               hash = (__force u32)flow.ports.ports;
>>diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>index 9c535fbccf2c..44a8f69a9198 100644
>>--- a/include/linux/skbuff.h
>>+++ b/include/linux/skbuff.h
>>@@ -1094,6 +1094,11 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
>> __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
>>                           void *data, int hlen_proto);
>>
>>+static inline bool skb_flow_is_icmp_any(const struct sk_buff *skb, u8 ip_proto)
>>+{
>>+      return flow_protos_are_icmp_any(skb->protocol, ip_proto);
>>+}
>>+
>> static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
>>                                       int thoff, u8 ip_proto)
>> {
>>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>>index c4f31666afd2..5540dfa18872 100644
>>--- a/include/net/flow_dissector.h
>>+++ b/include/net/flow_dissector.h
>>@@ -2,6 +2,7 @@
>> #define _NET_FLOW_DISSECTOR_H
>>
>> #include <linux/types.h>
>>+#include <linux/in.h>
>> #include <linux/in6.h>
>> #include <uapi/linux/if_ether.h>
>>
>>@@ -89,10 +90,15 @@ struct flow_dissector_key_addrs {
>> };
>>
>> /**
>>- * flow_dissector_key_tp_ports:
>>- *    @ports: port numbers of Transport header
>>+ * flow_dissector_key_ports:
>>+ *    @ports: port numbers of Transport header or
>>+ *            type and code of ICMP header
>>+ *            ports: source (high) and destination (low) port numbers
>>  *            src: source port number
>>  *            dst: destination port number
>>+ *            icmp: ICMP type (high) and code (low)
>>+ *            type: ICMP type
>>+ *            type: ICMP code
>>  */
>> struct flow_dissector_key_ports {
>>       union {
>>@@ -101,6 +107,11 @@ struct flow_dissector_key_ports {
>>                       __be16 src;
>>                       __be16 dst;
>>               };
>>+              __be16 icmp;
>>+              struct {
>>+                      u8 type;
>>+                      u8 code;
>>+              };
>
> Digging into this a bit more. I think it would be much nice not to mix
> up l4 ports and icmp stuff.
>
> How about to have FLOW_DISSECTOR_KEY_ICMP
> and
> struct flow_dissector_key_icmp {
>         u8 type;
>         u8 code;
> };
>
> The you can make this structure and struct flow_dissector_key_ports into
> an union in struct flow_keys.
>
> Looks much cleaner to me.
>
I agree, this patch adds to many conditionals into the fast path for
ICMP handling. Neither is there much point in using type and code as
input to the packet hash.

Tom

>
>
>>       };
>> };
>>
>>@@ -188,9 +199,42 @@ struct flow_keys_digest {
>> void make_flow_keys_digest(struct flow_keys_digest *digest,
>>                          const struct flow_keys *flow);
>>
>>+static inline bool flow_protos_are_icmpv4(__be16 n_proto, u8 ip_proto)
>>+{
>>+      return n_proto == htons(ETH_P_IP) && ip_proto == IPPROTO_ICMP;
>>+}
>>+
>>+static inline bool flow_protos_are_icmpv6(__be16 n_proto, u8 ip_proto)
>>+{
>>+      return n_proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_ICMPV6;
>>+}
>>+
>>+static inline bool flow_protos_are_icmp_any(__be16 n_proto, u8 ip_proto)
>>+{
>>+      return flow_protos_are_icmpv4(n_proto, ip_proto) ||
>>+              flow_protos_are_icmpv6(n_proto, ip_proto);
>>+}
>>+
>>+static inline bool flow_basic_key_is_icmpv4(const struct flow_dissector_key_basic *basic)
>>+{
>>+      return flow_protos_are_icmpv4(basic->n_proto, basic->ip_proto);
>>+}
>>+
>>+static inline bool flow_basic_key_is_icmpv6(const struct flow_dissector_key_basic *basic)
>>+{
>>+      return flow_protos_are_icmpv6(basic->n_proto, basic->ip_proto);
>>+}
>>+
>>+static inline bool flow_keys_are_icmp_any(const struct flow_keys *keys)
>>+{
>>+      return flow_protos_are_icmp_any(keys->basic.n_proto,
>>+                                      keys->basic.ip_proto);
>>+}
>>+
>> static inline bool flow_keys_have_l4(const struct flow_keys *keys)
>> {
>>-      return (keys->ports.ports || keys->tags.flow_label);
>>+      return (!flow_keys_are_icmp_any(keys) && keys->ports.ports) ||
>>+              keys->tags.flow_label;
>> }
>>
>> u32 flow_hash_from_keys(struct flow_keys *keys);
>>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>>index 1eb6f949e5b2..0584b4bb4390 100644
>>--- a/net/core/flow_dissector.c
>>+++ b/net/core/flow_dissector.c
>>@@ -58,6 +58,28 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
>> EXPORT_SYMBOL(skb_flow_dissector_init);
>>
>> /**
>>+ * skb_flow_get_be16 - extract be16 entity
>>+ * @skb: sk_buff to extract from
>>+ * @poff: offset to extract at
>>+ * @data: raw buffer pointer to the packet
>>+ * @hlen: packet header length
>>+ *
>>+ * The function will try to retrieve a be32 entity at
>>+ * offset poff
>>+ */
>>+__be16 skb_flow_get_be16(const struct sk_buff *skb, int poff, void *data,
>>+                       int hlen)
>>+{
>>+      __be16 *u, _u;
>>+
>>+      u = __skb_header_pointer(skb, poff, sizeof(_u), data, hlen, &_u);
>>+      if (u)
>>+              return *u;
>>+
>>+      return 0;
>>+}
>>+
>>+/**
>>  * __skb_flow_get_ports - extract the upper layer ports and return them
>>  * @skb: sk_buff to extract the ports from
>>  * @thoff: transport header offset
>>@@ -542,8 +564,13 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>>               key_ports = skb_flow_dissector_target(flow_dissector,
>>                                                     FLOW_DISSECTOR_KEY_PORTS,
>>                                                     target_container);
>>-              key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
>>-                                                      data, hlen);
>>+              if (flow_protos_are_icmp_any(proto, ip_proto))
>>+                      key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
>>+                                                          hlen);
>>+              else
>>+                      key_ports->ports = __skb_flow_get_ports(skb, nhoff,
>>+                                                              ip_proto, data,
>>+                                                              hlen);
>>       }
>>
>> out_good:
>>@@ -718,7 +745,8 @@ void make_flow_keys_digest(struct flow_keys_digest *digest,
>>
>>       data->n_proto = flow->basic.n_proto;
>>       data->ip_proto = flow->basic.ip_proto;
>>-      data->ports = flow->ports.ports;
>>+      if (flow_keys_have_l4(flow))
>>+              data->ports = flow->ports.ports;
>>       data->src = flow->addrs.v4addrs.src;
>>       data->dst = flow->addrs.v4addrs.dst;
>> }
>>diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
>>index e39672394c7b..a1a7ae71aa62 100644
>>--- a/net/sched/cls_flow.c
>>+++ b/net/sched/cls_flow.c
>>@@ -96,7 +96,7 @@ static u32 flow_get_proto(const struct sk_buff *skb,
>> static u32 flow_get_proto_src(const struct sk_buff *skb,
>>                             const struct flow_keys *flow)
>> {
>>-      if (flow->ports.ports)
>>+      if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
>>               return ntohs(flow->ports.src);
>>
>>       return addr_fold(skb->sk);
>>@@ -105,7 +105,7 @@ static u32 flow_get_proto_src(const struct sk_buff *skb,
>> static u32 flow_get_proto_dst(const struct sk_buff *skb,
>>                             const struct flow_keys *flow)
>> {
>>-      if (flow->ports.ports)
>>+      if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
>>               return ntohs(flow->ports.dst);
>>
>>       return addr_fold(skb_dst(skb)) ^ (__force u16) tc_skb_protocol(skb);
>>--
>>2.7.0.rc3.207.g0ac5344
>>

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Simon Horman]

On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
> >>port dissection code as although ICMP is not a transport protocol and their
> >>type and code are not ports this allows sharing of both code and storage.
> >>
> >>Signed-off-by: Simon Horman <simon.horman@netronome.com>

...

> > Digging into this a bit more. I think it would be much nice not to mix
> > up l4 ports and icmp stuff.
> >
> > How about to have FLOW_DISSECTOR_KEY_ICMP
> > and
> > struct flow_dissector_key_icmp {
> >         u8 type;
> >         u8 code;
> > };
> >
> > The you can make this structure and struct flow_dissector_key_ports into
> > an union in struct flow_keys.
> >
> > Looks much cleaner to me.

Hi Jiri,

I wondered about that too. The main reason that I didn't do this the first
time around is that I wasn't sure what to do to differentiate between ports
and icmp in fl_init_dissector() given that using a union would could to
mask bits being set in both if they are set in either.

I've taken a stab at addressing that below along with implementing your
suggestion. If the treatment in fl_init_dissector() is acceptable
perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS
too?

> I agree, this patch adds to many conditionals into the fast path for
> ICMP handling. Neither is there much point in using type and code as
> input to the packet hash.

Hi Tom,

I'm not sure that I follow this. A packet may be ICMP or not regardless of
if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
not. I'd appreciate some guidance here.


First-pass at implementing Jiri's suggestion follows:

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 5540dfa18872..475cd121496f 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -91,14 +91,10 @@ struct flow_dissector_key_addrs {
 
 /**
  * flow_dissector_key_ports:
- *	@ports: port numbers of Transport header or
- *		type and code of ICMP header
+ *	@ports: port numbers of Transport header
  *		ports: source (high) and destination (low) port numbers
  *		src: source port number
  *		dst: destination port number
- *		icmp: ICMP type (high) and code (low)
- *		type: ICMP type
- *		type: ICMP code
  */
 struct flow_dissector_key_ports {
 	union {
@@ -107,6 +103,18 @@ struct flow_dissector_key_ports {
 			__be16 src;
 			__be16 dst;
 		};
+	};
+};
+
+/**
+ * flow_dissector_key_icmp:
+ *	@ports: type and code of ICMP header
+ *		icmp: ICMP type (high) and code (low)
+ *		type: ICMP type
+ *		code: ICMP code
+ */
+struct flow_dissector_key_icmp {
+	union {
 		__be16 icmp;
 		struct {
 			u8 type;
@@ -133,6 +141,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_ipv4_addrs */
 	FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
 	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
+	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
 	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
 	FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
 	FLOW_DISSECTOR_KEY_VLAN, /* struct flow_dissector_key_flow_vlan */
@@ -171,7 +180,10 @@ struct flow_keys {
 	struct flow_dissector_key_tags tags;
 	struct flow_dissector_key_vlan vlan;
 	struct flow_dissector_key_keyid keyid;
-	struct flow_dissector_key_ports ports;
+	union {
+		struct flow_dissector_key_ports ports;
+		struct flow_dissector_key_icmp icmp;
+	};
 	struct flow_dissector_key_addrs addrs;
 };
 
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 0584b4bb4390..33e5fa2b3e87 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -139,6 +139,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	struct flow_dissector_key_basic *key_basic;
 	struct flow_dissector_key_addrs *key_addrs;
 	struct flow_dissector_key_ports *key_ports;
+	struct flow_dissector_key_icmp *key_icmp;
 	struct flow_dissector_key_tags *key_tags;
 	struct flow_dissector_key_vlan *key_vlan;
 	struct flow_dissector_key_keyid *key_keyid;
@@ -559,18 +560,25 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		break;
 	}
 
-	if (dissector_uses_key(flow_dissector,
-			       FLOW_DISSECTOR_KEY_PORTS)) {
-		key_ports = skb_flow_dissector_target(flow_dissector,
-						      FLOW_DISSECTOR_KEY_PORTS,
-						      target_container);
-		if (flow_protos_are_icmp_any(proto, ip_proto))
-			key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
-							    hlen);
-		else
+	if (flow_protos_are_icmp_any(proto, ip_proto)) {
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_ICMP)) {
+			key_icmp = skb_flow_dissector_target(flow_dissector,
+							     FLOW_DISSECTOR_KEY_ICMP,
+							     target_container);
+			key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data,
+							   hlen);
+		}
+	} else {
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_PORTS)) {
+			key_ports = skb_flow_dissector_target(flow_dissector,
+							      FLOW_DISSECTOR_KEY_PORTS,
+							      target_container);
 			key_ports->ports = __skb_flow_get_ports(skb, nhoff,
 								ip_proto, data,
 								hlen);
+		}
 	}
 
 out_good:
@@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
 		.offset = offsetof(struct flow_keys, ports),
 	},
 	{
+		.key_id = FLOW_DISSECTOR_KEY_ICMP,
+		.offset = offsetof(struct flow_keys, icmp),
+	},
+	{
 		.key_id = FLOW_DISSECTOR_KEY_VLAN,
 		.offset = offsetof(struct flow_keys, vlan),
 	},
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index c96b2a349779..f4ba69bd362f 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -38,7 +38,10 @@ struct fl_flow_key {
 		struct flow_dissector_key_ipv4_addrs ipv4;
 		struct flow_dissector_key_ipv6_addrs ipv6;
 	};
-	struct flow_dissector_key_ports tp;
+	union {
+		struct flow_dissector_key_ports tp;
+		struct flow_dissector_key_icmp icmp;
+	};
 	struct flow_dissector_key_keyid enc_key_id;
 	union {
 		struct flow_dissector_key_ipv4_addrs enc_ipv4;
@@ -511,19 +514,23 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 			       &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
 			       sizeof(key->tp.dst));
 	} else if (flow_basic_key_is_icmpv4(&key->basic)) {
-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
-			       sizeof(key->tp.type));
-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
-			       sizeof(key->tp.code));
+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
+			       &mask->icmp.type,
+			       TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
+			       sizeof(key->icmp.type));
+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
+			       &mask->icmp.code,
+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+			       sizeof(key->icmp.code));
 	} else if (flow_basic_key_is_icmpv6(&key->basic)) {
-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
-			       sizeof(key->tp.type));
-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
-			       sizeof(key->tp.code));
+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
+			       &mask->icmp.type,
+			       TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
+			       sizeof(key->icmp.type));
+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
+			       &mask->icmp.code,
+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+			       sizeof(key->icmp.code));
 	}
 
 	if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
@@ -609,15 +616,16 @@ static int fl_init_hashtable(struct cls_fl_head *head,
 		keys[cnt].key_id = id;						\
 		keys[cnt].offset = FL_KEY_MEMBER_OFFSET(member);		\
 		cnt++;								\
-	} while(0);
+	} while(0)
 
 #define FL_KEY_SET_IF_MASKED(mask, keys, cnt, id, member)			\
 	do {									\
 		if (FL_KEY_IS_MASKED(mask, member))				\
 			FL_KEY_SET(keys, cnt, id, member);			\
-	} while(0);
+	} while(0)
 
 static void fl_init_dissector(struct cls_fl_head *head,
+			      struct cls_fl_filter *f,
 			      struct fl_flow_mask *mask)
 {
 	struct flow_dissector_key keys[FLOW_DISSECTOR_KEY_MAX];
@@ -631,8 +639,12 @@ static void fl_init_dissector(struct cls_fl_head *head,
 			     FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);
-	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
-			     FLOW_DISSECTOR_KEY_PORTS, tp);
+	if (flow_basic_key_is_icmpv4(&f->key.basic))
+		FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
+				     FLOW_DISSECTOR_KEY_ICMP, icmp);
+	else
+		FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
+				     FLOW_DISSECTOR_KEY_PORTS, tp);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_VLAN, vlan);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
@@ -652,6 +664,7 @@ static void fl_init_dissector(struct cls_fl_head *head,
 }
 
 static int fl_check_assign_mask(struct cls_fl_head *head,
+				struct cls_fl_filter *f,
 				struct fl_flow_mask *mask)
 {
 	int err;
@@ -672,7 +685,7 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
 	memcpy(&head->mask, mask, sizeof(head->mask));
 	head->mask_assigned = true;
 
-	fl_init_dissector(head, mask);
+	fl_init_dissector(head, f, mask);
 
 	return 0;
 }
@@ -785,7 +798,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
 	if (err)
 		goto errout;
 
-	err = fl_check_assign_mask(head, &mask);
+	err = fl_check_assign_mask(head, fnew, &mask);
 	if (err)
 		goto errout;
 
@@ -1000,24 +1013,24 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 				  sizeof(key->tp.dst))))
 		goto nla_put_failure;
 	else if (flow_basic_key_is_icmpv4(&key->basic) &&
-		 (fl_dump_key_val(skb, &key->tp.type,
-				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
+		 (fl_dump_key_val(skb, &key->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
 				  TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
-				  sizeof(key->tp.type)) ||
-		  fl_dump_key_val(skb, &key->tp.code,
-				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
+				  sizeof(key->icmp.type)) ||
+		  fl_dump_key_val(skb, &key->icmp.code,
+				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->icmp.code,
 				  TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
-				  sizeof(key->tp.code))))
+				  sizeof(key->icmp.code))))
 		goto nla_put_failure;
 	else if (flow_basic_key_is_icmpv6(&key->basic) &&
-		 (fl_dump_key_val(skb, &key->tp.type,
-				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
+		 (fl_dump_key_val(skb, &key->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
 				  TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
-				  sizeof(key->tp.type)) ||
-		  fl_dump_key_val(skb, &key->tp.code,
-				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
+				  sizeof(key->icmp.type)) ||
+		  fl_dump_key_val(skb, &key->icmp.code,
+				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
 				  TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
-				  sizeof(key->tp.code))))
+				  sizeof(key->icmp.code))))
 		goto nla_put_failure;
 
 	if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Jiri Pirko]

Mon, Dec 05, 2016 at 03:23:16PM CET, simon.horman@netronome.com wrote:
>On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
>> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
>> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
>> >>port dissection code as although ICMP is not a transport protocol and their
>> >>type and code are not ports this allows sharing of both code and storage.
>> >>
>> >>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>
>...
>
>> > Digging into this a bit more. I think it would be much nice not to mix
>> > up l4 ports and icmp stuff.
>> >
>> > How about to have FLOW_DISSECTOR_KEY_ICMP
>> > and
>> > struct flow_dissector_key_icmp {
>> >         u8 type;
>> >         u8 code;
>> > };
>> >
>> > The you can make this structure and struct flow_dissector_key_ports into
>> > an union in struct flow_keys.
>> >
>> > Looks much cleaner to me.
>
>Hi Jiri,
>
>I wondered about that too. The main reason that I didn't do this the first
>time around is that I wasn't sure what to do to differentiate between ports
>and icmp in fl_init_dissector() given that using a union would could to
>mask bits being set in both if they are set in either.
>
>I've taken a stab at addressing that below along with implementing your
>suggestion. If the treatment in fl_init_dissector() is acceptable
>perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS

Looks fine.

>too?
>
>> I agree, this patch adds to many conditionals into the fast path for
>> ICMP handling. Neither is there much point in using type and code as
>> input to the packet hash.
>
>Hi Tom,
>
>I'm not sure that I follow this. A packet may be ICMP or not regardless of
>if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
>not. I'd appreciate some guidance here.
>
>
>First-pass at implementing Jiri's suggestion follows:
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index 5540dfa18872..475cd121496f 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -91,14 +91,10 @@ struct flow_dissector_key_addrs {
> 
> /**
>  * flow_dissector_key_ports:
>- *	@ports: port numbers of Transport header or
>- *		type and code of ICMP header
>+ *	@ports: port numbers of Transport header
>  *		ports: source (high) and destination (low) port numbers
>  *		src: source port number
>  *		dst: destination port number
>- *		icmp: ICMP type (high) and code (low)
>- *		type: ICMP type
>- *		type: ICMP code
>  */
> struct flow_dissector_key_ports {
> 	union {
>@@ -107,6 +103,18 @@ struct flow_dissector_key_ports {
> 			__be16 src;
> 			__be16 dst;
> 		};
>+	};
>+};
>+
>+/**
>+ * flow_dissector_key_icmp:
>+ *	@ports: type and code of ICMP header
>+ *		icmp: ICMP type (high) and code (low)
>+ *		type: ICMP type
>+ *		code: ICMP code
>+ */
>+struct flow_dissector_key_icmp {
>+	union {
> 		__be16 icmp;
> 		struct {
> 			u8 type;
>@@ -133,6 +141,7 @@ enum flow_dissector_key_id {
> 	FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_ipv4_addrs */
> 	FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
> 	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
>+	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
> 	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
> 	FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
> 	FLOW_DISSECTOR_KEY_VLAN, /* struct flow_dissector_key_flow_vlan */
>@@ -171,7 +180,10 @@ struct flow_keys {
> 	struct flow_dissector_key_tags tags;
> 	struct flow_dissector_key_vlan vlan;
> 	struct flow_dissector_key_keyid keyid;
>-	struct flow_dissector_key_ports ports;
>+	union {
>+		struct flow_dissector_key_ports ports;
>+		struct flow_dissector_key_icmp icmp;
>+	};
> 	struct flow_dissector_key_addrs addrs;
> };
> 
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 0584b4bb4390..33e5fa2b3e87 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -139,6 +139,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 	struct flow_dissector_key_basic *key_basic;
> 	struct flow_dissector_key_addrs *key_addrs;
> 	struct flow_dissector_key_ports *key_ports;
>+	struct flow_dissector_key_icmp *key_icmp;
> 	struct flow_dissector_key_tags *key_tags;
> 	struct flow_dissector_key_vlan *key_vlan;
> 	struct flow_dissector_key_keyid *key_keyid;
>@@ -559,18 +560,25 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 		break;
> 	}
> 
>-	if (dissector_uses_key(flow_dissector,
>-			       FLOW_DISSECTOR_KEY_PORTS)) {
>-		key_ports = skb_flow_dissector_target(flow_dissector,
>-						      FLOW_DISSECTOR_KEY_PORTS,
>-						      target_container);
>-		if (flow_protos_are_icmp_any(proto, ip_proto))
>-			key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
>-							    hlen);
>-		else
>+	if (flow_protos_are_icmp_any(proto, ip_proto)) {
>+		if (dissector_uses_key(flow_dissector,
>+				       FLOW_DISSECTOR_KEY_ICMP)) {
>+			key_icmp = skb_flow_dissector_target(flow_dissector,
>+							     FLOW_DISSECTOR_KEY_ICMP,
>+							     target_container);
>+			key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data,
>+							   hlen);
>+		}
>+	} else {
>+		if (dissector_uses_key(flow_dissector,
>+				       FLOW_DISSECTOR_KEY_PORTS)) {
>+			key_ports = skb_flow_dissector_target(flow_dissector,
>+							      FLOW_DISSECTOR_KEY_PORTS,
>+							      target_container);
> 			key_ports->ports = __skb_flow_get_ports(skb, nhoff,
> 								ip_proto, data,
> 								hlen);
>+		}
> 	}
> 
> out_good:
>@@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
> 		.offset = offsetof(struct flow_keys, ports),
> 	},
> 	{
>+		.key_id = FLOW_DISSECTOR_KEY_ICMP,
>+		.offset = offsetof(struct flow_keys, icmp),
>+	},
>+	{
> 		.key_id = FLOW_DISSECTOR_KEY_VLAN,
> 		.offset = offsetof(struct flow_keys, vlan),
> 	},
>diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>index c96b2a349779..f4ba69bd362f 100644
>--- a/net/sched/cls_flower.c
>+++ b/net/sched/cls_flower.c
>@@ -38,7 +38,10 @@ struct fl_flow_key {
> 		struct flow_dissector_key_ipv4_addrs ipv4;
> 		struct flow_dissector_key_ipv6_addrs ipv6;
> 	};
>-	struct flow_dissector_key_ports tp;
>+	union {
>+		struct flow_dissector_key_ports tp;
>+		struct flow_dissector_key_icmp icmp;
>+	};
> 	struct flow_dissector_key_keyid enc_key_id;
> 	union {
> 		struct flow_dissector_key_ipv4_addrs enc_ipv4;
>@@ -511,19 +514,23 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
> 			       &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
> 			       sizeof(key->tp.dst));
> 	} else if (flow_basic_key_is_icmpv4(&key->basic)) {
>-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
>-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
>-			       sizeof(key->tp.type));
>-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>-			       sizeof(key->tp.code));
>+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
>+			       &mask->icmp.type,
>+			       TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
>+			       sizeof(key->icmp.type));
>+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>+			       &mask->icmp.code,
>+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>+			       sizeof(key->icmp.code));
> 	} else if (flow_basic_key_is_icmpv6(&key->basic)) {
>-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
>-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
>-			       sizeof(key->tp.type));
>-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>-			       sizeof(key->tp.code));
>+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
>+			       &mask->icmp.type,
>+			       TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
>+			       sizeof(key->icmp.type));
>+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>+			       &mask->icmp.code,
>+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>+			       sizeof(key->icmp.code));
> 	}
> 
> 	if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
>@@ -609,15 +616,16 @@ static int fl_init_hashtable(struct cls_fl_head *head,
> 		keys[cnt].key_id = id;						\
> 		keys[cnt].offset = FL_KEY_MEMBER_OFFSET(member);		\
> 		cnt++;								\
>-	} while(0);
>+	} while(0)
> 
> #define FL_KEY_SET_IF_MASKED(mask, keys, cnt, id, member)			\
> 	do {									\
> 		if (FL_KEY_IS_MASKED(mask, member))				\
> 			FL_KEY_SET(keys, cnt, id, member);			\
>-	} while(0);
>+	} while(0)
> 
> static void fl_init_dissector(struct cls_fl_head *head,
>+			      struct cls_fl_filter *f,
> 			      struct fl_flow_mask *mask)
> {
> 	struct flow_dissector_key keys[FLOW_DISSECTOR_KEY_MAX];
>@@ -631,8 +639,12 @@ static void fl_init_dissector(struct cls_fl_head *head,
> 			     FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
> 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> 			     FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);
>-	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>-			     FLOW_DISSECTOR_KEY_PORTS, tp);
>+	if (flow_basic_key_is_icmpv4(&f->key.basic))
>+		FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>+				     FLOW_DISSECTOR_KEY_ICMP, icmp);
>+	else
>+		FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>+				     FLOW_DISSECTOR_KEY_PORTS, tp);
> 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> 			     FLOW_DISSECTOR_KEY_VLAN, vlan);
> 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>@@ -652,6 +664,7 @@ static void fl_init_dissector(struct cls_fl_head *head,
> }
> 
> static int fl_check_assign_mask(struct cls_fl_head *head,
>+				struct cls_fl_filter *f,
> 				struct fl_flow_mask *mask)
> {
> 	int err;
>@@ -672,7 +685,7 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
> 	memcpy(&head->mask, mask, sizeof(head->mask));
> 	head->mask_assigned = true;
> 
>-	fl_init_dissector(head, mask);
>+	fl_init_dissector(head, f, mask);
> 
> 	return 0;
> }
>@@ -785,7 +798,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
> 	if (err)
> 		goto errout;
> 
>-	err = fl_check_assign_mask(head, &mask);
>+	err = fl_check_assign_mask(head, fnew, &mask);
> 	if (err)
> 		goto errout;
> 
>@@ -1000,24 +1013,24 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
> 				  sizeof(key->tp.dst))))
> 		goto nla_put_failure;
> 	else if (flow_basic_key_is_icmpv4(&key->basic) &&
>-		 (fl_dump_key_val(skb, &key->tp.type,
>-				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
>+		 (fl_dump_key_val(skb, &key->icmp.type,
>+				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
> 				  TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
>-				  sizeof(key->tp.type)) ||
>-		  fl_dump_key_val(skb, &key->tp.code,
>-				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
>+				  sizeof(key->icmp.type)) ||
>+		  fl_dump_key_val(skb, &key->icmp.code,
>+				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->icmp.code,
> 				  TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>-				  sizeof(key->tp.code))))
>+				  sizeof(key->icmp.code))))
> 		goto nla_put_failure;
> 	else if (flow_basic_key_is_icmpv6(&key->basic) &&
>-		 (fl_dump_key_val(skb, &key->tp.type,
>-				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
>+		 (fl_dump_key_val(skb, &key->icmp.type,
>+				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
> 				  TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
>-				  sizeof(key->tp.type)) ||
>-		  fl_dump_key_val(skb, &key->tp.code,
>-				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
>+				  sizeof(key->icmp.type)) ||
>+		  fl_dump_key_val(skb, &key->icmp.code,
>+				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
> 				  TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
>-				  sizeof(key->tp.code))))
>+				  sizeof(key->icmp.code))))
> 		goto nla_put_failure;
> 
> 	if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Tom Herbert]

On Mon, Dec 5, 2016 at 6:23 AM, Simon Horman <simon.horman@netronome.com> wrote:
> On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
>> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
>> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
>> >>port dissection code as although ICMP is not a transport protocol and their
>> >>type and code are not ports this allows sharing of both code and storage.
>> >>
>> >>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>
> ...
>
>> > Digging into this a bit more. I think it would be much nice not to mix
>> > up l4 ports and icmp stuff.
>> >
>> > How about to have FLOW_DISSECTOR_KEY_ICMP
>> > and
>> > struct flow_dissector_key_icmp {
>> >         u8 type;
>> >         u8 code;
>> > };
>> >
>> > The you can make this structure and struct flow_dissector_key_ports into
>> > an union in struct flow_keys.
>> >
>> > Looks much cleaner to me.
>
> Hi Jiri,
>
> I wondered about that too. The main reason that I didn't do this the first
> time around is that I wasn't sure what to do to differentiate between ports
> and icmp in fl_init_dissector() given that using a union would could to
> mask bits being set in both if they are set in either.
>
> I've taken a stab at addressing that below along with implementing your
> suggestion. If the treatment in fl_init_dissector() is acceptable
> perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS
> too?
>
>> I agree, this patch adds to many conditionals into the fast path for
>> ICMP handling. Neither is there much point in using type and code as
>> input to the packet hash.
>
> Hi Tom,
>
> I'm not sure that I follow this. A packet may be ICMP or not regardless of
> if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
> not. I'd appreciate some guidance here.
>
>
> First-pass at implementing Jiri's suggestion follows:
>
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 5540dfa18872..475cd121496f 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -91,14 +91,10 @@ struct flow_dissector_key_addrs {
>
>  /**
>   * flow_dissector_key_ports:
> - *     @ports: port numbers of Transport header or
> - *             type and code of ICMP header
> + *     @ports: port numbers of Transport header
>   *             ports: source (high) and destination (low) port numbers
>   *             src: source port number
>   *             dst: destination port number
> - *             icmp: ICMP type (high) and code (low)
> - *             type: ICMP type
> - *             type: ICMP code
>   */
>  struct flow_dissector_key_ports {
>         union {
> @@ -107,6 +103,18 @@ struct flow_dissector_key_ports {
>                         __be16 src;
>                         __be16 dst;
>                 };
> +       };
> +};
> +
> +/**
> + * flow_dissector_key_icmp:
> + *     @ports: type and code of ICMP header
> + *             icmp: ICMP type (high) and code (low)
> + *             type: ICMP type
> + *             code: ICMP code
> + */
> +struct flow_dissector_key_icmp {
> +       union {
>                 __be16 icmp;
>                 struct {
>                         u8 type;
> @@ -133,6 +141,7 @@ enum flow_dissector_key_id {
>         FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_ipv4_addrs */
>         FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
>         FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
> +       FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
>         FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
>         FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
>         FLOW_DISSECTOR_KEY_VLAN, /* struct flow_dissector_key_flow_vlan */
> @@ -171,7 +180,10 @@ struct flow_keys {
>         struct flow_dissector_key_tags tags;
>         struct flow_dissector_key_vlan vlan;
>         struct flow_dissector_key_keyid keyid;
> -       struct flow_dissector_key_ports ports;
> +       union {
> +               struct flow_dissector_key_ports ports;
> +               struct flow_dissector_key_icmp icmp;
> +       };
>         struct flow_dissector_key_addrs addrs;
>  };
>
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 0584b4bb4390..33e5fa2b3e87 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -139,6 +139,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>         struct flow_dissector_key_basic *key_basic;
>         struct flow_dissector_key_addrs *key_addrs;
>         struct flow_dissector_key_ports *key_ports;
> +       struct flow_dissector_key_icmp *key_icmp;
>         struct flow_dissector_key_tags *key_tags;
>         struct flow_dissector_key_vlan *key_vlan;
>         struct flow_dissector_key_keyid *key_keyid;
> @@ -559,18 +560,25 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>                 break;
>         }
>
> -       if (dissector_uses_key(flow_dissector,
> -                              FLOW_DISSECTOR_KEY_PORTS)) {
> -               key_ports = skb_flow_dissector_target(flow_dissector,
> -                                                     FLOW_DISSECTOR_KEY_PORTS,
> -                                                     target_container);
> -               if (flow_protos_are_icmp_any(proto, ip_proto))
> -                       key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
> -                                                           hlen);
> -               else
> +       if (flow_protos_are_icmp_any(proto, ip_proto)) {
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_ICMP)) {
> +                       key_icmp = skb_flow_dissector_target(flow_dissector,
> +                                                            FLOW_DISSECTOR_KEY_ICMP,
> +                                                            target_container);
> +                       key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data,
> +                                                          hlen);
> +               }
> +       } else {
> +               if (dissector_uses_key(flow_dissector,
> +                                      FLOW_DISSECTOR_KEY_PORTS)) {
> +                       key_ports = skb_flow_dissector_target(flow_dissector,
> +                                                             FLOW_DISSECTOR_KEY_PORTS,
> +                                                             target_container);
>                         key_ports->ports = __skb_flow_get_ports(skb, nhoff,
>                                                                 ip_proto, data,
>                                                                 hlen);
> +               }
>         }
>
>  out_good:
> @@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
>                 .offset = offsetof(struct flow_keys, ports),
>         },
>         {
> +               .key_id = FLOW_DISSECTOR_KEY_ICMP,
> +               .offset = offsetof(struct flow_keys, icmp),
> +       },

This is the problem I was referring to. This adds ICMP to the keys for
computing the skb hash and the ICMP type and code are in a union with
port numbers so they are in the range over which the hash is computed.
This means that we are treating type and code as some sort of flow and
and so different type/code between the same src/dst could follow
different paths in ECMP. For the purposes of computing a packet hash I
don't think we want ICMP information included. This might be a matter
of not putting FLOW_DISSECTOR_KEY_ICMP in flow_keys_dissector_keys,
where we need this information we could define another structure that
has all the same fields as in flow_keys_dissector_keys and include
FLOW_DISSECTOR_KEY_ICMP. Alternatively, the flow_dissector_key_icmp
could also be outside of the area that is used in the hash: that is no
in flow_keys_hash_start(keys) to flow_keys_hash_length(keys), keyval);

> +       {
>                 .key_id = FLOW_DISSECTOR_KEY_VLAN,
>                 .offset = offsetof(struct flow_keys, vlan),
>         },
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index c96b2a349779..f4ba69bd362f 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -38,7 +38,10 @@ struct fl_flow_key {
>                 struct flow_dissector_key_ipv4_addrs ipv4;
>                 struct flow_dissector_key_ipv6_addrs ipv6;
>         };
> -       struct flow_dissector_key_ports tp;
> +       union {
> +               struct flow_dissector_key_ports tp;
> +               struct flow_dissector_key_icmp icmp;
> +       };

When an ICMP packet is encapsulated within UDP then icmp overwrites
valid port information with is a stronger signal of the flow (unless
FLOW_DISSECTOR_F_STOP_AT_ENCAP is set). This is another reason not to
use a union with ports.

Tom

>         struct flow_dissector_key_keyid enc_key_id;
>         union {
>                 struct flow_dissector_key_ipv4_addrs enc_ipv4;
> @@ -511,19 +514,23 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
>                                &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
>                                sizeof(key->tp.dst));
>         } else if (flow_basic_key_is_icmpv4(&key->basic)) {
> -               fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
> -                              &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
> -                              sizeof(key->tp.type));
> -               fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> -                              &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> -                              sizeof(key->tp.code));
> +               fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
> +                              &mask->icmp.type,
> +                              TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
> +                              sizeof(key->icmp.type));
> +               fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> +                              &mask->icmp.code,
> +                              TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> +                              sizeof(key->icmp.code));
>         } else if (flow_basic_key_is_icmpv6(&key->basic)) {
> -               fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
> -                              &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
> -                              sizeof(key->tp.type));
> -               fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> -                              &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> -                              sizeof(key->tp.code));
> +               fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
> +                              &mask->icmp.type,
> +                              TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
> +                              sizeof(key->icmp.type));
> +               fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
> +                              &mask->icmp.code,
> +                              TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> +                              sizeof(key->icmp.code));
>         }
>
>         if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
> @@ -609,15 +616,16 @@ static int fl_init_hashtable(struct cls_fl_head *head,
>                 keys[cnt].key_id = id;                                          \
>                 keys[cnt].offset = FL_KEY_MEMBER_OFFSET(member);                \
>                 cnt++;                                                          \
> -       } while(0);
> +       } while(0)
>
>  #define FL_KEY_SET_IF_MASKED(mask, keys, cnt, id, member)                      \
>         do {                                                                    \
>                 if (FL_KEY_IS_MASKED(mask, member))                             \
>                         FL_KEY_SET(keys, cnt, id, member);                      \
> -       } while(0);
> +       } while(0)
>
>  static void fl_init_dissector(struct cls_fl_head *head,
> +                             struct cls_fl_filter *f,
>                               struct fl_flow_mask *mask)
>  {
>         struct flow_dissector_key keys[FLOW_DISSECTOR_KEY_MAX];
> @@ -631,8 +639,12 @@ static void fl_init_dissector(struct cls_fl_head *head,
>                              FLOW_DISSECTOR_KEY_IPV4_ADDRS, ipv4);
>         FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>                              FLOW_DISSECTOR_KEY_IPV6_ADDRS, ipv6);
> -       FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> -                            FLOW_DISSECTOR_KEY_PORTS, tp);
> +       if (flow_basic_key_is_icmpv4(&f->key.basic))
> +               FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> +                                    FLOW_DISSECTOR_KEY_ICMP, icmp);
> +       else
> +               FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> +                                    FLOW_DISSECTOR_KEY_PORTS, tp);
>         FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>                              FLOW_DISSECTOR_KEY_VLAN, vlan);
>         FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> @@ -652,6 +664,7 @@ static void fl_init_dissector(struct cls_fl_head *head,
>  }
>
>  static int fl_check_assign_mask(struct cls_fl_head *head,
> +                               struct cls_fl_filter *f,
>                                 struct fl_flow_mask *mask)
>  {
>         int err;
> @@ -672,7 +685,7 @@ static int fl_check_assign_mask(struct cls_fl_head *head,
>         memcpy(&head->mask, mask, sizeof(head->mask));
>         head->mask_assigned = true;
>
> -       fl_init_dissector(head, mask);
> +       fl_init_dissector(head, f, mask);
>
>         return 0;
>  }
> @@ -785,7 +798,7 @@ static int fl_change(struct net *net, struct sk_buff *in_skb,
>         if (err)
>                 goto errout;
>
> -       err = fl_check_assign_mask(head, &mask);
> +       err = fl_check_assign_mask(head, fnew, &mask);
>         if (err)
>                 goto errout;
>
> @@ -1000,24 +1013,24 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
>                                   sizeof(key->tp.dst))))
>                 goto nla_put_failure;
>         else if (flow_basic_key_is_icmpv4(&key->basic) &&
> -                (fl_dump_key_val(skb, &key->tp.type,
> -                                 TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
> +                (fl_dump_key_val(skb, &key->icmp.type,
> +                                 TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
>                                   TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
> -                                 sizeof(key->tp.type)) ||
> -                 fl_dump_key_val(skb, &key->tp.code,
> -                                 TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
> +                                 sizeof(key->icmp.type)) ||
> +                 fl_dump_key_val(skb, &key->icmp.code,
> +                                 TCA_FLOWER_KEY_ICMPV4_CODE, &mask->icmp.code,
>                                   TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
> -                                 sizeof(key->tp.code))))
> +                                 sizeof(key->icmp.code))))
>                 goto nla_put_failure;
>         else if (flow_basic_key_is_icmpv6(&key->basic) &&
> -                (fl_dump_key_val(skb, &key->tp.type,
> -                                 TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
> +                (fl_dump_key_val(skb, &key->icmp.type,
> +                                 TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
>                                   TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
> -                                 sizeof(key->tp.type)) ||
> -                 fl_dump_key_val(skb, &key->tp.code,
> -                                 TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
> +                                 sizeof(key->icmp.type)) ||
> +                 fl_dump_key_val(skb, &key->icmp.code,
> +                                 TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
>                                   TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
> -                                 sizeof(key->tp.code))))
> +                                 sizeof(key->icmp.code))))
>                 goto nla_put_failure;
>
>         if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Simon Horman]

On Mon, Dec 05, 2016 at 09:29:45AM -0800, Tom Herbert wrote:
> On Mon, Dec 5, 2016 at 6:23 AM, Simon Horman <simon.horman@netronome.com> wrote:
> > On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
> >> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
> >> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
> >> >>port dissection code as although ICMP is not a transport protocol and their
> >> >>type and code are not ports this allows sharing of both code and storage.
> >> >>
> >> >>Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >
> > ...
> >
> >> > Digging into this a bit more. I think it would be much nice not to mix
> >> > up l4 ports and icmp stuff.
> >> >
> >> > How about to have FLOW_DISSECTOR_KEY_ICMP
> >> > and
> >> > struct flow_dissector_key_icmp {
> >> >         u8 type;
> >> >         u8 code;
> >> > };
> >> >
> >> > The you can make this structure and struct flow_dissector_key_ports into
> >> > an union in struct flow_keys.
> >> >
> >> > Looks much cleaner to me.
> >
> > Hi Jiri,
> >
> > I wondered about that too. The main reason that I didn't do this the first
> > time around is that I wasn't sure what to do to differentiate between ports
> > and icmp in fl_init_dissector() given that using a union would could to
> > mask bits being set in both if they are set in either.
> >
> > I've taken a stab at addressing that below along with implementing your
> > suggestion. If the treatment in fl_init_dissector() is acceptable
> > perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS
> > too?
> >
> >> I agree, this patch adds to many conditionals into the fast path for
> >> ICMP handling. Neither is there much point in using type and code as
> >> input to the packet hash.
> >
> > Hi Tom,
> >
> > I'm not sure that I follow this. A packet may be ICMP or not regardless of
> > if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
> > not. I'd appreciate some guidance here.
> >
> > First-pass at implementing Jiri's suggestion follows:
> >

...

> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 0584b4bb4390..33e5fa2b3e87 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c

...

> > @@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
> >                 .offset = offsetof(struct flow_keys, ports),
> >         },
> >         {
> > +               .key_id = FLOW_DISSECTOR_KEY_ICMP,
> > +               .offset = offsetof(struct flow_keys, icmp),
> > +       },
> 
> This is the problem I was referring to. This adds ICMP to the keys for
> computing the skb hash and the ICMP type and code are in a union with
> port numbers so they are in the range over which the hash is computed.
> This means that we are treating type and code as some sort of flow and
> and so different type/code between the same src/dst could follow
> different paths in ECMP. For the purposes of computing a packet hash I
> don't think we want ICMP information included. This might be a matter
> of not putting FLOW_DISSECTOR_KEY_ICMP in flow_keys_dissector_keys,
> where we need this information we could define another structure that
> has all the same fields as in flow_keys_dissector_keys and include
> FLOW_DISSECTOR_KEY_ICMP. Alternatively, the flow_dissector_key_icmp
> could also be outside of the area that is used in the hash: that is no
> in flow_keys_hash_start(keys) to flow_keys_hash_length(keys), keyval);

...

> > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> > index c96b2a349779..f4ba69bd362f 100644
> > --- a/net/sched/cls_flower.c
> > +++ b/net/sched/cls_flower.c
> > @@ -38,7 +38,10 @@ struct fl_flow_key {
> >                 struct flow_dissector_key_ipv4_addrs ipv4;
> >                 struct flow_dissector_key_ipv6_addrs ipv6;
> >         };
> > -       struct flow_dissector_key_ports tp;
> > +       union {
> > +               struct flow_dissector_key_ports tp;
> > +               struct flow_dissector_key_icmp icmp;
> > +       };
> 
> When an ICMP packet is encapsulated within UDP then icmp overwrites
> valid port information with is a stronger signal of the flow (unless
> FLOW_DISSECTOR_F_STOP_AT_ENCAP is set). This is another reason not to
> use a union with ports.

...

Hi Tom,

thanks for your explanation. I think I have a clearer picture now.

I have reworked things to try to address your concerns.
In particular:

* FLOW_DISSECTOR_KEY_ICMP is no longer added to flow_keys_dissector_keys.
  I don't think it is needed at all for the use-case I currently have in
  mind, which is classifying packets using tc_flower. And adding it there
  was an error on my part.

* I stopped using a union for ports and icmp. At the very least this seems
  to make it easier to reason about things as we no longer need to consider
  that a port value may in fact be an ICMP type or code.

  This seems to allow us avoid adding a number of is_icmp checks (as I believe
  you pointed out earlier). And should also address the problem you state
  immediately above.

* I have also placed icmp outside of the range flow_keys_hash_start(keys)
  to flow_keys_hash_length(keys), keyval). This is because I now see no
  value of having it inside that range and it both avoids any chance of
  contaminating the hash with ICMP values and hashing over unwanted
  (hopefully zero) values.

  This required an update to flow_keys_hash_length() as the bound
  of the end of fields the hash is no longer the end of struct flow_keys.
  It seems clean but I wonder if there are similar assumptions lurking
  elsewhere.

I have lightly tested this for the tc_flower case (only).

Incremental patch on top of this series:

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a6f75cfb2bf7..8029dd4912b6 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3181,8 +3181,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
 	} else {
 		return false;
 	}
-	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 &&
-	    proto >= 0 && !skb_flow_is_icmp_any(skb, proto))
+	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
 		fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
 
 	return true;
@@ -3210,8 +3209,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
 		return bond_eth_hash(skb);
 
 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
-	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23 ||
-	    flow_keys_are_icmp_any(&flow))
+	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23)
 		hash = bond_eth_hash(skb);
 	else
 		hash = (__force u32)flow.ports.ports;
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 44a8f69a9198..9c535fbccf2c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1094,11 +1094,6 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
 __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 			    void *data, int hlen_proto);
 
-static inline bool skb_flow_is_icmp_any(const struct sk_buff *skb, u8 ip_proto)
-{
-	return flow_protos_are_icmp_any(skb->protocol, ip_proto);
-}
-
 static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
 					int thoff, u8 ip_proto)
 {
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 5540dfa18872..058c9df8a4d8 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -91,14 +91,10 @@ struct flow_dissector_key_addrs {
 
 /**
  * flow_dissector_key_ports:
- *	@ports: port numbers of Transport header or
- *		type and code of ICMP header
+ *	@ports: port numbers of Transport header
  *		ports: source (high) and destination (low) port numbers
  *		src: source port number
  *		dst: destination port number
- *		icmp: ICMP type (high) and code (low)
- *		type: ICMP type
- *		type: ICMP code
  */
 struct flow_dissector_key_ports {
 	union {
@@ -107,6 +103,18 @@ struct flow_dissector_key_ports {
 			__be16 src;
 			__be16 dst;
 		};
+	};
+};
+
+/**
+ * flow_dissector_key_icmp:
+ *	@ports: type and code of ICMP header
+ *		icmp: ICMP type (high) and code (low)
+ *		type: ICMP type
+ *		code: ICMP code
+ */
+struct flow_dissector_key_icmp {
+	union {
 		__be16 icmp;
 		struct {
 			u8 type;
@@ -115,7 +123,6 @@ struct flow_dissector_key_ports {
 	};
 };
 
-
 /**
  * struct flow_dissector_key_eth_addrs:
  * @src: source Ethernet address
@@ -133,6 +140,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_ipv4_addrs */
 	FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
 	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
+	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
 	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
 	FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
 	FLOW_DISSECTOR_KEY_VLAN, /* struct flow_dissector_key_flow_vlan */
@@ -173,11 +181,16 @@ struct flow_keys {
 	struct flow_dissector_key_keyid keyid;
 	struct flow_dissector_key_ports ports;
 	struct flow_dissector_key_addrs addrs;
+#define FLOW_KEYS_HASH_END_FIELD addrs
+	struct flow_dissector_key_icmp icmp;
 };
 
 #define FLOW_KEYS_HASH_OFFSET		\
 	offsetof(struct flow_keys, FLOW_KEYS_HASH_START_FIELD)
 
+#define FLOW_KEYS_HASH_END		\
+	offsetofend(struct flow_keys, FLOW_KEYS_HASH_END_FIELD)
+
 __be32 flow_get_u32_src(const struct flow_keys *flow);
 __be32 flow_get_u32_dst(const struct flow_keys *flow);
 
@@ -233,8 +246,7 @@ static inline bool flow_keys_are_icmp_any(const struct flow_keys *keys)
 
 static inline bool flow_keys_have_l4(const struct flow_keys *keys)
 {
-	return (!flow_keys_are_icmp_any(keys) && keys->ports.ports) ||
-		keys->tags.flow_label;
+	return (keys->ports.ports || keys->tags.flow_label);
 }
 
 u32 flow_hash_from_keys(struct flow_keys *keys);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 0584b4bb4390..ed6d46475343 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -139,6 +139,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 	struct flow_dissector_key_basic *key_basic;
 	struct flow_dissector_key_addrs *key_addrs;
 	struct flow_dissector_key_ports *key_ports;
+	struct flow_dissector_key_icmp *key_icmp;
 	struct flow_dissector_key_tags *key_tags;
 	struct flow_dissector_key_vlan *key_vlan;
 	struct flow_dissector_key_keyid *key_keyid;
@@ -559,18 +560,25 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
 		break;
 	}
 
-	if (dissector_uses_key(flow_dissector,
-			       FLOW_DISSECTOR_KEY_PORTS)) {
-		key_ports = skb_flow_dissector_target(flow_dissector,
-						      FLOW_DISSECTOR_KEY_PORTS,
-						      target_container);
-		if (flow_protos_are_icmp_any(proto, ip_proto))
-			key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
-							    hlen);
-		else
+	if (flow_protos_are_icmp_any(proto, ip_proto)) {
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_ICMP)) {
+			key_icmp = skb_flow_dissector_target(flow_dissector,
+							     FLOW_DISSECTOR_KEY_ICMP,
+							     target_container);
+			key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data,
+							   hlen);
+		}
+	} else {
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_PORTS)) {
+			key_ports = skb_flow_dissector_target(flow_dissector,
+							      FLOW_DISSECTOR_KEY_PORTS,
+							      target_container);
 			key_ports->ports = __skb_flow_get_ports(skb, nhoff,
 								ip_proto, data,
 								hlen);
+		}
 	}
 
 out_good:
@@ -613,9 +621,10 @@ static inline const u32 *flow_keys_hash_start(const struct flow_keys *flow)
 static inline size_t flow_keys_hash_length(const struct flow_keys *flow)
 {
 	size_t diff = FLOW_KEYS_HASH_OFFSET + sizeof(flow->addrs);
-	BUILD_BUG_ON((sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) % sizeof(u32));
+	BUILD_BUG_ON((FLOW_KEYS_HASH_END - FLOW_KEYS_HASH_OFFSET) %
+		     sizeof(u32));
 	BUILD_BUG_ON(offsetof(typeof(*flow), addrs) !=
-		     sizeof(*flow) - sizeof(flow->addrs));
+		     FLOW_KEYS_HASH_END - sizeof(flow->addrs));
 
 	switch (flow->control.addr_type) {
 	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
@@ -628,7 +637,7 @@ static inline size_t flow_keys_hash_length(const struct flow_keys *flow)
 		diff -= sizeof(flow->addrs.tipcaddrs);
 		break;
 	}
-	return (sizeof(*flow) - diff) / sizeof(u32);
+	return (FLOW_KEYS_HASH_END - diff) / sizeof(u32);
 }
 
 __be32 flow_get_u32_src(const struct flow_keys *flow)
@@ -745,8 +754,7 @@ void make_flow_keys_digest(struct flow_keys_digest *digest,
 
 	data->n_proto = flow->basic.n_proto;
 	data->ip_proto = flow->basic.ip_proto;
-	if (flow_keys_have_l4(flow))
-		data->ports = flow->ports.ports;
+	data->ports = flow->ports.ports;
 	data->src = flow->addrs.v4addrs.src;
 	data->dst = flow->addrs.v4addrs.dst;
 }
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 408313e33bbe..6575aba87630 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -96,7 +96,7 @@ static u32 flow_get_proto(const struct sk_buff *skb,
 static u32 flow_get_proto_src(const struct sk_buff *skb,
 			      const struct flow_keys *flow)
 {
-	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
+	if (flow->ports.ports)
 		return ntohs(flow->ports.src);
 
 	return addr_fold(skb->sk);
@@ -105,7 +105,7 @@ static u32 flow_get_proto_src(const struct sk_buff *skb,
 static u32 flow_get_proto_dst(const struct sk_buff *skb,
 			      const struct flow_keys *flow)
 {
-	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
+	if (flow->ports.ports)
 		return ntohs(flow->ports.dst);
 
 	return addr_fold(skb_dst(skb)) ^ (__force u16) tc_skb_protocol(skb);
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index c96b2a349779..56df0368125a 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -39,6 +39,7 @@ struct fl_flow_key {
 		struct flow_dissector_key_ipv6_addrs ipv6;
 	};
 	struct flow_dissector_key_ports tp;
+	struct flow_dissector_key_icmp icmp;
 	struct flow_dissector_key_keyid enc_key_id;
 	union {
 		struct flow_dissector_key_ipv4_addrs enc_ipv4;
@@ -511,19 +512,23 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 			       &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
 			       sizeof(key->tp.dst));
 	} else if (flow_basic_key_is_icmpv4(&key->basic)) {
-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
-			       sizeof(key->tp.type));
-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
-			       sizeof(key->tp.code));
+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
+			       &mask->icmp.type,
+			       TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
+			       sizeof(key->icmp.type));
+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
+			       &mask->icmp.code,
+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+			       sizeof(key->icmp.code));
 	} else if (flow_basic_key_is_icmpv6(&key->basic)) {
-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
-			       sizeof(key->tp.type));
-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
-			       sizeof(key->tp.code));
+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
+			       &mask->icmp.type,
+			       TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
+			       sizeof(key->icmp.type));
+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
+			       &mask->icmp.code,
+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+			       sizeof(key->icmp.code));
 	}
 
 	if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
@@ -634,6 +639,8 @@ static void fl_init_dissector(struct cls_fl_head *head,
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_PORTS, tp);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
+			     FLOW_DISSECTOR_KEY_ICMP, icmp);
+	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_VLAN, vlan);
 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
 			     FLOW_DISSECTOR_KEY_ENC_KEYID, enc_key_id);
@@ -1000,24 +1007,24 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 				  sizeof(key->tp.dst))))
 		goto nla_put_failure;
 	else if (flow_basic_key_is_icmpv4(&key->basic) &&
-		 (fl_dump_key_val(skb, &key->tp.type,
-				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
+		 (fl_dump_key_val(skb, &key->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
 				  TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
-				  sizeof(key->tp.type)) ||
-		  fl_dump_key_val(skb, &key->tp.code,
-				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
+				  sizeof(key->icmp.type)) ||
+		  fl_dump_key_val(skb, &key->icmp.code,
+				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->icmp.code,
 				  TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
-				  sizeof(key->tp.code))))
+				  sizeof(key->icmp.code))))
 		goto nla_put_failure;
 	else if (flow_basic_key_is_icmpv6(&key->basic) &&
-		 (fl_dump_key_val(skb, &key->tp.type,
-				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
+		 (fl_dump_key_val(skb, &key->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
 				  TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
-				  sizeof(key->tp.type)) ||
-		  fl_dump_key_val(skb, &key->tp.code,
-				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
+				  sizeof(key->icmp.type)) ||
+		  fl_dump_key_val(skb, &key->icmp.code,
+				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
 				  TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
-				  sizeof(key->tp.code))))
+				  sizeof(key->icmp.code))))
 		goto nla_put_failure;
 
 	if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Jiri Pirko]

Tue, Dec 06, 2016 at 11:51:46AM CET, simon.horman@netronome.com wrote:
>On Mon, Dec 05, 2016 at 09:29:45AM -0800, Tom Herbert wrote:
>> On Mon, Dec 5, 2016 at 6:23 AM, Simon Horman <simon.horman@netronome.com> wrote:
>> > On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
>> >> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
>> >> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
>> >> >>port dissection code as although ICMP is not a transport protocol and their
>> >> >>type and code are not ports this allows sharing of both code and storage.
>> >> >>
>> >> >>Signed-off-by: Simon Horman <simon.horman@netronome.com>
>> >
>> > ...
>> >
>> >> > Digging into this a bit more. I think it would be much nice not to mix
>> >> > up l4 ports and icmp stuff.
>> >> >
>> >> > How about to have FLOW_DISSECTOR_KEY_ICMP
>> >> > and
>> >> > struct flow_dissector_key_icmp {
>> >> >         u8 type;
>> >> >         u8 code;
>> >> > };
>> >> >
>> >> > The you can make this structure and struct flow_dissector_key_ports into
>> >> > an union in struct flow_keys.
>> >> >
>> >> > Looks much cleaner to me.
>> >
>> > Hi Jiri,
>> >
>> > I wondered about that too. The main reason that I didn't do this the first
>> > time around is that I wasn't sure what to do to differentiate between ports
>> > and icmp in fl_init_dissector() given that using a union would could to
>> > mask bits being set in both if they are set in either.
>> >
>> > I've taken a stab at addressing that below along with implementing your
>> > suggestion. If the treatment in fl_init_dissector() is acceptable
>> > perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS
>> > too?
>> >
>> >> I agree, this patch adds to many conditionals into the fast path for
>> >> ICMP handling. Neither is there much point in using type and code as
>> >> input to the packet hash.
>> >
>> > Hi Tom,
>> >
>> > I'm not sure that I follow this. A packet may be ICMP or not regardless of
>> > if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
>> > not. I'd appreciate some guidance here.
>> >
>> > First-pass at implementing Jiri's suggestion follows:
>> >
>
>...
>
>> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>> > index 0584b4bb4390..33e5fa2b3e87 100644
>> > --- a/net/core/flow_dissector.c
>> > +++ b/net/core/flow_dissector.c
>
>...
>
>> > @@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
>> >                 .offset = offsetof(struct flow_keys, ports),
>> >         },
>> >         {
>> > +               .key_id = FLOW_DISSECTOR_KEY_ICMP,
>> > +               .offset = offsetof(struct flow_keys, icmp),
>> > +       },
>> 
>> This is the problem I was referring to. This adds ICMP to the keys for
>> computing the skb hash and the ICMP type and code are in a union with
>> port numbers so they are in the range over which the hash is computed.
>> This means that we are treating type and code as some sort of flow and
>> and so different type/code between the same src/dst could follow
>> different paths in ECMP. For the purposes of computing a packet hash I
>> don't think we want ICMP information included. This might be a matter
>> of not putting FLOW_DISSECTOR_KEY_ICMP in flow_keys_dissector_keys,
>> where we need this information we could define another structure that
>> has all the same fields as in flow_keys_dissector_keys and include
>> FLOW_DISSECTOR_KEY_ICMP. Alternatively, the flow_dissector_key_icmp
>> could also be outside of the area that is used in the hash: that is no
>> in flow_keys_hash_start(keys) to flow_keys_hash_length(keys), keyval);
>
>...
>
>> > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> > index c96b2a349779..f4ba69bd362f 100644
>> > --- a/net/sched/cls_flower.c
>> > +++ b/net/sched/cls_flower.c
>> > @@ -38,7 +38,10 @@ struct fl_flow_key {
>> >                 struct flow_dissector_key_ipv4_addrs ipv4;
>> >                 struct flow_dissector_key_ipv6_addrs ipv6;
>> >         };
>> > -       struct flow_dissector_key_ports tp;
>> > +       union {
>> > +               struct flow_dissector_key_ports tp;
>> > +               struct flow_dissector_key_icmp icmp;
>> > +       };
>> 
>> When an ICMP packet is encapsulated within UDP then icmp overwrites
>> valid port information with is a stronger signal of the flow (unless
>> FLOW_DISSECTOR_F_STOP_AT_ENCAP is set). This is another reason not to
>> use a union with ports.
>
>...
>
>Hi Tom,
>
>thanks for your explanation. I think I have a clearer picture now.
>
>I have reworked things to try to address your concerns.
>In particular:
>
>* FLOW_DISSECTOR_KEY_ICMP is no longer added to flow_keys_dissector_keys.
>  I don't think it is needed at all for the use-case I currently have in
>  mind, which is classifying packets using tc_flower. And adding it there
>  was an error on my part.

I was just about to ask why you are adding it there. Good.


>
>* I stopped using a union for ports and icmp. At the very least this seems
>  to make it easier to reason about things as we no longer need to consider
>  that a port value may in fact be an ICMP type or code.

This should be within csl_flower code. You can easily have it as a union
in struct fl_flow_key. 


>
>  This seems to allow us avoid adding a number of is_icmp checks (as I believe
>  you pointed out earlier). And should also address the problem you state
>  immediately above.

I don't understand the check you are talking about. The union just allow
to share the mem. I don't see any checks needed.


>
>* I have also placed icmp outside of the range flow_keys_hash_start(keys)
>  to flow_keys_hash_length(keys), keyval). This is because I now see no
>  value of having it inside that range and it both avoids any chance of
>  contaminating the hash with ICMP values and hashing over unwanted
>  (hopefully zero) values.

Okay, now I'm confused again. You don't want to add this to
flow_keys_dissector_keys. Why would you?


>
>  This required an update to flow_keys_hash_length() as the bound
>  of the end of fields the hash is no longer the end of struct flow_keys.
>  It seems clean but I wonder if there are similar assumptions lurking
>  elsewhere.
>
>I have lightly tested this for the tc_flower case (only).
>
>Incremental patch on top of this series:
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index a6f75cfb2bf7..8029dd4912b6 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3181,8 +3181,7 @@ static bool bond_flow_dissect(struct bonding *bond, struct sk_buff *skb,
> 	} else {
> 		return false;
> 	}
>-	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 &&
>-	    proto >= 0 && !skb_flow_is_icmp_any(skb, proto))
>+	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER34 && proto >= 0)
> 		fk->ports.ports = skb_flow_get_ports(skb, noff, proto);
> 
> 	return true;
>@@ -3210,8 +3209,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
> 		return bond_eth_hash(skb);
> 
> 	if (bond->params.xmit_policy == BOND_XMIT_POLICY_LAYER23 ||
>-	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23 ||
>-	    flow_keys_are_icmp_any(&flow))
>+	    bond->params.xmit_policy == BOND_XMIT_POLICY_ENCAP23)
> 		hash = bond_eth_hash(skb);
> 	else
> 		hash = (__force u32)flow.ports.ports;
>diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>index 44a8f69a9198..9c535fbccf2c 100644
>--- a/include/linux/skbuff.h
>+++ b/include/linux/skbuff.h
>@@ -1094,11 +1094,6 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
> __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
> 			    void *data, int hlen_proto);
> 
>-static inline bool skb_flow_is_icmp_any(const struct sk_buff *skb, u8 ip_proto)
>-{
>-	return flow_protos_are_icmp_any(skb->protocol, ip_proto);
>-}
>-
> static inline __be32 skb_flow_get_ports(const struct sk_buff *skb,
> 					int thoff, u8 ip_proto)
> {
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index 5540dfa18872..058c9df8a4d8 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -91,14 +91,10 @@ struct flow_dissector_key_addrs {
> 
> /**
>  * flow_dissector_key_ports:
>- *	@ports: port numbers of Transport header or
>- *		type and code of ICMP header
>+ *	@ports: port numbers of Transport header
>  *		ports: source (high) and destination (low) port numbers
>  *		src: source port number
>  *		dst: destination port number
>- *		icmp: ICMP type (high) and code (low)
>- *		type: ICMP type
>- *		type: ICMP code
>  */
> struct flow_dissector_key_ports {
> 	union {
>@@ -107,6 +103,18 @@ struct flow_dissector_key_ports {
> 			__be16 src;
> 			__be16 dst;
> 		};
>+	};
>+};
>+
>+/**
>+ * flow_dissector_key_icmp:
>+ *	@ports: type and code of ICMP header
>+ *		icmp: ICMP type (high) and code (low)
>+ *		type: ICMP type
>+ *		code: ICMP code
>+ */
>+struct flow_dissector_key_icmp {
>+	union {
> 		__be16 icmp;
> 		struct {
> 			u8 type;
>@@ -115,7 +123,6 @@ struct flow_dissector_key_ports {
> 	};
> };
> 
>-
> /**
>  * struct flow_dissector_key_eth_addrs:
>  * @src: source Ethernet address
>@@ -133,6 +140,7 @@ enum flow_dissector_key_id {
> 	FLOW_DISSECTOR_KEY_IPV4_ADDRS, /* struct flow_dissector_key_ipv4_addrs */
> 	FLOW_DISSECTOR_KEY_IPV6_ADDRS, /* struct flow_dissector_key_ipv6_addrs */
> 	FLOW_DISSECTOR_KEY_PORTS, /* struct flow_dissector_key_ports */
>+	FLOW_DISSECTOR_KEY_ICMP, /* struct flow_dissector_key_icmp */
> 	FLOW_DISSECTOR_KEY_ETH_ADDRS, /* struct flow_dissector_key_eth_addrs */
> 	FLOW_DISSECTOR_KEY_TIPC_ADDRS, /* struct flow_dissector_key_tipc_addrs */
> 	FLOW_DISSECTOR_KEY_VLAN, /* struct flow_dissector_key_flow_vlan */
>@@ -173,11 +181,16 @@ struct flow_keys {
> 	struct flow_dissector_key_keyid keyid;
> 	struct flow_dissector_key_ports ports;
> 	struct flow_dissector_key_addrs addrs;
>+#define FLOW_KEYS_HASH_END_FIELD addrs
>+	struct flow_dissector_key_icmp icmp;
> };
> 
> #define FLOW_KEYS_HASH_OFFSET		\
> 	offsetof(struct flow_keys, FLOW_KEYS_HASH_START_FIELD)
> 
>+#define FLOW_KEYS_HASH_END		\
>+	offsetofend(struct flow_keys, FLOW_KEYS_HASH_END_FIELD)
>+
> __be32 flow_get_u32_src(const struct flow_keys *flow);
> __be32 flow_get_u32_dst(const struct flow_keys *flow);
> 
>@@ -233,8 +246,7 @@ static inline bool flow_keys_are_icmp_any(const struct flow_keys *keys)
> 
> static inline bool flow_keys_have_l4(const struct flow_keys *keys)
> {
>-	return (!flow_keys_are_icmp_any(keys) && keys->ports.ports) ||
>-		keys->tags.flow_label;
>+	return (keys->ports.ports || keys->tags.flow_label);
> }
> 
> u32 flow_hash_from_keys(struct flow_keys *keys);
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 0584b4bb4390..ed6d46475343 100644
>--- a/net/core/flow_dissector.c
>+++ b/net/core/flow_dissector.c
>@@ -139,6 +139,7 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 	struct flow_dissector_key_basic *key_basic;
> 	struct flow_dissector_key_addrs *key_addrs;
> 	struct flow_dissector_key_ports *key_ports;
>+	struct flow_dissector_key_icmp *key_icmp;
> 	struct flow_dissector_key_tags *key_tags;
> 	struct flow_dissector_key_vlan *key_vlan;
> 	struct flow_dissector_key_keyid *key_keyid;
>@@ -559,18 +560,25 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
> 		break;
> 	}
> 
>-	if (dissector_uses_key(flow_dissector,
>-			       FLOW_DISSECTOR_KEY_PORTS)) {
>-		key_ports = skb_flow_dissector_target(flow_dissector,
>-						      FLOW_DISSECTOR_KEY_PORTS,
>-						      target_container);
>-		if (flow_protos_are_icmp_any(proto, ip_proto))
>-			key_ports->icmp = skb_flow_get_be16(skb, nhoff, data,
>-							    hlen);
>-		else
>+	if (flow_protos_are_icmp_any(proto, ip_proto)) {
>+		if (dissector_uses_key(flow_dissector,
>+				       FLOW_DISSECTOR_KEY_ICMP)) {
>+			key_icmp = skb_flow_dissector_target(flow_dissector,
>+							     FLOW_DISSECTOR_KEY_ICMP,
>+							     target_container);
>+			key_icmp->icmp = skb_flow_get_be16(skb, nhoff, data,
>+							   hlen);
>+		}
>+	} else {
>+		if (dissector_uses_key(flow_dissector,
>+				       FLOW_DISSECTOR_KEY_PORTS)) {
>+			key_ports = skb_flow_dissector_target(flow_dissector,
>+							      FLOW_DISSECTOR_KEY_PORTS,
>+							      target_container);
> 			key_ports->ports = __skb_flow_get_ports(skb, nhoff,
> 								ip_proto, data,
> 								hlen);
>+		}
> 	}
> 
> out_good:
>@@ -613,9 +621,10 @@ static inline const u32 *flow_keys_hash_start(const struct flow_keys *flow)
> static inline size_t flow_keys_hash_length(const struct flow_keys *flow)
> {
> 	size_t diff = FLOW_KEYS_HASH_OFFSET + sizeof(flow->addrs);
>-	BUILD_BUG_ON((sizeof(*flow) - FLOW_KEYS_HASH_OFFSET) % sizeof(u32));
>+	BUILD_BUG_ON((FLOW_KEYS_HASH_END - FLOW_KEYS_HASH_OFFSET) %
>+		     sizeof(u32));
> 	BUILD_BUG_ON(offsetof(typeof(*flow), addrs) !=
>-		     sizeof(*flow) - sizeof(flow->addrs));
>+		     FLOW_KEYS_HASH_END - sizeof(flow->addrs));
> 
> 	switch (flow->control.addr_type) {
> 	case FLOW_DISSECTOR_KEY_IPV4_ADDRS:
>@@ -628,7 +637,7 @@ static inline size_t flow_keys_hash_length(const struct flow_keys *flow)
> 		diff -= sizeof(flow->addrs.tipcaddrs);
> 		break;
> 	}
>-	return (sizeof(*flow) - diff) / sizeof(u32);
>+	return (FLOW_KEYS_HASH_END - diff) / sizeof(u32);
> }
> 
> __be32 flow_get_u32_src(const struct flow_keys *flow)
>@@ -745,8 +754,7 @@ void make_flow_keys_digest(struct flow_keys_digest *digest,
> 
> 	data->n_proto = flow->basic.n_proto;
> 	data->ip_proto = flow->basic.ip_proto;
>-	if (flow_keys_have_l4(flow))
>-		data->ports = flow->ports.ports;
>+	data->ports = flow->ports.ports;
> 	data->src = flow->addrs.v4addrs.src;
> 	data->dst = flow->addrs.v4addrs.dst;
> }
>diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
>index 408313e33bbe..6575aba87630 100644
>--- a/net/sched/cls_flow.c
>+++ b/net/sched/cls_flow.c
>@@ -96,7 +96,7 @@ static u32 flow_get_proto(const struct sk_buff *skb,
> static u32 flow_get_proto_src(const struct sk_buff *skb,
> 			      const struct flow_keys *flow)
> {
>-	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
>+	if (flow->ports.ports)
> 		return ntohs(flow->ports.src);
> 
> 	return addr_fold(skb->sk);
>@@ -105,7 +105,7 @@ static u32 flow_get_proto_src(const struct sk_buff *skb,
> static u32 flow_get_proto_dst(const struct sk_buff *skb,
> 			      const struct flow_keys *flow)
> {
>-	if (!flow_keys_are_icmp_any(flow) && flow->ports.ports)
>+	if (flow->ports.ports)
> 		return ntohs(flow->ports.dst);
> 
> 	return addr_fold(skb_dst(skb)) ^ (__force u16) tc_skb_protocol(skb);
>diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>index c96b2a349779..56df0368125a 100644
>--- a/net/sched/cls_flower.c
>+++ b/net/sched/cls_flower.c
>@@ -39,6 +39,7 @@ struct fl_flow_key {
> 		struct flow_dissector_key_ipv6_addrs ipv6;
> 	};
> 	struct flow_dissector_key_ports tp;
>+	struct flow_dissector_key_icmp icmp;
> 	struct flow_dissector_key_keyid enc_key_id;
> 	union {
> 		struct flow_dissector_key_ipv4_addrs enc_ipv4;
>@@ -511,19 +512,23 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
> 			       &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
> 			       sizeof(key->tp.dst));
> 	} else if (flow_basic_key_is_icmpv4(&key->basic)) {
>-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
>-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
>-			       sizeof(key->tp.type));
>-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>-			       sizeof(key->tp.code));
>+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
>+			       &mask->icmp.type,
>+			       TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
>+			       sizeof(key->icmp.type));
>+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>+			       &mask->icmp.code,
>+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>+			       sizeof(key->icmp.code));
> 	} else if (flow_basic_key_is_icmpv6(&key->basic)) {
>-		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
>-			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
>-			       sizeof(key->tp.type));
>-		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>-			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>-			       sizeof(key->tp.code));
>+		fl_set_key_val(tb, &key->icmp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
>+			       &mask->icmp.type,
>+			       TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
>+			       sizeof(key->icmp.type));
>+		fl_set_key_val(tb, &key->icmp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
>+			       &mask->icmp.code,
>+			       TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>+			       sizeof(key->icmp.code));
> 	}
> 
> 	if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
>@@ -634,6 +639,8 @@ static void fl_init_dissector(struct cls_fl_head *head,
> 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> 			     FLOW_DISSECTOR_KEY_PORTS, tp);
> 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
>+			     FLOW_DISSECTOR_KEY_ICMP, icmp);
>+	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> 			     FLOW_DISSECTOR_KEY_VLAN, vlan);
> 	FL_KEY_SET_IF_MASKED(&mask->key, keys, cnt,
> 			     FLOW_DISSECTOR_KEY_ENC_KEYID, enc_key_id);
>@@ -1000,24 +1007,24 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
> 				  sizeof(key->tp.dst))))
> 		goto nla_put_failure;
> 	else if (flow_basic_key_is_icmpv4(&key->basic) &&
>-		 (fl_dump_key_val(skb, &key->tp.type,
>-				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
>+		 (fl_dump_key_val(skb, &key->icmp.type,
>+				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
> 				  TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
>-				  sizeof(key->tp.type)) ||
>-		  fl_dump_key_val(skb, &key->tp.code,
>-				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
>+				  sizeof(key->icmp.type)) ||
>+		  fl_dump_key_val(skb, &key->icmp.code,
>+				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->icmp.code,
> 				  TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
>-				  sizeof(key->tp.code))))
>+				  sizeof(key->icmp.code))))
> 		goto nla_put_failure;
> 	else if (flow_basic_key_is_icmpv6(&key->basic) &&
>-		 (fl_dump_key_val(skb, &key->tp.type,
>-				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
>+		 (fl_dump_key_val(skb, &key->icmp.type,
>+				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
> 				  TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
>-				  sizeof(key->tp.type)) ||
>-		  fl_dump_key_val(skb, &key->tp.code,
>-				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
>+				  sizeof(key->icmp.type)) ||
>+		  fl_dump_key_val(skb, &key->icmp.code,
>+				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->icmp.code,
> 				  TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
>-				  sizeof(key->tp.code))))
>+				  sizeof(key->icmp.code))))
> 		goto nla_put_failure;
> 
> 	if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&

Re: [PATCH v2 net-next 1/2] flow dissector: ICMP support

[Simon Horman]

On Tue, Dec 06, 2016 at 01:26:34PM +0100, Jiri Pirko wrote:
> Tue, Dec 06, 2016 at 11:51:46AM CET, simon.horman@netronome.com wrote:
> >On Mon, Dec 05, 2016 at 09:29:45AM -0800, Tom Herbert wrote:
> >> On Mon, Dec 5, 2016 at 6:23 AM, Simon Horman <simon.horman@netronome.com> wrote:
> >> > On Sat, Dec 03, 2016 at 10:52:32AM -0800, Tom Herbert wrote:
> >> >> On Sat, Dec 3, 2016 at 2:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> > Fri, Dec 02, 2016 at 09:31:41PM CET, simon.horman@netronome.com wrote:
> >> >> >>Allow dissection of ICMP(V6) type and code. This re-uses transport layer
> >> >> >>port dissection code as although ICMP is not a transport protocol and their
> >> >> >>type and code are not ports this allows sharing of both code and storage.
> >> >> >>
> >> >> >>Signed-off-by: Simon Horman <simon.horman@netronome.com>
> >> >
> >> > ...
> >> >
> >> >> > Digging into this a bit more. I think it would be much nice not to mix
> >> >> > up l4 ports and icmp stuff.
> >> >> >
> >> >> > How about to have FLOW_DISSECTOR_KEY_ICMP
> >> >> > and
> >> >> > struct flow_dissector_key_icmp {
> >> >> >         u8 type;
> >> >> >         u8 code;
> >> >> > };
> >> >> >
> >> >> > The you can make this structure and struct flow_dissector_key_ports into
> >> >> > an union in struct flow_keys.
> >> >> >
> >> >> > Looks much cleaner to me.
> >> >
> >> > Hi Jiri,
> >> >
> >> > I wondered about that too. The main reason that I didn't do this the first
> >> > time around is that I wasn't sure what to do to differentiate between ports
> >> > and icmp in fl_init_dissector() given that using a union would could to
> >> > mask bits being set in both if they are set in either.
> >> >
> >> > I've taken a stab at addressing that below along with implementing your
> >> > suggestion. If the treatment in fl_init_dissector() is acceptable
> >> > perhaps something similar should be done for FLOW_DISSECTOR_KEY_IPV[46]_ADDRS
> >> > too?
> >> >
> >> >> I agree, this patch adds to many conditionals into the fast path for
> >> >> ICMP handling. Neither is there much point in using type and code as
> >> >> input to the packet hash.
> >> >
> >> > Hi Tom,
> >> >
> >> > I'm not sure that I follow this. A packet may be ICMP or not regardless of
> >> > if FLOW_DISSECTOR_KEY_PORT (and now FLOW_DISSECTOR_KEY_ICMP) port is set or
> >> > not. I'd appreciate some guidance here.
> >> >
> >> > First-pass at implementing Jiri's suggestion follows:
> >> >
> >
> >...
> >
> >> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >> > index 0584b4bb4390..33e5fa2b3e87 100644
> >> > --- a/net/core/flow_dissector.c
> >> > +++ b/net/core/flow_dissector.c
> >
> >...
> >
> >> > @@ -975,6 +983,10 @@ static const struct flow_dissector_key flow_keys_dissector_keys[] = {
> >> >                 .offset = offsetof(struct flow_keys, ports),
> >> >         },
> >> >         {
> >> > +               .key_id = FLOW_DISSECTOR_KEY_ICMP,
> >> > +               .offset = offsetof(struct flow_keys, icmp),
> >> > +       },
> >> 
> >> This is the problem I was referring to. This adds ICMP to the keys for
> >> computing the skb hash and the ICMP type and code are in a union with
> >> port numbers so they are in the range over which the hash is computed.
> >> This means that we are treating type and code as some sort of flow and
> >> and so different type/code between the same src/dst could follow
> >> different paths in ECMP. For the purposes of computing a packet hash I
> >> don't think we want ICMP information included. This might be a matter
> >> of not putting FLOW_DISSECTOR_KEY_ICMP in flow_keys_dissector_keys,
> >> where we need this information we could define another structure that
> >> has all the same fields as in flow_keys_dissector_keys and include
> >> FLOW_DISSECTOR_KEY_ICMP. Alternatively, the flow_dissector_key_icmp
> >> could also be outside of the area that is used in the hash: that is no
> >> in flow_keys_hash_start(keys) to flow_keys_hash_length(keys), keyval);
> >
> >...
> >
> >> > diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> >> > index c96b2a349779..f4ba69bd362f 100644
> >> > --- a/net/sched/cls_flower.c
> >> > +++ b/net/sched/cls_flower.c
> >> > @@ -38,7 +38,10 @@ struct fl_flow_key {
> >> >                 struct flow_dissector_key_ipv4_addrs ipv4;
> >> >                 struct flow_dissector_key_ipv6_addrs ipv6;
> >> >         };
> >> > -       struct flow_dissector_key_ports tp;
> >> > +       union {
> >> > +               struct flow_dissector_key_ports tp;
> >> > +               struct flow_dissector_key_icmp icmp;
> >> > +       };
> >> 
> >> When an ICMP packet is encapsulated within UDP then icmp overwrites
> >> valid port information with is a stronger signal of the flow (unless
> >> FLOW_DISSECTOR_F_STOP_AT_ENCAP is set). This is another reason not to
> >> use a union with ports.
> >
> >...
> >
> >Hi Tom,
> >
> >thanks for your explanation. I think I have a clearer picture now.
> >
> >I have reworked things to try to address your concerns.
> >In particular:
> >
> >* FLOW_DISSECTOR_KEY_ICMP is no longer added to flow_keys_dissector_keys.
> >  I don't think it is needed at all for the use-case I currently have in
> >  mind, which is classifying packets using tc_flower. And adding it there
> >  was an error on my part.
> 
> I was just about to ask why you are adding it there. Good.
> 
> 
> >
> >* I stopped using a union for ports and icmp. At the very least this seems
> >  to make it easier to reason about things as we no longer need to consider
> >  that a port value may in fact be an ICMP type or code.
> 
> This should be within csl_flower code. You can easily have it as a union
> in struct fl_flow_key. 

Ok, will do.

> >  This seems to allow us avoid adding a number of is_icmp checks (as I believe
> >  you pointed out earlier). And should also address the problem you state
> >  immediately above.
> 
> I don't understand the check you are talking about. The union just allow
> to share the mem. I don't see any checks needed.

I meant the checks that the patchset adds were in bond_main.c and
other places before accessing the ports fields. I now see we can avoid them
while still using a union.

> >* I have also placed icmp outside of the range flow_keys_hash_start(keys)
> >  to flow_keys_hash_length(keys), keyval). This is because I now see no
> >  value of having it inside that range and it both avoids any chance of
> >  contaminating the hash with ICMP values and hashing over unwanted
> >  (hopefully zero) values.
> 
> Okay, now I'm confused again. You don't want to add this to
> flow_keys_dissector_keys. Why would you?

Sorry, I think it was me that was being confused. I'll drop that change.
I agree it should not be necessary.

[PATCH v2 net-next 2/2] net/sched: cls_flower: Support matching on ICMP type and code

[Simon Horman]

Support matching on ICMP type and code.

Example usage:

tc qdisc add dev eth0 ingress

tc filter add dev eth0 protocol ip parent ffff: flower \
	indev eth0 ip_proto icmp type 8 code 0 action drop

tc filter add dev eth0 protocol ipv6 parent ffff: flower \
	indev eth0 ip_proto icmpv6 type 128 code 0 action drop

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 include/uapi/linux/pkt_cls.h | 10 ++++++++++
 net/sched/cls_flower.c       | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 86786d45ee66..58160fe80b80 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -457,6 +457,16 @@ enum {
 	TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK,	/* be16 */
 	TCA_FLOWER_KEY_ENC_UDP_DST_PORT,	/* be16 */
 	TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK,	/* be16 */
+
+	TCA_FLOWER_KEY_ICMPV4_CODE,	/* u8 */
+	TCA_FLOWER_KEY_ICMPV4_CODE_MASK,/* u8 */
+	TCA_FLOWER_KEY_ICMPV4_TYPE,	/* u8 */
+	TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,/* u8 */
+	TCA_FLOWER_KEY_ICMPV6_CODE,	/* u8 */
+	TCA_FLOWER_KEY_ICMPV6_CODE_MASK,/* u8 */
+	TCA_FLOWER_KEY_ICMPV6_TYPE,	/* u8 */
+	TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,/* u8 */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 1cacfa5c95f3..f639761eacfb 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -361,6 +361,14 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_ENC_UDP_SRC_PORT_MASK]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT]	= { .type = NLA_U16 },
 	[TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK]	= { .type = NLA_U16 },
+	[TCA_FLOWER_KEY_ICMPV4_TYPE]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV4_TYPE_MASK] = { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV4_CODE]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV4_CODE_MASK] = { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV6_TYPE]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV6_TYPE_MASK] = { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV6_CODE]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_ICMPV6_CODE_MASK] = { .type = NLA_U8 },
 };
 
 static void fl_set_key_val(struct nlattr **tb,
@@ -477,6 +485,20 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		fl_set_key_val(tb, &key->tp.dst, TCA_FLOWER_KEY_SCTP_DST,
 			       &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
 			       sizeof(key->tp.dst));
+	} else if (flow_basic_key_is_icmpv4(&key->basic)) {
+		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE,
+			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
+			       sizeof(key->tp.type));
+		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
+			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+			       sizeof(key->tp.code));
+	} else if (flow_basic_key_is_icmpv6(&key->basic)) {
+		fl_set_key_val(tb, &key->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE,
+			       &mask->tp.type, TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
+			       sizeof(key->tp.type));
+		fl_set_key_val(tb, &key->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE,
+			       &mask->tp.code, TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+			       sizeof(key->tp.code));
 	}
 
 	if (tb[TCA_FLOWER_KEY_ENC_IPV4_SRC] ||
@@ -950,6 +972,26 @@ static int fl_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 				  &mask->tp.dst, TCA_FLOWER_KEY_SCTP_DST_MASK,
 				  sizeof(key->tp.dst))))
 		goto nla_put_failure;
+	else if (flow_basic_key_is_icmpv4(&key->basic) &&
+		 (fl_dump_key_val(skb, &key->tp.type,
+				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->tp.type,
+				  TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
+				  sizeof(key->tp.type)) ||
+		  fl_dump_key_val(skb, &key->tp.code,
+				  TCA_FLOWER_KEY_ICMPV4_CODE, &mask->tp.code,
+				  TCA_FLOWER_KEY_ICMPV4_CODE_MASK,
+				  sizeof(key->tp.code))))
+		goto nla_put_failure;
+	else if (flow_basic_key_is_icmpv6(&key->basic) &&
+		 (fl_dump_key_val(skb, &key->tp.type,
+				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->tp.type,
+				  TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
+				  sizeof(key->tp.type)) ||
+		  fl_dump_key_val(skb, &key->tp.code,
+				  TCA_FLOWER_KEY_ICMPV6_CODE, &mask->tp.code,
+				  TCA_FLOWER_KEY_ICMPV6_CODE_MASK,
+				  sizeof(key->tp.code))))
+		goto nla_put_failure;
 
 	if (key->enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&
 	    (fl_dump_key_val(skb, &key->enc_ipv4.src,
-- 
2.7.0.rc3.207.g0ac5344