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

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

[Simon Horman]

Hi,

this series adds support for matching on ICMP type and code to cls_flower.

Changes v2->v3:
* Add FLOW_DISSECTOR_KEY_ICMP and use separate structure for ICMP

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

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

 include/net/flow_dissector.h | 55 +++++++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/pkt_cls.h | 10 ++++++++
 net/core/flow_dissector.c    | 49 +++++++++++++++++++++++++++++++++------
 net/sched/cls_flower.c       | 49 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 155 insertions(+), 8 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

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

[Simon Horman]

Allow dissection of ICMP(V6) type and code. This should only occur
if a packet is ICMP(V6) and the dissector has FLOW_DISSECTOR_KEY_ICMP set.

There are currently no users of FLOW_DISSECTOR_KEY_ICMP.
A follow-up patch will allow FLOW_DISSECTOR_KEY_ICMP to be used by
the flower classifier.
---
v3
* Add FLOW_DISSECTOR_KEY_ICMP and use separate structure for ICMP, struct
  flow_dissector_key_icmp, which is a union with struct
  flow_dissector_key_ports in struct flow_keys.
* Drop checks for !ICMP before accessing port field

v2
* Include all helpers in this patch
---
 include/net/flow_dissector.h | 55 +++++++++++++++++++++++++++++++++++++++++++-
 net/core/flow_dissector.c    | 49 +++++++++++++++++++++++++++++++++------
 2 files changed, 96 insertions(+), 8 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index c4f31666afd2..aefb7a8138a6 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>
 
@@ -104,6 +105,22 @@ struct flow_dissector_key_ports {
 	};
 };
 
+/**
+ * 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;
+			u8 code;
+		};
+	};
+};
 
 /**
  * struct flow_dissector_key_eth_addrs:
@@ -122,6 +139,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 */
@@ -160,7 +178,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;
 };
 
@@ -188,6 +209,38 @@ 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);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 1eb6f949e5b2..e37ff021a19e 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
@@ -117,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;
@@ -537,13 +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);
-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
-							data, hlen);
+	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:
-- 
2.7.0.rc3.207.g0ac5344

