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.
Full review at: https://sashiko.dev
---
--- Patch [1]: [PATCH net] cxgb4: consider EtherType when validating filter priority ordering ---
commit eeb9ac7dfbda27ed06d0d26da6184a96f69d0da6
Author: Harshita V Rajput <harshitha.vr@chelsio.com>

cxgb4: consider EtherType when validating filter priority ordering

This commit updates cxgb4_filter_prio_in_range() and cxgb4_get_free_ftid()
to consider EtherType during priority validation. It allows adjacent filters
with different EtherTypes to be inserted regardless of their priority order,
as they would not match the same packets.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_filter.c
> index 657d96b9e2f66..8462455d9330f 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 &&

[Severity: High]
This is a pre-existing issue, but by only checking the valid flag here,
could concurrent filter insertions bypass the priority checks entirely?

When a filter is allocated, its bit is set in the hardware map but its valid
flag remains 0 until the firmware replies asynchronously (where filter_rpl
sets f->valid = 1).

If a concurrent TC filter insertion occurs (e.g., via unlocked TC or parallel
shared block updates across different ports on the same adapter),
cxgb4_get_free_ftid() finds the pending rule as a neighbor using
find_last_bit() or find_next_bit(), but skips the priority check because
prev_fe->valid or next_fe->valid is 0.

Should this logic also check the f->pending flag to prevent a second thread
from inserting a conflicting filter at an index that violates priority
ordering?

> +	    (!prev_fe->fs.mask.ethtype || !ethtype ||
> +	     prev_fe->fs.val.ethtype == ethtype))
> +		return false;

[Severity: High]
Does suppressing the priority check against immediate neighbors with a
different ethtype break the global sorting invariant of the TCAM filter array?

Since cxgb4_filter_prio_in_range() only validates against immediate allocated
neighbors, skipping the check allows out-of-order insertions. Once the table
contains interleaved EtherTypes, could this lead to hardware priority
inversions?

For example:
Index 0: IPv4 prio 5
Index 1: IPv6 prio 2

If a new IPv4 prio 3 rule is inserted at Index 2, it only checks against
Index 1. Since Index 1 is IPv6, the check is skipped. This inserts the prio 3
IPv4 rule after the prio 5 IPv4 rule, resulting in priority inversion since
hardware evaluates TCAM filters sequentially by index.

Additionally, does this broken transitivity affect cxgb4_get_free_ftid()
when inserting into the HASH region?

cxgb4_get_free_ftid() incorrectly assumes that find_last_bit() in the
HPFILTER region yields the rule with the highest tc_prio:

cxgb4_get_free_ftid() {
	...
	ftid = find_last_bit(t->hpftid_bmap, t->nhpftids);
	if (ftid < t->nhpftids) {
		f = &t->hpftid_tab[ftid];
		if (f->valid && tc_prio < f->fs.tc_prio)
			break;
	}
	...
}

With interleaved priorities, this assumption no longer holds.

> +
> +	if (next_fe->valid && next_fe->fs.tc_prio < prio &&
> +	    (!next_fe->fs.mask.ethtype || !ethtype ||
> +	     next_fe->fs.val.ethtype == ethtype))
>  		return false;
>  
>  	return true;