Re: [PATCH net-next v2 12/12] ice: use ACL for ntuple rules that conflict with FDir

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 (1) · Medium: 2 · Low: 4

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] ice_fdir_has_input_set_conflict()
  (drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c:1543) reports a…
- [High] The new conflict check dereferences a possibly-NULL aRFS counter
  block.
- [Medium] Conflict-routed rules that carry FLOW_EXT are rejected by the
  ACL path with -EINVAL even when they contain no flex match, because…
- [Medium] ice_fdir_has_input_set_conflict() makes the FDir-vs-ACL
  routing decision from state that can change before the decision is…
- [Low] Claimed regression: the new `if (userdata.flex_fltr) return
  -EOPNOTSUPP;` block in ice_add_ntuple_ethtool() sits inside the…
- [Low] The two new messages in the ACL branch of
  ice_add_ntuple_ethtool() describe an offload that may never occur.
- [Low] ice_fdir_has_input_set_conflict() documents `false` as "can use
  FD" and ice_add_ntuple_ethtool() consumes it as permission to skip the…
- [Low] Documentation/networking/device_drivers/ethernet/intel/ice.rst
  (lines 326-345) still states unconditionally that "For each flow-type,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new input-
  set-dependent backend selection makes it reachable for full-mask…

--

ice: use ACL for ntuple rules that conflict with FDir

This adds a pre-check that builds the candidate IP flow segment for an
ethtool ntuple rule, compares it against the active non-tunneled Flow
Director profile, and routes the rule to ACL when the input sets differ.
The IP flow segment setup is factored out into ice_set_fdir_ip_flow_seg()
so both the extraction-sequence path and the new check can use it.

