> -----Original Message----- > From: [email protected] <[email protected]> > Sent: Tuesday, October 6, 2026 4:50 PM > To: Loktionov, Aleksandr <[email protected]> > Cc: [email protected] > 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 <[email protected]> > > 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- > [email protected]?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
