[PATCH iwl-net v3 0/2] iavf: fix TC flower filter handling

[PATCH iwl-net v3 0/2] iavf: fix TC flower filter handling

[Aleksandr Loktionov]

Reject out-of-range traffic classes and require a destination port for
valid nonzero classes. Treat deletion of an already-removed flower filter
as successful after qdisc teardown.

Avinash Dayanand (1):
  iavf: fix TC boundary check in iavf_handle_tclass

Kiran Patil (1):
  iavf: return 0 when TC flower filter not found after qdisc teardown

 drivers/net/ethernet/intel/iavf/iavf_main.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
2.52.0

[PATCH iwl-net v3 1/2] iavf: fix TC boundary check in iavf_handle_tclass

[Aleksandr Loktionov]

From: Avinash Dayanand <avinash.dayanand@intel.com>

The condition 'tc < adapter->num_tc' only gates the destination-port
check for in-range TCs; when tc is out of range (tc >= num_tc) the if
body is skipped entirely and the function falls through to
VIRTCHNL_ACTION_TC_REDIRECT and returns 0, silently steering traffic to
a non-existent traffic class instead of rejecting the request.

Simply flipping the comparison to 'tc > adapter->num_tc' does not fix
this: it would let every in-range, non-zero tc (tc <= num_tc) skip the
destination-port requirement entirely, while an out-of-range tc with a
destination port set would still fall through and return 0.

