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

[Jiri Pirko <jiri@resnulli.us>]

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
>