[PATCH v2] ice: Lock TC flower filter list operations

[PATCH v2] ice: Lock TC flower filter list operations

[Binbin Deng]

The per-PF TC flower filter list is used without a common lock.  TC flower
add and delete paths traverse the list, send firmware commands, and then
insert or remove and free nodes.  The reset path can concurrently walk the
same list in ice_rem_all_chnl_fltrs() while removing queue-channel filters,
and ice_replay_tc_fltrs() replays the same list after reset.  The reset
state check in the flower handlers is not a synchronization primitive.

The race is reachable by creating or deleting a TC flower filter while
service work handles a reset and removes queue channels.  A flower
operation can obtain a filter node before reset teardown removes and frees
it, or reset/replay can traverse a list concurrently modified by flower
control operations.

This can produce a KASAN use-after-free, hlist corruption, stale filter
state after replay, or a panic.  Firmware commands are slow-path
operations, so mutual exclusion is appropriate here.

Add a per-PF mutex and hold it across each list lookup, firmware operation,
list update, and free.  This keeps every reader of the filter list stable
while an entry is being added, removed, or replayed.

Found during a static-analysis-assisted audit: candidate sites were
surfaced by a Coccinelle rule, and the reader/writer paths were
confirmed by LLM-assisted (GLM-5.3) code inspection. Due to a lack
of hardware infrastructure. The issue was not observed at runtime.
It is identified by code inspection only.

Fixes: 9fea749856d1 ("ice: Add tc-flower filter support for channel")
Assisted-by: GLM-5.3 coccinelle
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 drivers/net/ethernet/intel/ice/ice.h        |  1 +
 drivers/net/ethernet/intel/ice/ice_main.c   |  3 +++
 drivers/net/ethernet/intel/ice/ice_tc_lib.c | 26 +++++++++++++++++----
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index db3c7015c56c..dac14c97bcbc 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -643,6 +643,7 @@ struct ice_pf {
 	 */
 	u16 num_dmac_chnl_fltrs;
 	struct hlist_head tc_flower_fltr_list;
+	struct mutex tc_flower_fltr_lock; /* Protect TC flower filters */
 
 	u64 supported_rxdids;
 
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index d88835482d3a..90031ba17b06 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3877,6 +3877,7 @@ void ice_deinit_pf(struct ice_pf *pf)
 	mutex_destroy(&pf->adev_mutex);
 	mutex_destroy(&pf->sw_mutex);
 	mutex_destroy(&pf->tc_mutex);
+	mutex_destroy(&pf->tc_flower_fltr_lock);
 	mutex_destroy(&pf->avail_q_mutex);
 	mutex_destroy(&pf->vfs.table_lock);
 
@@ -3976,6 +3977,7 @@ int ice_init_pf(struct ice_pf *pf)
 
 	mutex_init(&pf->sw_mutex);
 	mutex_init(&pf->tc_mutex);
+	mutex_init(&pf->tc_flower_fltr_lock);
 	mutex_init(&pf->adev_mutex);
 	mutex_init(&pf->lag_mutex);
 
@@ -8882,6 +8884,7 @@ static void ice_rem_all_chnl_fltrs(struct ice_pf *pf)
 		hlist_del(&fltr->tc_flower_node);
 		kfree(fltr);
 	}
+	mutex_lock(&pf->tc_flower_fltr_lock);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index d20357c04127..955ad3cc5417 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -2276,21 +2276,29 @@ int ice_add_cls_flower(struct net_device *netdev, struct ice_vsi *vsi,
 		return -EINVAL;
 	}
 
+	mutex_lock(&pf->tc_flower_fltr_lock);
+
 	/* avoid duplicate entries, if exists - return error */
 	fltr = ice_find_tc_flower_fltr(pf, cls_flower->cookie);
 	if (fltr) {
 		NL_SET_ERR_MSG_MOD(extack, "filter cookie already exists, ignoring");
+		mutex_unlock(&pf->tc_flower_fltr_lock);
 		return -EEXIST;
 	}
 
 	/* prep and add TC-flower filter in HW */
 	err = ice_add_tc_fltr(netdev, vsi, cls_flower, &fltr, ingress);
 	if (err)
-		return err;
+		goto unlock;
 
 	/* add filter into an ordered list */
 	hlist_add_head(&fltr->tc_flower_node, &pf->tc_flower_fltr_list);
+	mutex_unlock(&pf->tc_flower_fltr_lock);
 	return 0;
+
+unlock:
+	mutex_unlock(&pf->tc_flower_fltr_lock);
+	return err;
 }
 
 /**
@@ -2305,28 +2313,36 @@ ice_del_cls_flower(struct ice_vsi *vsi, struct flow_cls_offload *cls_flower)
 	struct ice_pf *pf = vsi->back;
 	int err;
 
+	mutex_lock(&pf->tc_flower_fltr_lock);
+
 	/* find filter */
 	fltr = ice_find_tc_flower_fltr(pf, cls_flower->cookie);
 	if (!fltr) {
 		if (!test_bit(ICE_FLAG_TC_MQPRIO, pf->flags) &&
-		    hlist_empty(&pf->tc_flower_fltr_list))
-			return 0;
+		    hlist_empty(&pf->tc_flower_fltr_list)) {
+				mutex_unlock(&pf->tc_flower_fltr_lock);
+				return 0;
+		}
 
 		NL_SET_ERR_MSG_MOD(cls_flower->common.extack, "failed to delete TC flower filter because unable to find it");
+		mutex_unlock(&pf->tc_flower_fltr_lock);
 		return -EINVAL;
 	}
 
 	fltr->extack = cls_flower->common.extack;
 	/* delete filter from HW */
 	err = ice_del_tc_fltr(vsi, fltr);
