Re: [PATCH net v2] ethtool: Drop check for vlan etype and vlan tci when parsing flow_rule

[Pablo Neira Ayuso]

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@xxxxxxxxxxx>
> ---
> 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.