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

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

[Harshita V Rajput]

cxgb4_filter_prio_in_range() currently validates the priority
ordering of adjacent filters based only on tc_prio. This can
incorrectly reject insertion of a new filter when an existing
filter with a higher priority matches a different EtherType.

For example:
- An LLDP filter (pref 4) blocks IPv4 (pref 2) and IPv6 (pref 3)
  filters from being added.
- An IPv6 filter (pref 3) blocks an IPv4 (pref 2) filter from being
  added.

In both cases the filters match different EtherTypes, so they could
never have matched the same packet anyway.

Fix this by considering both tc_prio and the matched EtherType. A
neighbouring filter only blocks the new filter if it has no specific
EtherType set, or its EtherType is the same as the new filter's.
Filters with different EtherTypes are now allowed regardless of
priority order. Filters with the same EtherType still follow the
original strict priority check, since they really can overlap.

Fixes: 41ec03e534ca ("cxgb4: check rule prio conflicts before offload")
Signed-off-by: Harshita V Rajput <harshitha.vr@chelsio.com>
Signed-off-by: Potnuri Bharat Teja <bharat@chelsio.com>
---
 .../net/ethernet/chelsio/cxgb4/cxgb4_filter.c | 19 ++++++++++++++-----
 .../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c  |  4 ++--
 .../chelsio/cxgb4/cxgb4_tc_matchall.c         |  2 +-
 .../net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c |  2 +-
 .../net/ethernet/chelsio/cxgb4/cxgb4_uld.h    |  2 +-
 5 files changed, 19 insertions(+), 10 deletions(-)

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
@@ -443,7 +443,7 @@ int cxgb4_get_filter_counters(struct net_device *dev, unsigned int fidx,
 }
 
 static bool cxgb4_filter_prio_in_range(struct tid_info *t, u32 idx, u8 nslots,
-				       u32 prio)
+				       u32 prio, u32 ethtype)
 {
 	struct filter_entry *prev_tab, *next_tab, *prev_fe, *next_fe;
 	u32 prev_ftid, next_ftid;
@@ -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;
 
 	return true;
 }
 
 int cxgb4_get_free_ftid(struct net_device *dev, u8 family, bool hash_en,
-			u32 tc_prio)
+			u32 tc_prio, u32 ethtype)
 {
 	struct adapter *adap = netdev2adap(dev);
 	struct tid_info *t = &adap->tids;
@@ -671,7 +680,7 @@ int cxgb4_get_free_ftid(struct net_device *dev, u8 family, bool hash_en,
 			 * with existing rules.
 			 */
 			if (cxgb4_filter_prio_in_range(t, ftid, n,
-						       tc_prio)) {
+						       tc_prio, ethtype)) {
 				ftid &= ~(n - 1);
 				found = true;
 				break;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
index 41a2998ee2a0..754d9f5f0fe0 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_flower.c
@@ -911,8 +911,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);
 	if (fidx < 0) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "No free LETCAM index available");
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
index bc290430245e..bab263785082 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_matchall.c
@@ -312,7 +312,7 @@ static int cxgb4_matchall_add_filter(struct net_device *dev,
 	 * existing rules.
 	 */
 	fidx = cxgb4_get_free_ftid(dev, filter_type ? PF_INET6 : PF_INET,
-				   false, cls->common.prio);
+				   false, cls->common.prio, 0);
 	if (fidx < 0) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "No free LETCAM index available");
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
index 1c1a7bc5a896..37a878f07f91 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_tc_u32.c
@@ -174,7 +174,7 @@ int cxgb4_config_knode(struct net_device *dev, struct tc_cls_u32_offload *cls)
 	 * existing rules.
 	 */
 	filter_id = cxgb4_get_free_ftid(dev, inet_family, false,
-					TC_U32_NODE(cls->knode.handle));
+					TC_U32_NODE(cls->knode.handle), 0);
 	if (filter_id < 0) {
 		NL_SET_ERR_MSG_MOD(extack,
 				   "No free LETCAM index available");
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h
index d7713038386c..42efc239df88 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h
@@ -282,7 +282,7 @@ struct chcr_ktls {
 struct ch_filter_specification;
 
 int cxgb4_get_free_ftid(struct net_device *dev, u8 family, bool hash_en,
-			u32 tc_prio);
+			u32 tc_prio, u32 ethtype);
 int __cxgb4_set_filter(struct net_device *dev, int filter_id,
 		       struct ch_filter_specification *fs,
 		       struct filter_ctx *ctx);
-- 
2.43.0

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

[Simon Horman]

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;

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

[Simon Horman]

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?