-	if (err)
+	if (err) {
+		mutex_unlock(&pf->tc_flower_fltr_lock);
 		return err;
+	}
 
 	/* delete filter from an ordered list */
 	hlist_del(&fltr->tc_flower_node);
 
 	/* free the filter node */
 	kfree(fltr);
+	mutex_unlock(&pf->tc_flower_fltr_lock);
 
 	return 0;
 }
@@ -2340,10 +2356,12 @@ void ice_replay_tc_fltrs(struct ice_pf *pf)
 	struct ice_tc_flower_fltr *fltr;
 	struct hlist_node *node;
 
+	mutex_lock(&pf->tc_flower_fltr_lock);
 	hlist_for_each_entry_safe(fltr, node,
 				  &pf->tc_flower_fltr_list,
 				  tc_flower_node) {
 		fltr->extack = NULL;
 		ice_add_switch_fltr(fltr->src_vsi, fltr);
 	}
+	mutex_unlock(&pf->tc_flower_fltr_lock);
 }
-- 
2.43.0

Re: [PATCH v2] ice: Lock TC flower filter list operations

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In ice_rem_all_chnl_fltrs()
  (drivers/net/ethernet/intel/ice/ice_main.c), the patch adds…
- [Low] The commit message says 'ice_replay_tc_fltrs() replays the same
  list after reset' and lists 'stale filter state after replay' and…

--

ice: Lock TC flower filter list operations

This patch adds a per-PF mutex, tc_flower_fltr_lock, to serialize access
to pf->tc_flower_fltr_list. The lock is taken in the TC flower add and
delete paths and in ice_replay_tc_fltrs(), and a mutex_lock() is added to
ice_rem_all_chnl_fltrs().

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index d88835482d3a..90031ba17b06 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c

[ ... ]

> @@ -8882,6 +8884,7 @@ static void ice_rem_all_chnl_fltrs(struct ice_pf *pf)
>  		hlist_del(&fltr->tc_flower_node);
>  		kfree(fltr);
>  	}
> +	mutex_lock(&pf->tc_flower_fltr_lock);
>  }

[Severity: High]
Was this meant to be a mutex_unlock(), with a matching mutex_lock() before
the hlist_for_each_entry_safe() loop?

As written, none of this work holds tc_flower_fltr_lock:

  - the list walk
  - the ice_rem_adv_rule_by_id() calls
  - the num_chnl_fltr and num_dmac_chnl_fltrs decrements
  - the hlist_del() and kfree(fltr)

The function then returns with the mutex held, and nothing releases it.

Can the race described in the commit message still happen?
ice_del_cls_flower() can take the mutex, find fltr and send the firmware
delete. Meanwhile the service task can run this on the same node without
the lock:

ice_prepare_for_reset()
  ice_remove_q_channels(vsi, true)
    ice_rem_all_chnl_fltrs()
      hlist_del(&fltr->tc_flower_node);
      kfree(fltr);

ice_del_cls_flower() would then call hlist_del() and kfree() on memory
that is already freed. Both paths could also decrement the filter counters
for the same rule.

The ice_is_reset_in_progress() check in ice_add_cls_flower() still runs
before the lock is taken.

Also, once any call to ice_rem_all_chnl_fltrs() returns (even with an
empty list), won't every later ice_add_cls_flower() and
ice_del_cls_flower() block forever in
mutex_lock(&pf->tc_flower_fltr_lock)? The next ice_rem_all_chnl_fltrs()
call would block as well.

This looks reachable from ice_prepare_for_reset() in the service task, on
a non-PF reset with ICE_FLAG_TC_MQPRIO set. It is also reachable from
ice_setup_tc_mqprio_qdisc() on mqprio channel teardown, which returns to
user space with the lock held:

	if (!hw && !test_bit(ICE_FLAG_TC_MQPRIO, pf->flags))
		ice_remove_q_channels(vsi, true);

The channel-setup cleanup path that calls ice_remove_q_channels() is
affected the same way. ice_deinit_pf() would then call mutex_destroy() on
a mutex that is still locked, which CONFIG_DEBUG_MUTEXES warns about.

Changing only this trailing mutex_lock() to mutex_unlock() would not close
the race. The lock needs to cover the whole loop.

> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index d20357c04127..955ad3cc5417 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c

[ ... ]

> @@ -2340,10 +2356,12 @@ void ice_replay_tc_fltrs(struct ice_pf *pf)
>  	struct ice_tc_flower_fltr *fltr;
>  	struct hlist_node *node;
>  
> +	mutex_lock(&pf->tc_flower_fltr_lock);
>  	hlist_for_each_entry_safe(fltr, node,
>  				  &pf->tc_flower_fltr_list,
>  				  tc_flower_node) {
>  		fltr->extack = NULL;
>  		ice_add_switch_fltr(fltr->src_vsi, fltr);
>  	}
> +	mutex_unlock(&pf->tc_flower_fltr_lock);
>  }

[Severity: Low]
Does anything call ice_replay_tc_fltrs()? The commit message says:

  and ice_replay_tc_fltrs() replays the same list after reset.

It also lists these as consequences:

  stale filter state after replay

  or reset/replay can traverse a list concurrently modified by flower
  control operations.

A search of drivers/net/ethernet/intel/ only finds the kernel-doc and
definition in ice_tc_lib.c, plus the prototype in ice_tc_lib.h. The reset
and rebuild paths don't appear to call it, so the locking added here has
no runtime effect.

Could the commit message describe only the paths that actually walk the
list: ice_add_cls_flower(), ice_del_cls_flower() and
ice_rem_all_chnl_fltrs()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020033457.523151-1-18983559317%40163.com