Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 29 May 2019 18:09:40 +0200
From:   Pablo Neira Ayuso <pablo@netfilter.org>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     davem@davemloft.net, Florian Fainelli <f.fainelli@gmail.com>,
        Jiri Pirko <jiri@mellanox.com>,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Antoine Tenart <antoine.tenart@bootlin.com>,
        thomas.petazzoni@bootlin.com
Subject: Re: [PATCH net v2] ethtool: Drop check for vlan etype and vlan tci
 when parsing flow_rule
Message-ID: <20190529160940.k2hnuoauaa4y2rga@salvia>
References: <20190529151344.31267-1-maxime.chevallier@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190529151344.31267-1-maxime.chevallier@bootlin.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, May 29, 2019 at 05:13:44PM +0200, Maxime Chevallier wrote:
> When parsing an ethtool flow spec to build a flow_rule, the code checks
> if both the vlan etype and the vlan tci are specified by the user to add
> a FLOW_DISSECTOR_KEY_VLAN match.
> 
> However, when the user only specified a vlan etype or a vlan tci, this
> check silently ignores these parameters.
> 
> For example, the following rule :
> 
> ethtool -N eth0 flow-type udp4 vlan 0x0010 action -1 loc 0
> 
> will result in no error being issued, but the equivalent rule will be
> created and passed to the NIC driver :
> 
> ethtool -N eth0 flow-type udp4 action -1 loc 0
> 
> In the end, neither the NIC driver using the rule nor the end user have
> a way to know that these keys were dropped along the way, or that
> incorrect parameters were entered.
> 
> This kind of check should be left to either the driver, or the ethtool
> flow spec layer.
> 
> This commit makes so that ethtool parameters are forwarded as-is to the
> NIC driver.
> 
> Since none of the users of ethtool_rx_flow_rule_create are using the
> VLAN dissector, I don't think this qualifies as a regression.
> 
> Fixes: eca4205f9ec3 ("ethtool: add ethtool_rx_flow_spec to flow_rule structure translator")
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> V2: Added Fixes: tag, targetted to -net.
> 
>  net/core/ethtool.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 4a593853cbf2..2fe86893e9b5 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -3010,26 +3010,23 @@ ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input)
>  		const struct ethtool_flow_ext *ext_h_spec = &fs->h_ext;
>  		const struct ethtool_flow_ext *ext_m_spec = &fs->m_ext;
>  
> -		if (ext_m_spec->vlan_etype &&
> -		    ext_m_spec->vlan_tci) {
> -			match->key.vlan.vlan_tpid = ext_h_spec->vlan_etype;
> -			match->mask.vlan.vlan_tpid = ext_m_spec->vlan_etype;
> +		match->key.vlan.vlan_tpid = ext_h_spec->vlan_etype;
> +		match->mask.vlan.vlan_tpid = ext_m_spec->vlan_etype;

Could you just check for ext_m_spec->vlan_etype, then set vlan_tpid
accordingly? ie.

        if (ext_m_spec->vlan_etype) {
		match->key.vlan.vlan_tpid = ext_h_spec->vlan_etype;
		match->mask.vlan.vlan_tpid = ext_m_spec->vlan_etype;
        }
        if (ext_m_spec->vlan_tci) {
                match->key.vlan.vlan_id = ...;
                match->mask.vlan.vlan_id = ...;
                match->key.vlan.vlan_priority = ...;
                match->mask.vlan.vlan_priority = ...;
        }

        if (ext_m_spec->vlan_etype ||
            ext_m_spec->vlan_tci) {
		match->dissector.used_keys |=
			BIT(FLOW_DISSECTOR_KEY_VLAN);
		match->dissector.offset[FLOW_DISSECTOR_KEY_VLAN] =
			offsetof(struct ethtool_rx_flow_key, vlan);
        }

Something like this above.