A few questions below about the new helper and the ACL dispatch.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c b/drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c
> index fef8640d23fb6..691b4500e6be6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c
> @@ -1484,6 +1484,120 @@ ice_set_fdir_vlan_seg(struct ice_flow_seg_info *seg,
>  	return 0;
>  }
>  
> +/**
> + * ice_set_fdir_ip_flow_seg - set IP flow segment based on ethtool flow type
> + * @fsp: pointer to ethtool Rx flow specification
> + * @seg: flow segment for programming
> + * @perfect_fltr: valid on success; returns true if perfect fltr, false if not
> + *
> + * Return: 0 on success and errno in case of error.
> + */
> +static int ice_set_fdir_ip_flow_seg(struct ethtool_rx_flow_spec *fsp,
> +				    struct ice_flow_seg_info *seg,
> +				    bool *perfect_fltr)
> +{
> +	switch (fsp->flow_type & ~FLOW_EXT) {

[ ... ]

> +	case TCP_V6_FLOW:
> +		return ice_set_fdir_ip6_seg(seg, &fsp->m_u.tcp_ip6_spec,
> +					    ICE_FLOW_SEG_HDR_TCP, perfect_fltr);
> +	case UDP_V6_FLOW:
> +		return ice_set_fdir_ip6_seg(seg, &fsp->m_u.tcp_ip6_spec,
> +					    ICE_FLOW_SEG_HDR_UDP, perfect_fltr);
> +	case SCTP_V6_FLOW:
> +		return ice_set_fdir_ip6_seg(seg, &fsp->m_u.tcp_ip6_spec,
> +					    ICE_FLOW_SEG_HDR_SCTP,
> +					    perfect_fltr);
> +	case IPV6_USER_FLOW:
> +		return ice_set_fdir_ip6_usr_seg(seg, &fsp->m_u.usr_ip6_spec,
> +						perfect_fltr);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

[ ... ]

> +static bool
> +ice_fdir_has_input_set_conflict(struct ice_pf *pf,
> +				struct ethtool_rx_flow_spec *fsp,
> +				const struct ice_rx_flow_userdef *user)
> +{
> +	struct ice_flow_seg_info *test_seg, *old_seg;
> +	bool perfect_fltr = false, conflict = false;
> +	struct ice_fd_hw_prof *hw_prof;
> +	struct ice_hw *hw = &pf->hw;
> +	enum ice_fltr_ptype flow;
> +	int err;
> +
> +	if ((fsp->flow_type & ~FLOW_EXT) == ETHER_FLOW)
> +		return false;

[Severity: High]
This gate excludes only ETHER_FLOW, and ice_set_fdir_ip_flow_seg() above
happily builds segments for TCP_V6_FLOW, UDP_V6_FLOW, SCTP_V6_FLOW and
IPV6_USER_FLOW, so a true verdict can be produced for IPv6 rules.

The only consumer of a true verdict is ice_acl_add_rule_ethtool(), which
starts with ice_acl_prof_add_ethtool().  Can ACL program IPv6 flow types
at all?

drivers/net/ethernet/intel/ice/ice_acl_main.c:ice_acl_prof_add_ethtool() {
	switch (fsp->flow_type & ~FLOW_EXT) {
	case TCP_V4_FLOW:
	...
	case IPV4_USER_FLOW:
	...
	default:
		err = -EOPNOTSUPP;
	}
}

With this sequence on an ACL-capable device:

  ethtool -U ethX flow-type tcp6 src-ip A dst-ip B src-port P dst-port Q action 1
  ethtool -U ethX flow-type tcp6 src-ip C action 2

the second rule has a full mask, so ice_is_acl_filter() returns false (it
only inspects the IPv4 specs), while ice_fdir_has_input_set_conflict()
returns true.  Does the rule then end up in ice_acl_prof_add_ethtool()'s
default case and get rejected with -EOPNOTSUPP, instead of being offloaded?

If so, the descriptive rejection that used to come from
ice_fdir_set_hw_fltr_rule():

	dev_err(dev, "Failed to add filter. Flow director filters on each port must have the same input set.\n");
	return -EINVAL;

is no longer reached for these rules.  Since the new gate only tests
pf->hw.acl_tbl (ACL block present) and never "ACL can offload this flow
type", should the conflict detection be restricted to the flow types
ice_acl_prof_add_ethtool() supports?

> +
> +	flow = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT);
> +	if (flow >= ICE_FLTR_PTYPE_MAX || !hw->fdir_prof ||
> +	    !hw->fdir_prof[flow]) {
> +		return false;
> +	}
> +
> +	hw_prof = hw->fdir_prof[flow];
> +	old_seg = hw_prof->fdir_seg[ICE_FD_HW_SEG_NON_TUN];
> +
> +	/* A profile with no ethtool FDir filters (fdir_fltr_cnt == 0) may
> +	 * still be locked by aRFS perfect (4-tuple) filters, which keep their
> +	 * own active counters separate from fdir_fltr_cnt.
> +	 */
> +	if (!old_seg || (hw->fdir_fltr_cnt[flow] == 0 &&
> +			 !ice_is_arfs_using_perfect_flow(hw, flow)))
> +		return false;

[Severity: High]
Can ice_is_arfs_using_perfect_flow() be called here with
vsi->arfs_fltr_cntrs still NULL?

With CONFIG_RFS_ACCEL=y it dereferences the counter block without a NULL
check:

drivers/net/ethernet/intel/ice/ice_arfs.c:ice_is_arfs_using_perfect_flow() {
	arfs_fltr_cntrs = vsi->arfs_fltr_cntrs;

	/* active counters can be updated by multiple CPUs */
	smp_mb__before_atomic();
	switch (flow_type) {
	case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
		return atomic_read(&arfs_fltr_cntrs->active_udpv4_cnt) > 0;
	...
}

ice_set_features() enables Flow Director and ACL before the fallible aRFS
setup:

drivers/net/ethernet/intel/ice/ice_main.c:ice_set_features() {
	ice_vsi_manage_fdir(vsi, ena);
	ice_vsi_manage_acl(vsi, ena);
	ena ? ice_init_arfs(vsi) : ice_clear_arfs(vsi);
}

and ice_init_arfs() swallows the allocation failure:

drivers/net/ethernet/intel/ice/ice_arfs.c:ice_init_arfs() {
	if (ice_init_arfs_cntrs(vsi))
		goto free_arfs_fltr_list;
	...
}

so after "ethtool -K ethX ntuple on" with a failing allocation,
ICE_FLAG_FD_ENA is set, ice_fdir_create_dflt_rules() has already installed
the default perfect tcp4/udp4/tcp6/udp6 profiles, and
vsi->arfs_fltr_cntrs is NULL.  A following "ethtool -U ethX flow-type
tcp4 ..." on a device with hw->acl_tbl set reaches this check with
old_seg != NULL and hw->fdir_fltr_cnt[flow] == 0.  Does that oops inside
atomic_read()?

The missing NULL check in ice_is_arfs_using_perfect_flow() predates this
patch, but previously ice_fdir_set_hw_fltr_rule() compared the segments
first and returned -EEXIST without consulting the aRFS counters, so rules
whose input set matches the existing profile never touched them.  Does
this new call site widen that exposure to the common case?

[Severity: Medium]
Is the state read here stable until it is acted upon?

hw->fdir_prof[flow], hw_prof->fdir_seg[ICE_FD_HW_SEG_NON_TUN],
hw->fdir_fltr_cnt[flow] and the aRFS atomics are all read without
hw->fdir_fltr_lock; ice_add_ntuple_ethtool() takes that mutex only later,
after ice_cfg_fdir_xtrct_seq() has already programmed the profile.  The
aRFS counters are written from contexts that are not serialized against
the ethtool ioctl:

ice_service_task()
  ice_sync_arfs_fltrs()
    ice_arfs_add_flow_rules()
      ice_arfs_update_active_fltr_cntrs()

and also from ice_rx_flow_steer().

So:

CPU0 (ethtool -U)
  ice_fdir_has_input_set_conflict()
    hw->fdir_fltr_cnt[flow] == 0
    aRFS tcpv4 counter == 0  -> returns false, take the FDir path

CPU1 (service task)
  ice_arfs_add_flow_rules()
    ice_arfs_update_active_fltr_cntrs()  -> counter becomes 1

CPU0 continues into ice_fdir_set_hw_fltr_rule(), which re-reads the same
state:

		if (ice_is_arfs_using_perfect_flow(hw, flow)) {
			dev_err(dev, "aRFS using perfect flow type %d, cannot change input set\n",
				flow);
			return -EINVAL;
		}

Can that produce the -EINVAL this patch is meant to avoid, with no ACL
fallback left at that point?  The mirror ordering would route to ACL a
rule Flow Director could have taken.

> +
> +	test_seg = kzalloc_obj(*test_seg);
> +	if (!test_seg)
> +		return false;

[Severity: Low]
The kernel-doc for this helper documents false as "can use FD", and
ice_add_ntuple_ethtool() treats false as permission to skip the ACL
branch.  Here false also means "the comparison could not be performed".

On this allocation failure the rule goes down the Flow Director path and
ice_fdir_set_hw_fltr_rule() rejects the differing input set:

		if (hw->fdir_fltr_cnt[flow]) {
			dev_err(dev, "Failed to add filter. Flow director filters on each port must have the same input set.\n");
			return -EINVAL;
		}

Should this report -ENOMEM (or still take the ACL route) rather than
returning the value that means no conflict?

The other early false path, ice_set_fdir_ip_flow_seg() returning an error,
looks benign since the FDir path re-runs the same builder and produces the
same errno.

> +
> +	err = ice_set_fdir_ip_flow_seg(fsp, test_seg, &perfect_fltr);
> +
> +	if (err) {
> +		kfree(test_seg);
> +		return false;
> +	}
> +
> +	if (user && user->flex_fltr)
> +		ice_flow_add_fld_raw(test_seg, user->flex_offset,
> +				     ICE_FLTR_PRGM_FLEX_WORD_SIZE,
> +				     ICE_FLOW_FLD_OFF_INVAL,
> +				     ICE_FLOW_FLD_OFF_INVAL);
> +
> +	/* Compare the test segment with the existing segment */
> +	if (memcmp(old_seg, test_seg, sizeof(*test_seg)) != 0)
> +		conflict = true;
> +
> +	kfree(test_seg);
> +
> +	return conflict;
> +}
> +
>  /**
>   * ice_cfg_fdir_xtrct_seq - Configure extraction sequence for the given filter
>   * @pf: PF structure

[ ... ]

> @@ -2368,9 +2441,27 @@ int ice_add_ntuple_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd)
>  		return -ENOSPC;
>  	}
>  
> -	/* ACL filter */
> -	if (pf->hw.acl_tbl && ice_is_acl_filter(fsp))
> +	/* ACL filter, or this filter would cause an input set conflict with
> +	 * existing FD filters
> +	 */
> +	if (pf->hw.acl_tbl &&
> +	    (ice_is_acl_filter(fsp) ||
> +	     ice_fdir_has_input_set_conflict(pf, fsp, &userdata))) {
> +		/* The ACL programming path does not honor flex byte
> +		 * (user-def) constraints. Routing a flex filter to ACL would
> +		 * silently drop the flex match and offload a much broader
> +		 * rule than requested, so reject it explicitly instead.
> +		 */
> +		if (userdata.flex_fltr) {
> +			dev_info(dev, "Failed to add filter. Flex byte (user-def) filters cannot be offloaded to ACL.\n");
> +			return -EOPNOTSUPP;
> +		}

[Severity: Low]
This new rejection sits inside the combined condition, so it also applies
to the pre-existing partial-mask ACL route selected by
ice_is_acl_filter().

Such rules already failed before this patch, just with a different errno:
flex_fltr requires FLOW_EXT (ice_parse_rx_flow_user_data() returns early
when !(fsp->flow_type & FLOW_EXT)), and the ACL decoder strips only
FLOW_MAC_EXT:

drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c:ice_ntuple_set_input_set() {
	else if (blk == ICE_BLK_ACL)
		flow_mask = FLOW_MAC_EXT;
	...
	flow_type = fsp->flow_type & ~flow_mask;
	...
	switch (flow_type) {
	...
	default:
		return -EINVAL;
	}
}

So a command like

  ethtool -U ethX flow-type tcp4 src-ip A m 0.255.255.255 ... user-def 0x4ffff

returned -EINVAL before and returns -EOPNOTSUPP now.  Would it be worth
mentioning that user-visible errno change in the changelog, since it is
not a consequence of the conflict detection being added?

> +
> +		dev_dbg(dev, "ntuple filter at location %d offloaded to ACL instead of Flow Director\n",
> +			fsp->location);

[Severity: Low]
This message is printed before ice_acl_add_rule_ethtool() is called, so it
claims an offload that may never happen.  For flow types that
ice_acl_prof_add_ethtool() rejects:

drivers/net/ethernet/intel/ice/ice_acl_main.c:ice_acl_prof_add_ethtool() {
	default:
		err = -EOPNOTSUPP;
}

the log says the rule was offloaded to ACL while the command failed.
Should it be emitted only after ice_acl_add_rule_ethtool() succeeds?

Similarly, the dev_info above attributes the failure to an ACL flex byte
limitation even for tcp6/udp6/sctp6/ip6, where ACL cannot take the rule at
all and the real blocker is the Flow Director input set conflict.

> +
>  		return ice_acl_add_rule_ethtool(vsi, cmd);
> +	}

[Severity: Medium]
Is FLOW_EXT normalized consistently between the new selector and the ACL
destination?

ice_fdir_has_input_set_conflict() and ice_acl_prof_add_ethtool() both mask
with & ~FLOW_EXT, but ice_ntuple_set_input_set() uses FLOW_MAC_EXT for
ICE_BLK_ACL, so FLOW_EXT survives into its switch and hits
default: return -EINVAL.

A command that sets FLOW_EXT without any flex match, for example

  ethtool -U ethX flow-type tcp4 src-ip C vlan 0x1 m 0xffff action 2

against an existing conflicting tcp4 FDir profile, leaves flex_fltr false
(m_ext.data is zero), so it passes the check above and is routed to ACL,
where it fails with -EINVAL.

By that point ice_acl_prof_add_ethtool() has already created and
programmed a profile for the flow type:

	hw->acl_prof[fltr_type] = hw_prof;

Does the failed command leave that profile registered, so later ACL rules
for the same flow type with a different input set fail the
if (hw_prof->seg) memcmp check?

[Severity: High]
This isn't a bug introduced by this patch, but the new input-set-dependent
backend selection makes it reachable for full-mask rules: replacing an
ACL-backed ntuple rule with an FDir-backed rule at the same location looks
like it leaves the old ACL TCAM entry programmed.

The only ACL hardware removal in ice_ntuple_update_list_entry() is gated
on the replacement pointer being NULL:

drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c:ice_ntuple_update_list_entry() {
	if (old_fltr->acl_fltr) {
		...
		if (!input) {
			err = ice_del_acl_ethtool(hw, old_fltr);
			if (err)
				return err;
		}
	}
	...
	list_del(&old_fltr->fltr_node);
	kfree(old_fltr);
}

which holds only for ice_acl_add_rule_ethtool() replacing ACL with ACL.
The Flow Director path afterwards calls ice_fdir_write_all_fltr(), which
does not touch the ACL table.

Sequence:

  1. add an FDir rule matching src-ip
  2. add a full-mask dst-ip rule at location L; the new conflict check
     routes it to ACL
  3. re-add at location L a src-ip rule compatible with the active FDir
     profile; it now takes the FDir path

Does the ACL entry for the dst-ip match survive step 3 with no software
record, so it keeps dropping or steering matching packets and is invisible
to ethtool -u?

[Severity: Low]
Should Documentation/networking/device_drivers/ethernet/intel/ice.rst be
updated along with this?  The Flow Director Filters notes still say
unconditionally:

  For each flow-type, the programmed filters must all have the same
  matching input set.

and the example there, showing

  ethtool -U ethX flow-type ip4 src-ip 192.168.0.1 src-port 5300 action 7
  ethtool -U ethX flow-type ip4 dst-ip 192.168.0.5 src-port 55 action 10

"will fail with an error", plus "on one device, you may not program two
tcp4 filters with different matching fields".  On an ACL-capable device
that second rule now succeeds through ACL.  The new flex byte limitation
is also undocumented.

>  
>  	/* Only fdir filters below */
>  	if (!test_bit(ICE_FLAG_FD_ENA, pf->flags))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917183927.2692068-1-anthony.l.nguyen%40intel.com