Re: [PATCH iwl-next 2/2] ice: don't offload drop filters that bypass higher priority filters

[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/#/patchset/20260910154824.3603687-2-poros@xxxxxxxxxx?part=2
---
ice: don't offload drop filters that bypass higher priority filters

This commit tracks software-only TC filters to prevent the hardware from
incorrectly offloading lower priority drop rules that would override them.
It tracks un-offloaded filters within a device block and rejects conflicting
drop offloads.

> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
[ ... ]
> @@ -2269,6 +2269,112 @@ ice_find_tc_flower_fltr(struct ice_pf *pf, unsigned long cookie)
[ ... ]
> +static void ice_tc_track_sw_fltr(struct ice_pf *pf,
> +				 struct net_device *filter_dev,
> +				 struct flow_cls_offload *cls_flower,
> +				 enum ice_eswitch_fltr_direction direction)
> +{
> +	struct ice_tc_sw_fltr *sw_fltr;
> +
> +	hlist_for_each_entry(sw_fltr, &pf->tc_sw_fltr_list, node)
> +		if (sw_fltr->cookie == cls_flower->cookie)
> +			return;

[Severity: High]
Does this early return break tracking for TC shared blocks?

When a shared block is bound to multiple interfaces, the driver will only
track the cookie for the first interface because this check returns early
when it sees the same cookie.

> +
> +	sw_fltr = kzalloc_obj(*sw_fltr);
[ ... ]
> +static bool
> +ice_tc_drop_bypasses_fltr(struct ice_pf *pf, struct net_device *filter_dev,
> +			  u32 prio, enum ice_eswitch_fltr_direction direction)
> +{
> +	struct ice_tc_flower_fltr *fltr;
> +	struct ice_tc_sw_fltr *sw_fltr;
> +
> +	hlist_for_each_entry(sw_fltr, &pf->tc_sw_fltr_list, node)
> +		if (sw_fltr->filter_dev == filter_dev &&

[Severity: High]
Because the tracking entry is bound strictly to the first interface's
filter_dev, won't this check fail to protect other ports sharing the same
block?

> +		    sw_fltr->direction == direction && sw_fltr->prio < prio &&
> +		    !sw_fltr->is_drop)
> +			return true;
[ ... ]
> @@ -2336,6 +2471,9 @@ ice_del_cls_flower(struct ice_vsi *vsi, struct flow_cls_offload *cls_flower)
>  	struct ice_pf *pf = vsi->back;
>  	int err;
>  
> +	if (ice_tc_untrack_sw_fltr(pf, cls_flower->cookie))
> +		return 0;
> +

[Severity: High]
When deleting a filter from a shared block, the first net_device will delete
the unique tracking entry here. Won't subsequent callback invocations for the
other ports in the shared block fall through to the hardware failure path
and incorrectly return -EINVAL?

>  	/* find filter */
>  	fltr = ice_find_tc_flower_fltr(pf, cls_flower->cookie);
>  	if (!fltr) {

Later in ice_del_cls_flower(), the filter is removed and freed:

drivers/net/ethernet/intel/ice/ice_tc_lib.c:ice_del_cls_flower() {
    ...
	/* delete filter from an ordered list */
	hlist_del(&fltr->tc_flower_node);

	/* free the filter node */
	kfree(fltr);
    ...
}

[Severity: High]
This is a pre-existing issue, but is it safe to modify tc_flower_fltr_list
and free the filter here?

While this runs under the rtnl_lock, the background device reset task
(ice_reset_subtask) calls ice_rem_all_chnl_fltrs(), which iterates over
pf->tc_flower_fltr_list and frees filters using kfree() without holding
the rtnl_lock or any driver-specific lock protecting the list.

Could this concurrent access during a hardware reset result in list corruption
or a use-after-free?