[PATCH net-next v3 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>
---
v3
* Use separate dissector key FLOW_DISSECTOR_KEY_ICMP
* Use separate structure to hold icmp fields, this does not share
  storage with ports

v2
* Move helpers to another patch
---
 include/uapi/linux/pkt_cls.h | 10 +++++++++
 net/sched/cls_flower.c       | 49 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 1adc0b654996..884c5aa515c4 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -458,6 +458,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 29a9e6d9f274..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;
@@ -386,6 +387,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,
@@ -502,6 +511,24 @@ 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->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->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] ||
@@ -612,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);
@@ -977,6 +1006,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->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV4_TYPE, &mask->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV4_TYPE_MASK,
+				  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->icmp.code))))
+		goto nla_put_failure;
+	else if (flow_basic_key_is_icmpv6(&key->basic) &&
+		 (fl_dump_key_val(skb, &key->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV6_TYPE, &mask->icmp.type,
+				  TCA_FLOWER_KEY_ICMPV6_TYPE_MASK,
+				  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->icmp.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

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

[Jiri Pirko]

Wed, Dec 07, 2016 at 10:41:36AM CET, simon.horman@netronome.com wrote:
>Allow dissection of ICMP(V6) type and code. This should only occur
>if a packet is ICMP(V6) and the dissector has FLOW_DISSECTOR_KEY_ICMP set.
>
>There are currently no users of FLOW_DISSECTOR_KEY_ICMP.
>A follow-up patch will allow FLOW_DISSECTOR_KEY_ICMP to be used by
>the flower classifier.
>---
>v3
>* Add FLOW_DISSECTOR_KEY_ICMP and use separate structure for ICMP, struct
>  flow_dissector_key_icmp, which is a union with struct
>  flow_dissector_key_ports in struct flow_keys.
>* Drop checks for !ICMP before accessing port field
>
>v2
>* Include all helpers in this patch
>---
> include/net/flow_dissector.h | 55 +++++++++++++++++++++++++++++++++++++++++++-
> net/core/flow_dissector.c    | 49 +++++++++++++++++++++++++++++++++------
> 2 files changed, 96 insertions(+), 8 deletions(-)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index c4f31666afd2..aefb7a8138a6 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>
> 
>@@ -104,6 +105,22 @@ struct flow_dissector_key_ports {
> 	};
> };
> 
>+/**
>+ * 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;
>+			u8 code;
>+		};
>+	};
>+};
> 
> /**
>  * struct flow_dissector_key_eth_addrs:
>@@ -122,6 +139,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 */
>@@ -160,7 +178,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;
>+	};

Why? You don't need this here.


> 	struct flow_dissector_key_addrs addrs;
> };
> 
>@@ -188,6 +209,38 @@ 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);
>diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
>index 1eb6f949e5b2..e37ff021a19e 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
>@@ -117,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;
>@@ -537,13 +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);
>-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
>-							data, hlen);
>+	if (flow_protos_are_icmp_any(proto, ip_proto)) {

You don't need this.

Just left FLOW_DISSECTOR_KEY_PORTS untouched, and add:

	if (dissector_uses_key(flow_dissector,
			       FLOW_DISSECTOR_KEY_ICMP)) {
			....


>+		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:
>-- 
>2.7.0.rc3.207.g0ac5344
>

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

[Simon Horman]

On Wed, Dec 07, 2016 at 11:01:48AM +0100, Jiri Pirko wrote:
> Wed, Dec 07, 2016 at 10:41:36AM CET, simon.horman@netronome.com wrote:
> >Allow dissection of ICMP(V6) type and code. This should only occur
> >if a packet is ICMP(V6) and the dissector has FLOW_DISSECTOR_KEY_ICMP set.
> >
> >There are currently no users of FLOW_DISSECTOR_KEY_ICMP.
> >A follow-up patch will allow FLOW_DISSECTOR_KEY_ICMP to be used by
> >the flower classifier.
> >---
> >v3
> >* Add FLOW_DISSECTOR_KEY_ICMP and use separate structure for ICMP, struct
> >  flow_dissector_key_icmp, which is a union with struct
> >  flow_dissector_key_ports in struct flow_keys.
> >* Drop checks for !ICMP before accessing port field
> >
> >v2
> >* Include all helpers in this patch
> >---
> > include/net/flow_dissector.h | 55 +++++++++++++++++++++++++++++++++++++++++++-
> > net/core/flow_dissector.c    | 49 +++++++++++++++++++++++++++++++++------
> > 2 files changed, 96 insertions(+), 8 deletions(-)
> >
> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >index c4f31666afd2..aefb7a8138a6 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>
> > 
> >@@ -104,6 +105,22 @@ struct flow_dissector_key_ports {
> > 	};
> > };
> > 
> >+/**
> >+ * 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;
> >+			u8 code;
> >+		};
> >+	};
> >+};
> > 
> > /**
> >  * struct flow_dissector_key_eth_addrs:
> >@@ -122,6 +139,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 */
> >@@ -160,7 +178,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;
> >+	};
> 
> Why? You don't need this here.

Thanks, I will remove drop that hunk.

> > 	struct flow_dissector_key_addrs addrs;
> > };
> > 
> >@@ -188,6 +209,38 @@ 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);
> >diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >index 1eb6f949e5b2..e37ff021a19e 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
> >@@ -117,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;
> >@@ -537,13 +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);
> >-		key_ports->ports = __skb_flow_get_ports(skb, nhoff, ip_proto,
> >-							data, hlen);
> >+	if (flow_protos_are_icmp_any(proto, ip_proto)) {
> 
> You don't need this.
> 
> Just left FLOW_DISSECTOR_KEY_PORTS untouched, and add:
> 
> 	if (dissector_uses_key(flow_dissector,
> 			       FLOW_DISSECTOR_KEY_ICMP)) {
> 			....

Thanks, will do.

> 
> 
> >+		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:
> >-- 
> >2.7.0.rc3.207.g0ac5344
> >