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

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:

quoted

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.

quoted

	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.

quoted

+		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