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/[email protected]?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?
