Re: [PATCH net-next,v2 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Nov 19, 2018 at 03:49:01PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:16AM CET, pablo@netfilter.org wrote:
> >This patch adds a function to translate the ethtool_rx_flow_spec
> >structure to the flow_rule representation.
> >
> >This allows us to reuse code from the driver side given that both flower
> >and ethtool_rx_flow interfaces use the same representation.
> >
> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> >---
> >v2: no changes.
> >
> > include/net/flow_dissector.h |   5 ++
> > net/core/flow_dissector.c    | 190 +++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 195 insertions(+)
> >
> >diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> >index 7a4683646d5a..ec9036232538 100644
> >--- a/include/net/flow_dissector.h
> >+++ b/include/net/flow_dissector.h
> >@@ -485,4 +485,9 @@ static inline bool flow_rule_match_key(const struct flow_rule *rule,
> > 	return dissector_uses_key(rule->match.dissector, key);
> > }
> > 
> >+struct ethtool_rx_flow_spec;
> >+
> >+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs);
> >+void ethtool_rx_flow_rule_free(struct flow_rule *rule);
> >+
> > #endif
> >diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> >index b9368349f0f7..ef5bdb62620c 100644
> >--- a/net/core/flow_dissector.c
> >+++ b/net/core/flow_dissector.c
> >@@ -17,6 +17,7 @@
> > #include <linux/dccp.h>
> > #include <linux/if_tunnel.h>
> > #include <linux/if_pppox.h>
> >+#include <uapi/linux/ethtool.h>
> > #include <linux/ppp_defs.h>
> > #include <linux/stddef.h>
> > #include <linux/if_ether.h>
> >@@ -276,6 +277,195 @@ void flow_action_free(struct flow_action *flow_action)
> > }
> > EXPORT_SYMBOL(flow_action_free);
> > 
> >+struct ethtool_rx_flow_key {
> >+	struct flow_dissector_key_basic			basic;
> >+	union {
> >+		struct flow_dissector_key_ipv4_addrs	ipv4;
> >+		struct flow_dissector_key_ipv6_addrs	ipv6;
> >+	};
> >+	struct flow_dissector_key_ports			tp;
> >+	struct flow_dissector_key_ip			ip;
> >+} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
> >+
> >+struct ethtool_rx_flow_match {
> >+	struct flow_dissector		dissector;
> >+	struct ethtool_rx_flow_key	key;
> >+	struct ethtool_rx_flow_key	mask;
> >+};
> >+
> >+struct flow_rule *ethtool_rx_flow_rule(const struct ethtool_rx_flow_spec *fs)
> >+{
> >+	static struct in6_addr zero_addr = {};
> >+	struct ethtool_rx_flow_match *match;
> >+	struct flow_action_key *act;
> >+	struct flow_rule *rule;
> >+
> >+	rule = kmalloc(sizeof(struct flow_rule), GFP_KERNEL);
> 
> Allocating struct flow_rule manually here seems wrong. There should
> be some helpers. Please see below.***

Will add them.

> >+	if (!rule)
> >+		return NULL;
> >+
> >+	match = kzalloc(sizeof(struct ethtool_rx_flow_match), GFP_KERNEL);
> >+	if (!match)
> >+		goto err_match;
> >+
> >+	rule->match.dissector	= &match->dissector;
> >+	rule->match.mask	= &match->mask;
> >+	rule->match.key		= &match->key;
> >+
> >+	match->mask.basic.n_proto = 0xffff;
> >+
> >+	switch (fs->flow_type & ~FLOW_EXT) {
> >+	case TCP_V4_FLOW:
> >+	case UDP_V4_FLOW: {
> >+		const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec;
> >+
> >+		match->key.basic.n_proto = htons(ETH_P_IP);
> >+
> >+		v4_spec = &fs->h_u.tcp_ip4_spec;
> >+		v4_m_spec = &fs->m_u.tcp_ip4_spec;
> >+
> >+		if (v4_m_spec->ip4src) {
> >+			match->key.ipv4.src = v4_spec->ip4src;
> >+			match->mask.ipv4.src = v4_m_spec->ip4src;
> >+		}
> >+		if (v4_m_spec->ip4dst) {
> >+			match->key.ipv4.dst = v4_spec->ip4dst;
> >+			match->mask.ipv4.dst = v4_m_spec->ip4dst;
> >+		}
> >+		if (v4_m_spec->ip4src ||
> >+		    v4_m_spec->ip4dst) {
> >+			match->dissector.used_keys |=
> >+				FLOW_DISSECTOR_KEY_IPV4_ADDRS;
> >+			match->dissector.offset[FLOW_DISSECTOR_KEY_IPV4_ADDRS] =
> >+				offsetof(struct ethtool_rx_flow_key, ipv4);
> >+		}
> >+		if (v4_m_spec->psrc) {
> >+			match->key.tp.src = v4_spec->psrc;
> >+			match->mask.tp.src = v4_m_spec->psrc;
> >+		}
> >+		if (v4_m_spec->pdst) {
> >+			match->key.tp.dst = v4_spec->pdst;
> >+			match->mask.tp.dst = v4_m_spec->pdst;
> >+		}
> >+		if (v4_m_spec->psrc ||
> >+		    v4_m_spec->pdst) {
> >+			match->dissector.used_keys |= FLOW_DISSECTOR_KEY_PORTS;
> >+			match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] =
> >+				offsetof(struct ethtool_rx_flow_key, tp);
> >+		}
> >+		if (v4_m_spec->tos) {
> >+			match->key.ip.tos = v4_spec->pdst;
> >+			match->mask.ip.tos = v4_m_spec->pdst;
> >+			match->dissector.used_keys |= FLOW_DISSECTOR_KEY_IP;
> >+			match->dissector.offset[FLOW_DISSECTOR_KEY_IP] =
> >+				offsetof(struct ethtool_rx_flow_key, ip);
> >+		}
> >+		}
> >+		break;
> >+	case TCP_V6_FLOW:
> >+	case UDP_V6_FLOW: {
> >+		const struct ethtool_tcpip6_spec *v6_spec, *v6_m_spec;
> >+
> >+		match->key.basic.n_proto = htons(ETH_P_IPV6);
> >+
> >+		v6_spec = &fs->h_u.tcp_ip6_spec;
> >+		v6_m_spec = &fs->m_u.tcp_ip6_spec;
> >+		if (memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr))) {
> >+			memcpy(&match->key.ipv6.src, v6_spec->ip6src,
> >+			       sizeof(match->key.ipv6.src));
> >+			memcpy(&match->mask.ipv6.src, v6_m_spec->ip6src,
> >+			       sizeof(match->mask.ipv6.src));
> >+		}
> >+		if (memcmp(v6_m_spec->ip6dst, &zero_addr, sizeof(zero_addr))) {
> >+			memcpy(&match->key.ipv6.dst, v6_spec->ip6dst,
> >+			       sizeof(match->key.ipv6.dst));
> >+			memcpy(&match->mask.ipv6.dst, v6_m_spec->ip6dst,
> >+			       sizeof(match->mask.ipv6.dst));
> >+		}
> >+		if (memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr)) ||
> >+		    memcmp(v6_m_spec->ip6src, &zero_addr, sizeof(zero_addr))) {
> >+			match->dissector.used_keys |=
> >+				FLOW_DISSECTOR_KEY_IPV6_ADDRS;
> >+			match->dissector.offset[FLOW_DISSECTOR_KEY_IPV6_ADDRS] =
> >+				offsetof(struct ethtool_rx_flow_key, ipv6);
> >+		}
> >+		if (v6_m_spec->psrc) {
> >+			match->key.tp.src = v6_spec->psrc;
> >+			match->mask.tp.src = v6_m_spec->psrc;
> >+		}
> >+		if (v6_m_spec->pdst) {
> >+			match->key.tp.dst = v6_spec->pdst;
> >+			match->mask.tp.dst = v6_m_spec->pdst;
> >+		}
> >+		if (v6_m_spec->psrc ||
> >+		    v6_m_spec->pdst) {
> >+			match->dissector.used_keys |= FLOW_DISSECTOR_KEY_PORTS;
> >+			match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] =
> >+				offsetof(struct ethtool_rx_flow_key, tp);
> >+		}
> >+		if (v6_m_spec->tclass) {
> >+			match->key.ip.tos = v6_spec->tclass;
> >+			match->mask.ip.tos = v6_m_spec->tclass;
> >+			match->dissector.used_keys |= FLOW_DISSECTOR_KEY_IP;
> >+			match->dissector.offset[FLOW_DISSECTOR_KEY_IP] =
> >+				offsetof(struct ethtool_rx_flow_key, ip);
> >+		}
> >+		}
> >+		break;
> >+	}
> >+
> >+	switch (fs->flow_type & ~FLOW_EXT) {
> >+	case TCP_V4_FLOW:
> >+	case TCP_V6_FLOW:
> >+		match->key.basic.ip_proto = IPPROTO_TCP;
> >+		break;
> >+	case UDP_V4_FLOW:
> >+	case UDP_V6_FLOW:
> >+		match->key.basic.ip_proto = IPPROTO_UDP;
> >+		break;
> >+	}
> >+	match->mask.basic.ip_proto = 0xff;
> >+
> >+	match->dissector.used_keys |= FLOW_DISSECTOR_KEY_BASIC;
> >+	match->dissector.offset[FLOW_DISSECTOR_KEY_BASIC] =
> >+		offsetof(struct ethtool_rx_flow_key, basic);
> >+
> >+	/* ethtool_rx supports only one single action per rule. */
> >+	if (flow_action_init(&rule->action, 1) < 0)
> >+		goto err_action;
> >+
> >+	act = &rule->action.keys[0];
> >+	switch (fs->ring_cookie) {
> >+	case RX_CLS_FLOW_DISC:
> >+		act->id = FLOW_ACTION_KEY_DROP;
> >+		break;
> >+	case RX_CLS_FLOW_WAKE:
> >+		act->id = FLOW_ACTION_KEY_WAKE;
> >+		break;
> >+	default:
> >+		act->id = FLOW_ACTION_KEY_QUEUE;
> >+		act->queue_index = fs->ring_cookie;
> >+		break;
> >+	}
> >+
> >+	return rule;
> >+
> >+err_action:
> >+	kfree(match);
> >+err_match:
> >+	kfree(rule);
> >+	return NULL;
> >+}
> >+EXPORT_SYMBOL(ethtool_rx_flow_rule);
> >+
> >+void ethtool_rx_flow_rule_free(struct flow_rule *rule)
> >+{
> >+	kfree((struct flow_match *)rule->match.dissector);
> 
> Ouch. I wonder if it cannot be stored rather in some rule->priv or
> something.

I can use container_of instead, which is what I should be using here.
This works because rule->match.dissector is at the top of struct
flow_match at this moment.

> On alloc, you can have a helper to allocate both:
> 
> ***
> struct flow_rule {
> 	...
> 	unsigned long priv[0];
> };
> 
> struct flow_rule *flow_rule_alloc(size_t priv_size)
> {
> 	return kzalloc(sizeof(struct flow_rule) + priv_size, ...);
> }

Yes, will do that. Thanks.