Fix this by explicitly rejecting tc >= adapter->num_tc with -EINVAL,
and only then requiring a destination port for the remaining in-range,
non-zero TCs.

Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Signed-off-by: Avinash Dayanand <avinash.dayanand@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
v2 -> v3: Resend the reviewed TC fix in a focused two-patch series.

 drivers/net/ethernet/intel/iavf/iavf_main.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index d1fe7db..09b7f51 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4098,12 +4098,15 @@ static int iavf_handle_tclass(struct iavf_adapter *adapter, u32 tc,
 {
 	if (tc == 0)
 		return 0;
-	if (tc < adapter->num_tc) {
-		if (!filter->f.data.tcp_spec.dst_port) {
-			dev_err(&adapter->pdev->dev,
-				"Specify destination port to redirect to traffic class other than TC0\n");
-			return -EINVAL;
-		}
+	if (tc >= adapter->num_tc) {
+		dev_err(&adapter->pdev->dev,
+			"Unable to add filter because of invalid destination traffic class\n");
+		return -EINVAL;
+	}
+	if (!filter->f.data.tcp_spec.dst_port) {
+		dev_err(&adapter->pdev->dev,
+			"Specify destination port to redirect to traffic class other than TC0\n");
+		return -EINVAL;
 	}
 	/* redirect to a traffic class on the same device */
 	filter->f.action = VIRTCHNL_ACTION_TC_REDIRECT;
-- 
2.52.0

[PATCH iwl-net v3 2/2] iavf: return 0 when TC flower filter not found after qdisc teardown

[Aleksandr Loktionov]

From: Kiran Patil <kiran.patil@intel.com>

When an egress qdisc is destroyed, the driver proactively deletes all
associated cloud filters to prevent stale hardware state, decrementing
num_cloud_filters to zero in the process.

The kernel netdev layer is unaware of this implicit cleanup and may
still try to delete the same filters individually. If the filter is
not found in the driver's list and num_cloud_filters is already zero,
return 0 instead of -EINVAL to avoid confusing upper layers that
believe the filter is still offloaded in hardware.

Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
v2 -> v3: Resend the reviewed TC fix in a focused two-patch series.

 drivers/net/ethernet/intel/iavf/iavf_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 09b7f51..81d7082 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4214,7 +4214,8 @@ static int iavf_delete_clsflower(struct iavf_adapter *adapter,
 	if (filter) {
 		filter->del = true;
 		adapter->aq_required |= IAVF_FLAG_AQ_DEL_CLOUD_FILTER;
-	} else {
+	} else if (adapter->num_cloud_filters) {
+		/* other filters still tracked, so this one is a genuine miss */
 		err = -EINVAL;
 	}
 	spin_unlock_bh(&adapter->cloud_filter_list_lock);
-- 
2.52.0

Re: [PATCH iwl-net v3 2/2] iavf: return 0 when TC flower filter not found after qdisc teardown

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] iavf: race condition in clsflower teardown logic
--

commit 3c89e574f59d7231645a9c5af1ee68174ff89b80
Author: Kiran Patil <kiran.patil@intel.com>

iavf: return 0 when TC flower filter not found after qdisc teardown

When an egress qdisc is destroyed, the driver proactively deletes all
associated cloud filters, decrementing num_cloud_filters to zero. This commit
returns 0 instead of -EINVAL when the netdev layer subsequently tries to delete
a filter that isn't found, provided num_cloud_filters is zero, to avoid
confusing upper layers.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 2890e3c8005ee..c502e0a5391f3 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -4163,7 +4163,8 @@ static int iavf_delete_clsflower(struct iavf_adapter *adapter,
>  	if (filter) {
>  		filter->del = true;
>  		adapter->aq_required |= IAVF_FLAG_AQ_DEL_CLOUD_FILTER;
> -	} else {
> +	} else if (adapter->num_cloud_filters) {
> +		/* other filters still tracked, so this one is a genuine miss */
>  		err = -EINVAL;
>  	}

[Severity: Low]
Can this logic incorrectly assume that a non-zero adapter->num_cloud_filters
means a genuine miss, ignoring that new filters might be added before the
asynchronous teardown completes?

If a user executes 'tc qdisc del' followed immediately by 'tc qdisc add' and
'tc filter add', the qdisc deletion initiates a deferred block destruction
via tcf_block_put(), which queues work to tc_filter_wq.

Meanwhile, the new qdisc and filter can be added successfully, incrementing
adapter->num_cloud_filters.

When the deferred teardown work finally executes iavf_delete_clsflower()
for the old filter, the filter will not be found in the list.

Because adapter->num_cloud_filters is now non-zero from the newly added
filter, the driver takes this 'else if' branch and incorrectly returns -EINVAL.

Using a global counter to infer asynchronous teardown context seems brittle and
can negate the patch's intended fix under these race conditions.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006143723.2883728-1-aleksandr.loktionov@intel.com?part=2

RE: [PATCH iwl-net v3 2/2] iavf: return 0 when TC flower filter not found after qdisc teardown

[Loktionov, Aleksandr]

> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: Tuesday, October 6, 2026 4:50 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [PATCH iwl-net v3 2/2] iavf: return 0 when TC flower
> filter not found after qdisc teardown
> 
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Low] iavf: race condition in clsflower teardown logic
> --
> 
> commit 3c89e574f59d7231645a9c5af1ee68174ff89b80
> Author: Kiran Patil <kiran.patil@intel.com>
> 
> iavf: return 0 when TC flower filter not found after qdisc teardown
> 
> When an egress qdisc is destroyed, the driver proactively deletes all
> associated cloud filters, decrementing num_cloud_filters to zero. This
> commit returns 0 instead of -EINVAL when the netdev layer subsequently
> tries to delete a filter that isn't found, provided num_cloud_filters
> is zero, to avoid confusing upper layers.
> 
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > index 2890e3c8005ee..c502e0a5391f3 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> > @@ -4163,7 +4163,8 @@ static int iavf_delete_clsflower(struct
> iavf_adapter *adapter,
> >  	if (filter) {
> >  		filter->del = true;
> >  		adapter->aq_required |= IAVF_FLAG_AQ_DEL_CLOUD_FILTER;
> > -	} else {
> > +	} else if (adapter->num_cloud_filters) {
> > +		/* other filters still tracked, so this one is a genuine
> miss */
> >  		err = -EINVAL;
> >  	}
> 
> [Severity: Low]
> Can this logic incorrectly assume that a non-zero adapter-
> >num_cloud_filters means a genuine miss, ignoring that new filters
> might be added before the asynchronous teardown completes?
> 
> If a user executes 'tc qdisc del' followed immediately by 'tc qdisc
> add' and 'tc filter add', the qdisc deletion initiates a deferred
> block destruction via tcf_block_put(), which queues work to
> tc_filter_wq.
> 
> Meanwhile, the new qdisc and filter can be added successfully,
> incrementing
> adapter->num_cloud_filters.
> 
> When the deferred teardown work finally executes
> iavf_delete_clsflower() for the old filter, the filter will not be
> found in the list.
> 
> Because adapter->num_cloud_filters is now non-zero from the newly
> added filter, the driver takes this 'else if' branch and incorrectly
> returns -EINVAL.
> 
> Using a global counter to infer asynchronous teardown context seems
> brittle and can negate the patch's intended fix under these race
> conditions.
> 
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/20261006143723.2883728-1-
> aleksandr.loktionov@intel.com?part=2


Sashiko, thanks for the review 😊

The race is real: adapter->num_cloud_filters is global to the whole
adapter, not scoped to a block/qdisc, so a filter that lands on a
freshly-added block in between the old block's destruction and the
deferred iavf_delete_clsflower() call for its last cookie can make the
counter nonzero at exactly the wrong moment. This patch can't tell that
case apart from "the cookie really is missing" - which is exactly the
case that still needs -EINVAL.

Dropping this patch for now. A real fix needs a signal scoped to the
specific block/qdisc being torn down rather than a device-wide counter;
I don't have that design ready yet.

Patch 1/2 (the TC boundary check fix) doesn't depend on this one and
already has Simon's Reviewed-by, so it can go in on its own.

Thanks,
Aleksandr