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

From: Pablo Neira Ayuso
Date: Wed May 29 2019 - 12:13:14 EST


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.