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

[netdev-bot+sashiko@kernel.org]

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] The patch puts the new lock in the wrong place in
  ice_rem_all_chnl_fltrs() (drivers/net/ethernet/intel/ice/ice_main.c).
- [Low] The commit message describes ice_replay_tc_fltrs() as an active
  reader that "replays the same list after reset", and lists "stale…

--

ice: Lock TC flower filter list operations

This commit adds a per-PF mutex, tc_flower_fltr_lock, to serialize access
to pf->tc_flower_fltr_list. The mutex is taken in ice_add_cls_flower(),
ice_del_cls_flower() and ice_replay_tc_fltrs(). A lock call is also added
to ice_rem_all_chnl_fltrs() on the queue-channel teardown path.

> 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_lock() before the hlist_for_each_entry_safe()
loop, with a matching mutex_unlock() after it?

As written, the lock is taken only after the walk has finished, and nothing
releases it. So ice_rem_all_chnl_fltrs() always returns with
tc_flower_fltr_lock held, even when the list is empty.

ice_remove_q_channels(vsi, true) reaches this function from these paths:

  ice_prepare_for_reset()
    (non-PFR reset with ICE_FLAG_TC_MQPRIO set, in the service task)
  ice_setup_tc_mqprio_qdisc()
    (mqprio qdisc removal, under RTNL and tc_mutex)
  ice_rebuild_channels()
    (cleanup error path, with rem_adv_fltr)

After the first call, every later ice_add_cls_flower() and
ice_del_cls_flower() blocks forever in mutex_lock().

A second ice_rem_all_chnl_fltrs() call deadlocks on itself. One way to hit
this is to add an mqprio channel qdisc, delete it, add it again and delete
it again. The second delete then hangs while holding RTNL. If the second
call comes from ice_prepare_for_reset() instead, the reset never finishes.

ice_deinit_pf() would also end up calling mutex_destroy() on a mutex that
is still locked.

The race described in the commit message also looks like it is still
open. The walk, the ice_rem_adv_rule_by_id() calls, the num_chnl_fltr and
num_dmac_chnl_fltrs updates, hlist_del() and kfree() all still run without
the lock.

ice_del_cls_flower() can find a filter under the mutex while this loop
frees it:

  ice_del_cls_flower()
    fltr = ice_find_tc_flower_fltr(pf, cls_flower->cookie);
    ...
    fltr->extack = cls_flower->common.extack;
    err = ice_del_tc_fltr(vsi, fltr);

Could that write freed memory, or lead to a double hlist_del() and
kfree() of the same fltr?

The reverse also seems possible. The locked delete path can free the node
this unlocked safe iterator has already cached in node. A concurrent
hlist_add_head() in ice_add_cls_flower() can also corrupt the list during
the walk.

> 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."

and lists among the consequences:

  "stale filter state after replay"

The only references to ice_replay_tc_fltrs() in drivers/net/ethernet/intel/ice
are its definition and kernel-doc in ice_tc_lib.c and its prototype in
ice_tc_lib.h. Since nothing calls it, it does not run after reset and
cannot race with flower add or delete. The locking added here has no
effect.

Could the commit message be updated so it does not list
ice_replay_tc_fltrs() as a race participant or replay as a consequence?

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