Re: [PATCH net] cxgb4: consider EtherType when validating filter priority ordering

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/
---
cxgb4: consider EtherType when validating filter priority ordering

The commit relaxes cxgb4_filter_prio_in_range() so that neighbouring
filters with a different matched EtherType no longer block insertion of
a new filter. Both the callee and cxgb4_get_free_ftid() gain a new
ethtype argument, and callers in the tc flower, matchall, and u32 paths
are updated accordingly.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c
> index 657d96b9e2f6..8462455d9330 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c

[ ... ]

> @@ -536,15 +536,24 @@ static bool cxgb4_filter_prio_in_range(struct tid_info *t, u32 idx, u8 nslots,
>  	if (!prev_fe->fs.type)
>  		prev_fe = &prev_tab[prev_ftid];
>  
> -	if ((prev_fe->valid && prev_fe->fs.tc_prio > prio) ||
> -	    (next_fe->valid && next_fe->fs.tc_prio < prio))
> +	/* Filters with different EtherTypes can never match the same
> +	 * packet, so skip the priority check between them.
> +	 */
> +	if (prev_fe->valid && prev_fe->fs.tc_prio > prio &&
> +	    (!prev_fe->fs.mask.ethtype || !ethtype ||
> +	     prev_fe->fs.val.ethtype == ethtype))
> +		return false;
> +
> +	if (next_fe->valid && next_fe->fs.tc_prio < prio &&
> +	    (!next_fe->fs.mask.ethtype || !ethtype ||
> +	     next_fe->fs.val.ethtype == ethtype))
>  		return false;

[Low]
Does this comparison handle neighbouring filters that use a partial
ethtype mask?

The check treats two filters as targeting different EtherTypes when
prev_fe->fs.val.ethtype != ethtype, but it consults only the neighbour's
mask and never the new filter's mask. If a neighbour is inserted with,
for example, val.ethtype = 0x0801 and mask.ethtype = 0xFFFF, and the new
filter is inserted with val.ethtype = 0x0800 and (a hypothetically
propagated) mask.ethtype = 0xFF00, both filters can in principle match a
packet with EtherType 0x0801, yet the equality test 0x0801 == 0x0800
evaluates false and permits an out-of-order insertion.

A masked form such as:

    ((prev_fe->fs.val.ethtype ^ new_val) &
     prev_fe->fs.mask.ethtype & new_mask) == 0

would cover partial masks, but the current cxgb4_get_free_ftid() API
only carries the new filter's ethtype value and not its mask, so a
masked check cannot be expressed. Was the partial-mask case considered,
and should the new argument include the mask as well?

> +		return false;
> +
>  	return true;
>  }
>  
>  int cxgb4_get_free_ftid(struct net_device *dev, u8 family, bool hash_en,
> -			u32 tc_prio)
> +			u32 tc_prio, u32 ethtype)
>  {

[ ... ]

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> index 3307e5042681..07d9170cdb6f 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
> @@ -885,8 +885,8 @@ int cxgb4_flow_rule_replace(struct net_device *dev, struct flow_rule *rule,
>  	 * rule. Only insert rule if its prio doesn't conflict with
>  	 * existing rules.
>  	 */
> -	fidx = cxgb4_get_free_ftid(dev, inet_family, fs->hash,
> -				   tc_prio);
> +	fidx = cxgb4_get_free_ftid(dev, inet_family, fs->hash, tc_prio,
> +				   fs->mask.ethtype ? fs->val.ethtype : 0);

Related to the question above: the caller here discards fs->mask.ethtype
before it reaches cxgb4_filter_prio_in_range(), so a new filter with a
partial ethtype mask is compared as if its value were fully specified.
Is that intentional, or should the mask be plumbed through as well?