| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20181119161626.bxz5cbzuq22y3nrj@salvia>
Date: Mon, 19 Nov 2018 17:16:26 +0100
From: Pablo Neira Ayuso <pablo@...filter.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
thomas.lendacky@....com, f.fainelli@...il.com,
ariel.elior@...ium.com, michael.chan@...adcom.com,
santosh@...lsio.com, madalin.bucur@....com,
yisen.zhuang@...wei.com, salil.mehta@...wei.com,
jeffrey.t.kirsher@...el.com, tariqt@...lanox.com,
saeedm@...lanox.com, jiri@...lanox.com, idosch@...lanox.com,
jakub.kicinski@...ronome.com, peppe.cavallaro@...com,
grygorii.strashko@...com, andrew@...n.ch,
vivien.didelot@...oirfairelinux.com, alexandre.torgue@...com,
joabreu@...opsys.com, linux-net-drivers@...arflare.com,
ganeshgr@...lsio.com, ogerlitz@...lanox.com
Subject: Re: [PATCH net-next,v2 09/12] flow_dissector: add basic
ethtool_rx_flow_spec to flow_rule structure translator
On Mon, Nov 19, 2018 at 03:49:01PM +0100, Jiri Pirko wrote:
> Mon, Nov 19, 2018 at 01:15:16AM CET, pablo@...filter.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@...filter.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.
Powered by blists - more mailing lists