netdev - Re: [PATCH net-next,v4 09/12] ethtool: add basic ethtool_rx_flow_spec to flow

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Nov 2018 14:12:14 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Pablo Neira Ayuso <pablo@...filter.org>
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,
        Manish.Chopra@...ium.com, marcelo.leitner@...il.com
Subject: Re: [PATCH net-next,v4 09/12] ethtool: add basic
 ethtool_rx_flow_spec to flow_rule structure translator

On Thu, Nov 29, 2018 at 03:22:28AM +0100, Pablo Neira Ayuso 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.
> 
> This patch also includes support for FLOW_EXT and FLOW_MAC_EXT.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@...filter.org>
...
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index afd9596ce636..c76d1b34c9a2 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
...
> +
> +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;
> +	struct flow_dissector_key_vlan			vlan;
> +	struct flow_dissector_key_eth_addrs		eth_addrs;
> +} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */

How about __aligned(sizeof(long)) ?

...
> +	act = &flow->rule->action.entries[0];
> +	switch (fs->ring_cookie) {
> +	case RX_CLS_FLOW_DISC:
> +		act->id = FLOW_ACTION_DROP;
> +		break;
> +	case RX_CLS_FLOW_WAKE:
> +		act->id = FLOW_ACTION_WAKE;
> +		break;
> +	default:
> +		act->id = FLOW_ACTION_QUEUE;
> +		act->queue_index = fs->ring_cookie;
> +		break;
> +	}

IMHO it would be cleaner if rules passing packets to RSS contexts were
also implemented using action, e.g. by using FLOW_ACTION_CONTEXT for
act->id and reusing act->queue_index for context id (using either an
anonymous union or more generic name).

Another idea: I would prefer moving this translation from NIC drivers to
the generic ethtool code. That would mean providing new ethtool_ops
callbacks (with a plan to deprecate the old ones). But it's probably
something something to leave for later as there would be a lot of pain
for no actual gain right now. (The reason I'm thinking about this is
that it wouldn't feel right to translate netlink messages to the ethtool
structures only to translate those once again in the NIC driver.)

Michal Kubecek