Re: [PATCH net-next,v3 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
From: Florian Fainelli <f.fainelli@gmail.com>
Date: 2018-11-22 15:35:24
On 11/20/2018 6:51 PM, Pablo Neira Ayuso wrote:
quoted hunk ↗ jump to hunk
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> --- v3: Suggested by Jiri Pirko: - Add struct ethtool_rx_flow_rule, keep placeholder to private dissector information. Reported by Manish Chopra: - Fix incorrect dissector user_keys flags. include/linux/ethtool.h | 10 +++ net/core/ethtool.c | 189 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+)diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index afd9596ce636..99849e0858b2 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h@@ -400,4 +400,14 @@ struct ethtool_ops { void (*get_ethtool_phy_stats)(struct net_device *, struct ethtool_stats *, u64 *); }; + +struct ethtool_rx_flow_rule { + struct flow_rule *rule; + unsigned long priv[0]; +}; + +struct ethtool_rx_flow_rule * +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs); +void ethtool_rx_flow_rule_free(struct ethtool_rx_flow_rule *rule); + #endif /* _LINUX_ETHTOOL_H */diff --git a/net/core/ethtool.c b/net/core/ethtool.c index d05402868575..e679d6478371 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c@@ -28,6 +28,7 @@ #include <linux/sched/signal.h> #include <linux/net.h> #include <net/xdp_sock.h> +#include <net/flow_offload.h> /* * Some useful ethtool_ops methods that're device independent.@@ -2808,3 +2809,191 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) return rc; } + +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 ethtool_rx_flow_rule * +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs)
This is more than alloc, it's allocate and map, no reason to split the two operations AFAICT, but the name could be improved, how about alloc_from()?
+{
+ static struct in6_addr zero_addr = {};
+ struct ethtool_rx_flow_match *match;
+ struct ethtool_rx_flow_rule *flow;
+ struct flow_action_entry *act;
+
+ flow = kzalloc(sizeof(struct ethtool_rx_flow_rule) +
+ sizeof(struct ethtool_rx_flow_match), GFP_KERNEL);
+ if (!flow)
+ return NULL;
+
+ /* ethtool_rx supports only one single action per rule. */
+ flow->rule = flow_rule_alloc(1);
+ if (!flow->rule) {
+ kfree(flow);
+ return NULL;
+ }
+
+ match = (struct ethtool_rx_flow_match *)flow->priv;
+ flow->rule->match.dissector = &match->dissector;
+ flow->rule->match.mask = &match->mask;
+ flow->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;
+ }I got confused a while ago between the ethtool ntuple and nfc semantics, and I can't remember if the following is true: - bits set to 1 indicate a match and bit set to 0 indicate a don't care for nfc - bits set to 0 indicate a match and bit set to 1 indicate a don't care for ntuple Depending on the answer that could mean that this check on a zero address may have to change.
+ if (v4_m_spec->ip4src ||
+ v4_m_spec->ip4dst) {
+ match->dissector.used_keys |=
+ (1 << FLOW_DISSECTOR_KEY_IPV4_ADDRS);Can you use BIT() here (and likewise for every one below). [snip]
+ + return flow;
What about the extended fields and non-IP protocols? -- Florian