Re: [PATCH net-next v2 09/12] ice: program ACL entry

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

Critical: 0 · High: 2 · Medium: 9 · Low: 6

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_flow_acl_add_scen_entry_sync() merges two distinct ethtool
  rules (different 'location' slots) onto one ACL scenario entry…
- [High] Replacing an ACL rule with a Flow Director rule at the same
  ethtool location leaves the old ACL TCAM entry programmed and active.
- [Medium] In ice_ntuple_update_list_entry() the new last-ACL-filter
  cleanup calls ice_fdir_rem_flow(hw, ICE_BLK_ACL, old_fltr->flow_type),…
- [Medium] ice_set_fd_vsi_ctx() now unconditionally sets
  ICE_AQ_VSI_PROP_ACL_VALID and programs acl_def_act (hardcoded miss-…
- [Medium] ACL ice_ntuple_fltr objects are inserted into
  hw->fdir_list_head (input->acl_fltr = true) but the teardown paths…
- [Medium] ACL rule placement ignores the ethtool rule-ordering contract.
- [Medium] In ice_acl_add_rule_ethtool(), the new `del_entry` error path
  unconditionally calls ice_flow_rem_entry(hw, ICE_BLK_ACL, entry_h).
- [Medium] ice_flow_rem_prof_sync() iterates prof->entries calling
  ice_flow_rem_entry_sync() and only `break`s on the first failure; the…
- [Medium] ice_acl_rem_entry() only releases the scenario slot
  (ice_acl_scen_free_entry_idx(), i.e. clearing the entry_bitmap bit…
- [Medium] In the in-place action-update branch (do_chg_action) of
  ice_flow_acl_add_scen_entry_sync(), the driver releases the existing…
- [Medium] The new ice_flow_find_entry() converts a struct ice_flow_entry
  pointer into a u64 'handle' and returns it after releasing both…
- [Low] Concern that ACL filters are never reprogrammed after a PF/core
  reset (ice_fdir_replay_fltrs() skips acl_fltr entries and…
- [Low] ice_acl_prof_aq_send() is extended to set LIBIE_AQ_FLAG_RD for
  ice_aqc_opc_program_acl_prof_ranges, but that opcode can never reach…
- [Low] ice_flow_find_entry()'s kernel-doc states it returns
  'ICE_FLOW_ENTRY_ID_INVAL' when no entry is found, but no such macro…
- [Low] The new struct ice_acl_scen.entry_bitmap comment states that a
  set bit means the entry is available, while the allocator and free…
- [Low] ice_flow_acl_find_scen_entry_cond() bounds its action-comparison
  loop with the existing entry's count but dereferences both arrays:…
- [Low] ice_acl_add_entry() programs all cascaded TCAM slices - including
  the leftmost slice that completes the match - before calling…

--

ice: program ACL entry

This completes the ACL filter programming path: the formatted key and the
action set are written into the scenario's TCAM entry and action memories,
scenario entry allocation/removal helpers are added, and the VSI context is
extended with the ACL section.  Masked ntuple rules for tcp4, udp4, sctp4
and ip4 can now be installed through ethtool.

A few questions on the new code below.

[Severity: Low]
Can ice_aqc_opc_program_acl_prof_ranges ever reach ice_acl_prof_aq_send()?
The only callers are ice_prgm_acl_prof_xtrct() (extraction) and
ice_query_acl_prof() (query prof).

Also, this helper's buf parameter is struct ice_aqc_acl_prof_generic_frmt *
and it always sends sizeof(*buf) of that type, while 0x0C1E takes a
struct ice_aqc_acl_profile_ranges buffer.  The new
ice_prog_acl_prof_ranges() added by this patch builds its own descriptor
and sets LIBIE_AQ_FLAG_RD itself, so should this branch be dropped?

[ ... ]

[Severity: Low]
Is the polarity in this comment inverted?  The users treat a set bit as
"allocated":

	if (!test_and_set_bit(i, scen->entry_bitmap))
		return i;			/* allocate */

	if (!test_and_clear_bit(idx, scen->entry_bitmap))
		return -ENOENT;			/* free */

	if (!test_bit(entry_idx, scen->entry_bitmap))
		return -ENOENT;			/* remove */

[ ... ]

[Severity: Medium]
How is the ethtool rule ordering contract honoured here?
include/uapi/linux/ethtool.h says for struct ethtool_rx_flow_spec:

	 * @location: Location of rule in the table.  Locations must be
	 *	numbered such that a flow matching multiple rules will be
	 *	classified according to the first (lowest numbered) rule.

ice_acl_add_rule_ethtool() passes ICE_FLOW_PRIO_NORMAL for every rule and
uses fsp->location only as the flow entry id, and this function picks the
first free slot of the priority partition without ever seeing the
location.  For two overlapping masked rules, doesn't the hardware match
order end up being the order in which they were added rather than the
location order?

[ ... ]

[ ... ]

+		offset = num_cscd - i - 1;
+		memcpy(&buf.entry_key.val,
+		       &keys[offset * sizeof(buf.entry_key.val)],
+		       sizeof(buf.entry_key.val));
+		memcpy(&buf.entry_key_invert.val,
+		       &inverts[offset * sizeof(buf.entry_key_invert.val)],
+		       sizeof(buf.entry_key_invert.val));
+		err = ice_aq_program_acl_entry(hw, entry_tcam + offset, idx,
+					       &buf, NULL);
+		if (err) {
+			ice_debug(hw, ICE_DBG_ACL, "aq program acl entry failed status: %d\n",
+				  err);
+			goto out;
+		}
+	}
+
+	err = ice_acl_prog_act(hw, scen, acts, acts_cnt, *entry_idx);

[Severity: Low]
Is there a window here where the entry matches but has no action?  The
loop programs all cascaded slices, including the leftmost one that
completes the match, and only afterwards does ice_acl_prog_act() program
the action memory in a separate AdminQ command.

The action memories are zeroed at table init, and mdid == 0 is documented
as NOP in struct ice_acl_act_entry, so for the duration of that extra
round trip a matching packet gets a NOP instead of the requested
drop/redirect.  If the new entry lands in a higher priority slot than an
existing overlapping rule, does it transiently shadow that rule?
Would programming the action memory first and publishing the completing
slice last avoid this?

+
+out:
+	if (err) {
+		ice_acl_rem_entry(hw, scen, *entry_idx);
+		*entry_idx = 0;
+	}
+
+	return err;
+}

[ ... ]

+int ice_acl_rem_entry(struct ice_hw *hw, struct ice_acl_scen *scen,
+		      u16 entry_idx)
+{

[ ... ]

+	for (i = 0; i < num_cscd; i++) {
+		int aq_err = ice_aq_program_acl_entry(hw, entry_tcam + i, idx,
+						      &buf, NULL);
+		if (aq_err) {
+			dev_warn(ice_hw_to_dev(hw), "AQ program ACL entry failed, status: %d\n",
+				 aq_err);
+			err = aq_err;
+		}
+	}

[ ... ]

+	if (!err)
+		err = ice_acl_scen_free_entry_idx(scen, entry_idx);
+
+	return err;
+}

[Severity: Medium]
[Sources: gpt-5-6-sol, gpt-6-astra, opus-5
Can this leak scenario entries?  The bit reserved by test_and_set_bit() in
ice_acl_scen_assign_entry_idx() is only released when every
ice_aq_program_acl_entry()/ice_aq_program_actpair() invalidation above
succeeded.  The loops themselves treat an AdminQ failure as non-fatal
(dev_warn() and continue), so on such a failure the bit stays set and no
other path clears it.

Each such removal would then permanently consume one TCAM slot until
ice_acl_scen_assign_entry_idx() starts returning
ICE_ACL_SCEN_ENTRY_INVAL and all further adds fail with -ENOSPC.

Related, the rollback in ice_acl_add_entry() calls
ice_acl_rem_entry(hw, scen, *entry_idx) and discards the return value
before zeroing *entry_idx, so a failed add can lose the index it just
reserved.

[Severity: Medium]
Is entry_h always a freshly created resource on this error path?  On the
update path ice_flow_acl_add_scen_entry_sync() can return the
pre-existing entry:

	*entry = exist;

which is still referenced by a live software filter record.  The
failures that reach del_entry include the early returns of
ice_ntuple_update_list_entry():

	if (ice_is_reset_in_progress(pf->state))
		return -EBUSY;

	vsi = ice_get_main_vsi(pf);
	if (!vsi)
		return -EINVAL;

Both happen before old_fltr is touched, so does the unconditional
ice_flow_rem_entry() here tear down the hardware state of a filter that
stays in hw->fdir_list_head?  Afterwards ice_del_acl_ethtool() cannot
find an entry for that id, ice_flow_rem_entry() returns -EINVAL, and
ice_ntuple_update_list_entry() returns before list_del()/kfree(), so the
rule is listed by ethtool -n forever while not being programmed.

[Severity: Medium]
With input->acl_fltr = true the filter is handed to hw->fdir_list_head,
but do the owners of that list free ACL entries?
ice_fdir_del_all_fltrs() starts with:

	list_for_each_entry_safe(f_rule, tmp, &hw->fdir_list_head, fltr_node) {
		if (f_rule->acl_fltr)
			continue;

and ice_deinit_acl() only calls ice_acl_rem_flows()/ice_acl_destroy_tbl()
and frees hw->acl_prof, never walking the filter list.  On unload
(ice_deinit_fdir() -> ice_vsi_manage_fdir(vsi, false) ->
ice_fdir_del_all_fltrs()) does every kzalloc'd struct ice_ntuple_fltr
with acl_fltr set leak?

The same skip also applies to ice_remove_q_channels(), which removes
ntuple filters precisely because queue configuration is changing but only
calls ice_fdir_del_all_fltrs(), so queue-directed ACL rules would keep
pointing at queues that no longer exist.

[Severity: Low]
At this point in the series ACL filters are not reprogrammed after a
PF/core reset: ice_fdir_replay_fltrs() skips acl_fltr records and
ice_rebuild() has no ACL restoration, so the rules stay listed by
ethtool -n while no longer matching in hardware.  This is addressed later
in the same series by the commit adding ice_acl_replay_flows() and
ice_acl_replay_fltrs() to ice_rebuild(), so it is only an issue for the
intermediate state at this commit.  Would folding the ordering
differently avoid the transient?

 free_input:
 	kfree(input);
 

[Severity: High]
Does this assumption hold for all callers?  The comment says the caller
has already applied the hardware change, which is true for
ice_acl_add_rule_ethtool() (it looks up old_entry_h and removes it), but
ice_add_ntuple_ethtool() also calls:

	ret = ice_ntuple_update_list_entry(pf, input, fsp->location);

with a non-NULL input and no ACL hardware removal at all.

So for an ACL rule at location N replaced by a Flow Director rule at the
same location, the "if (!input)" guard skips ice_del_acl_ethtool(), and a
few lines below:

		list_del(&old_fltr->fltr_node);
		kfree(old_fltr);

drops the only record of the ACL entry id.  Does that leave the old ACL
TCAM entry armed (possibly a drop rule) with no way for userspace to see
or delete it, plus a leaked ice_flow_entry, a leaked entry_bitmap slot
and a stale hw->acl_fltr_cnt?

+		} else {
+			/* FD filter */
+			err = ice_fdir_write_all_fltr(pf, old_fltr, false);
+			if (err)
+				return err;
+		}
+
 		ice_ntuple_update_cntrs(hw, old_fltr, false);
 		/* update sb-filters count, specific to ring->channel */
 		ice_update_per_q_fltr(vsi, old_fltr->orig_q_index, false);
-		if (!input && !hw->fdir_fltr_cnt[old_fltr->flow_type])
+		/* Also delete the HW filter info if we have just deleted the
+		 * last filter of flow_type.
+		 */
+		if (!old_fltr->acl_fltr && !input &&
+		    !hw->fdir_fltr_cnt[old_fltr->flow_type])
 			/* we just deleted the last filter of flow_type so we
 			 * should also delete the HW filter info.
 			 */
 			ice_fdir_do_rem_flow(pf, old_fltr->flow_type);
+		else if (old_fltr->acl_fltr && !input &&
+			 !hw->acl_fltr_cnt[old_fltr->flow_type])
+			ice_fdir_rem_flow(hw, ICE_BLK_ACL, old_fltr->flow_type);
+

[Severity: Medium]
Does this ICE_BLK_ACL call do anything?  ice_fdir_rem_flow() resolves the
profile with ice_fdir_get_hw_prof():

	if (blk == ICE_BLK_FD && hw->fdir_prof)
		return hw->fdir_prof[flow];

	return NULL;

and returns immediately on NULL, so for ICE_BLK_ACL it looks like an
unconditional no-op and hw->acl_prof[flow] is never torn down.

Then a later add for the same flow-type still sees the old input set in
ice_acl_prof_add_ethtool():

	if (hw_prof->seg) {
		if (!memcmp(hw_prof->seg, seg, sizeof(*seg))) {
			kfree(seg);
			return 0;
		}

		err = -EINVAL;

So after deleting all tcp4 ACL rules, does adding a tcp4 rule with a
different mask selection fail with -EINVAL for the rest of the
interface's lifetime?  Should this use the ACL API instead, the way
ice_acl_rem_flows() does with
ice_flow_rem_prof(hw, ICE_BLK_ACL, prof->prof_id)?  The two profile types
also differ (struct ice_acl_hw_prof vs struct ice_fd_hw_prof).

 		list_del(&old_fltr->fltr_node);
 		kfree(old_fltr);
 	}

[Severity: Medium]
This makes the ACL branch of ice_flow_rem_entry_sync() fallible for the
first time, and the early return happens before list_del() and
ice_dealloc_flow_entry().  Is the caller prepared for that?
ice_flow_rem_prof_sync() does:

		list_for_each_entry_safe(e, t, &prof->entries, l_entry) {
			status = ice_flow_rem_entry_sync(hw, blk, e);
			if (status)
				break;
		}

	mutex_unlock(&prof->entries_lock);

and the loop's status is then overwritten and execution continues to
ice_rem_prof() and devm_kfree(prof).

So on an AdminQ failure during teardown (for example
ice_acl_rem_flows() -> ice_flow_rem_prof() after a reset), does the
failing entry and everything after it stay linked in prof->entries and
never get freed, while the profile they point to through entry->prof is
devm_kfree()d?

 		if (entry->acts_cnt && entry->acts)
 			ice_flow_acl_free_act_cntr(hw, entry->acts,
 						   entry->acts_cnt);

[ ... ]

[Severity: Low]
ICE_FLOW_ENTRY_ID_INVAL does not exist anywhere in the tree; the code
returns ICE_FLOW_ENTRY_HANDLE_INVAL (defined as 0 in ice_flow.h), which
is also what the new callers compare against.

+u64 ice_flow_find_entry(struct ice_hw *hw, enum ice_block blk, u64 entry_id)
+{
+	struct ice_flow_entry *found = NULL;
+	struct ice_flow_prof *p;
+
+	mutex_lock(&hw->fl_profs_locks[blk]);
+
+	list_for_each_entry(p, &hw->fl_profs[blk], l_entry) {
+		struct ice_flow_entry *e;
+
+		mutex_lock(&p->entries_lock);
+		list_for_each_entry(e, &p->entries, l_entry)
+			if (e->id == entry_id) {
+				found = e;
+				break;
+			}
+		mutex_unlock(&p->entries_lock);
+
+		if (found)
+			break;
+	}
+
+	mutex_unlock(&hw->fl_profs_locks[blk]);
+
+	return found ? ICE_FLOW_ENTRY_HNDL(found) : ICE_FLOW_ENTRY_HANDLE_INVAL;
+}

[Severity: Medium]
What keeps the returned object alive?  The raw struct ice_flow_entry
pointer is published as a handle after both hw->fl_profs_locks[blk] and
p->entries_lock have been dropped, with no refcount and no RCU.

Both new callers then dereference it unlocked, since ice_flow_rem_entry()
reads entry->prof, takes prof->entries_lock and list_del()s
entry->l_entry:

ice_del_acl_ethtool()
	entry = ice_flow_find_entry(hw, ICE_BLK_ACL, fltr->fltr_id);
	return ice_flow_rem_entry(hw, ICE_BLK_ACL, entry);

In ice_acl_add_rule_ethtool() the handle is additionally cached across a
whole ice_flow_add_entry() call, and the only validity test is
old_entry_h != entry_h, which shows identity rather than liveness.  Would
resolving the id again under prof->entries_lock, or removing under that
lock, be safer than caching a bare pointer?

[ ... ]

+	list_for_each_entry(p, &prof->entries, l_entry) {
+		if (memcmp(p->entry, e->entry, p->entry_sz))
+			continue;
+
+		/* Now we have the same matching_data */
+
+		*do_add_entry = false;
+		return_entry = p;
+
+		if (p->priority != e->priority) {
+			/* matching data && !priority */
+			*do_add_entry = true;
+			*do_rem_entry = true;
+			break;
+		}
+
+		/* Now we have matching_data && priority */
+
+		if (p->acts_cnt != e->acts_cnt)
+			*do_chg_action = true;
+		for (int i = 0; i < p->acts_cnt; i++) {
+			if (memcmp(&p->acts[i], &e->acts[i],
+				   sizeof(struct ice_flow_action))) {

[Severity: Low]
Can this read past the end of e->acts?  The loop is bounded by
p->acts_cnt but indexes both arrays, and the check just above has already
established that the counts can differ.  e->acts comes from
ice_flow_acl_frmt_entry():

	e->acts = kmemdup(acts, acts_cnt * sizeof(*acts), GFP_KERNEL);

so it is sized exactly to e->acts_cnt.  Today every ACL entry is created
with ICE_ACL_NUM_ACT (1) and ice_flow_add_entry() rejects acts_cnt == 0
for ICE_BLK_ACL, so the counts are always equal, but
ICE_FLOW_ACL_MAX_NUM_ACT is 2.  Should this use
min(p->acts_cnt, e->acts_cnt), or skip the loop when the counts differ?

+				*do_chg_action = true;
+				break;
+			}
+		}

[ ... ]

+static int ice_flow_acl_add_scen_entry_sync(struct ice_hw *hw,
+					    struct ice_flow_prof *prof,
+					    struct ice_flow_entry **entry)
+{

[ ... ]

+	} else {
+		if (do_chg_action) {
+			/* For the action memory info, update the SW's copy of
+			 * exist entry with e's action memory info
+			 */
+			if (exist->acts_cnt && exist->acts)
+				ice_flow_acl_free_act_cntr(hw, exist->acts,
+							   exist->acts_cnt);
+			kfree(exist->acts);
+			exist->acts = kzalloc_objs(*exist->acts, e->acts_cnt);
+			if (!exist->acts) {
+				exist->acts_cnt = 0;
+				err = -ENOMEM;
+				goto out;
+			}
+			exist->acts_cnt = e->acts_cnt;
+
+			memcpy(exist->acts, e->acts,
+			       sizeof(struct ice_flow_action) * e->acts_cnt);
+
+			err = ice_acl_prog_act(hw, prof->cfg.scen, acts,
+					       e->acts_cnt,
+					       exist->scen_entry_idx);
+			if (err) {
+				/* HW programming failed; e's counters were not
+				 * transferred. Drop exist's new acts so the
+				 * caller's free_cntrs path does not release IDs
+				 * still referenced by exist->acts.
+				 */
+				kfree(exist->acts);
+				exist->acts = NULL;
+				exist->acts_cnt = 0;
+				goto out;
+			}

[Severity: Medium]
What describes the hardware state after this error path?  The existing
entry's counters were already released by ice_flow_acl_free_act_cntr()
before any hardware was touched, and ice_acl_prog_act() issues one
ice_aq_program_actpair() per matching action memory in the cascade, so a
failure on the second or later command leaves some action memories
already updated.

This path then sets exist->acts = NULL and exist->acts_cnt = 0 but leaves
the TCAM entry programmed and exist on prof->entries, so the device keeps
executing an action set (old, new, or a mix) that the driver no longer
tracks.

Also, with acts_cnt zeroed, ice_flow_rem_entry_sync() will skip
ice_flow_acl_free_act_cntr():

		if (entry->acts_cnt && entry->acts)
			ice_flow_acl_free_act_cntr(hw, entry->acts,
						   entry->acts_cnt);

so are the counter IDs still referenced by the action memory
unreclaimable, while the ones released earlier can be handed out to
another filter?

+
+			/* e's counter IDs are now owned by exist->acts; clear
+			 * acts_cnt so the trailing ice_flow_acl_free_act_cntr
+			 * call does not release IDs that exist->acts owns.
+			 * ice_dealloc_flow_entry still frees the acts memory
+			 * via kfree(e->acts).
+			 */
+			e->acts_cnt = 0;
+		}

[ ... ]

+		/* As we don't add the new entry to our SW DB, free its HW
+		 * counter resources and deallocate its memory, then return
+		 * the exist entry to the caller. Transfer the new caller's
+		 * entry_id to exist so that ice_flow_find_entry() can locate
+		 * it and the caller's handle remains valid for deletion.
+		 */
+		if (e->acts_cnt && e->acts)
+			ice_flow_acl_free_act_cntr(hw, e->acts, e->acts_cnt);
+		exist->id = e->id;
+		kfree(e->entry);
+		kfree(e->range_buf);
+		kfree(e->acts);
+		devm_kfree(ice_hw_to_dev(hw), e);
+		*entry = exist;
+	}

[Severity: High]
Can overwriting exist->id orphan the rule that previously owned that id?
Two ethtool rules at different locations can end up here with
byte-identical keys:

ice_acl_is_dup_fltr() -> ice_acl_comp_rules() compares raw values and
masks, so rules that differ only in masked-out bits are accepted as
distinct:

		       a->ip.v4.dst_port == b->ip.v4.dst_port &&
		       a->ip.v4.src_port == b->ip.v4.src_port &&

but ice_gen_key_word() emits the fixed don't-care key/invert pair for
every masked bit, ignoring the value, so both rules produce identical
e->entry buffers and ice_flow_acl_find_scen_entry_cond()'s
memcmp(p->entry, e->entry, p->entry_sz) matches.  ethtool always passes
ICE_FLOW_PRIO_NORMAL, so do_rem_entry stays false, do_add_entry becomes
false, and this merge branch runs.

After exist->id = e->id, no flow entry carries the first rule's id any
more, although its ice_ntuple_fltr is still in hw->fdir_list_head and is
still reported by ethtool -n.  Deleting the second rule invalidates the
single shared TCAM entry, so does the first rule remain listed but no
longer enforced?  And does deleting the first rule then fail forever,
since ice_del_acl_ethtool() gets handle 0 and ice_flow_rem_entry()
returns -EINVAL before ice_ntuple_update_list_entry() reaches
list_del()/kfree(), leaving hw->acl_fltr_cnt[] and
hw->ntuple_active_fltr_cnt inflated?

+out:
+	kfree(acts);
+
+	return err;
+}

[ ... ]

[Severity: Medium]
Is ICE_FLAG_FD_ENA the right condition for this?  The only call site is
still:

	if (test_bit(ICE_FLAG_FD_ENA, pf->flags))
		ice_set_fd_vsi_ctx(ctxt, vsi);

so the ACL section is now advertised for every PF/CTRL/VF/CHNL VSI
whenever Flow Director is on, with no check that ACL was actually
initialized.  ice_init_features() treats ACL init failure as non-fatal:

	/* Note: ACL init failure is non-fatal to load */
	if (ice_init_acl(pf))
		dev_err(dev, "Failed to initialize ACL\n");

so hw->acl_tbl can be NULL while the VSI context claims the ACL section.
Conversely, for a configuration where ACL is available but the FD flag is
clear, the ACL VSI configuration is never applied even though
ice_add_ntuple_ethtool() routes rules to ACL based only on
pf->hw.acl_tbl.

 	dflt_q = 0;
 	dflt_q_group = 0;

[ ... ]

Where do the miss-counter indices 2 and 3 come from?  They are not
allocated anywhere in the driver and there is no comment explaining the
choice.

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