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

[Petr Oros <poros@redhat.com>]

On 9/12/26 10:36, Simon Horman wrote:
> 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@redhat.com?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.
Correct. For a block bound to multiple devices of one PF only the first
binding's filter_dev is remembered, so the guard protects the first
binding only. The impact is a missed refusal on the other bindings,
never a false refusal, but it is easy to do better. I will rework the
tracking in v2 to keep one entry per cookie and filter_dev pair, so
every binding replay creates its own entry and the guard covers all of
them.
>
>> +
>> +	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?
Yes, same root cause as above, fixed by the same v2 rework. The
per device comparison itself has to stay because TC priorities are not
ordered across blocks, only the tracking granularity was wrong.
>
>> +		    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?
They will, though that is what every DESTROY for an unknown cookie
returned before this patch too, so nothing gets worse. With the v2
per binding tracking each DESTROY invocation removes its own entry and
returns 0, which fixes this for tracked filters as a side effect.
>
>>   	/* 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?
Confirmed, this race is real and pre-existing. ice_prepare_for_reset()
runs from the service task without RTNL and for a non PFR reset with
mqprio active it calls ice_remove_q_channels() with rem_fltr true,
which walks and frees tc_flower_fltr_list entries concurrently with
the RTNL serialized cls_flower paths. It predates this series and the
list reader added here runs in the same RTNL context as the existing
readers, so this series does not widen it. I will address it in a
separate fix.

I will send v2 of this series with the per binding tracking rework.

Regards,

Petr