Re: [PATCH net-next v2 05/12] ice: create flow profile

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

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

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] The new ethtool ACL path publishes and consumes the shared ACL
  state hw->acl_prof[] and hw->acl_tbl with no lock, while the teardown…
- [High] (Validated as NOT a defect in the final series state.) Concern:
  the ACL profile bookkeeping (hw->acl_prof[fltr_type]->seg/->prof_id)…
- [Medium] ice_flow_acl_def_entry_frmt()
  (drivers/net/ethernet/intel/ice/ice_flow.c:1407) copies the 64-bit…
- [Medium] (Validated as NOT a defect in the final series state.)
  Concern: at this commit ice_add_ntuple_ethtool() diverts partial-mask…
- [Low] The comment added immediately above the ice_flow_add_prof() call
  in ice_acl_prof_add_ethtool()…
- [Low] The kernel-doc added for ice_flow_xtract_pkt_flags()
  (drivers/net/ethernet/intel/ice/ice_flow.c:993-1003) misdescribes both…

--

ice: create flow profile

This adds an ACL path to the ethtool ntuple code: partial-mask rules are
diverted to a new ice_acl_main.c which builds an ice_flow_seg_info from the
ethtool mask and creates an ICE_BLK_ACL flow profile.  Supporting bits are
added to ice_flow.c (packet-flags extraction, ACL entry format, scenario
selection) and hw->acl_prof is allocated in ice_init_acl().

A few questions below.

[ ... ]

+static int ice_acl_prof_add_ethtool(struct ice_pf *pf,
+				    struct ethtool_rx_flow_spec *fsp)
+{
+	struct ice_flow_prof *prof = NULL;
+	struct ice_acl_hw_prof *hw_prof;
+	struct ice_flow_seg_info *seg;
+	enum ice_fltr_ptype fltr_type;
+	struct ice_hw *hw = &pf->hw;
+	int err;
+
+	seg = kzalloc_obj(*seg);
+	if (!seg)
+		return -ENOMEM;

[ ... ]

+	fltr_type = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT);
+
+	hw_prof = hw->acl_prof[fltr_type];
+	if (!hw_prof) {
+		hw_prof = kzalloc_obj(**hw->acl_prof);
+		if (!hw_prof) {
+			err = -ENOMEM;
+			goto free_seg;
+		}
+	}
+
+	if (hw_prof->seg) {
+		/* This flow_type already has an input set.
+		 * If it matches the requested input set then we are
+		 * done. If it's different then it's an error.
+		 */
+		if (!memcmp(hw_prof->seg, seg, sizeof(*seg))) {
+			kfree(seg);
+			return 0;
+		}

[Severity: High]
Does this short-circuit survive a PF reset?  The bookkeeping in
hw->acl_prof[fltr_type] (seg and prof_id) is not invalidated on reset, but
ice_prepare_for_reset() drops the hardware state for this block:

ice_prepare_for_reset()
    ice_clear_hw_tbls(hw);      /* clears ICE_BLK_ACL profile map, ES and
                                   profile TCAM shadow tables */

After that, a second identical request would match here and return 0 while
no ACL profile exists in hardware, and a later ice_acl_rem_flows() would
call ice_flow_rem_prof() with a stale profile ID.

For the record, this appears to be addressed later in the same series by
"ice: add ACL reset recovery and NTUPLE feature toggle", which makes
ice_rebuild() do:

	ice_acl_destroy_tbl(hw);
	if (!ice_acl_create_hw(pf)) {
		ice_acl_replay_flows(hw);
		ice_acl_replay_fltrs(pf);
	}

so the concern is limited to the intermediate state of the series.

+
+		err = -EINVAL;
+		goto free_seg;
+	}
+
+	/* Adding a profile for the given flow specification with no
+	 * actions (NULL) and zero actions 0.
+	 */

[Severity: Low]
This isn't a bug, but does this comment still match the callee?
ice_flow_add_prof() takes no action arguments:

int ice_flow_add_prof(struct ice_hw *hw, enum ice_block blk,
		      enum ice_flow_dir dir, struct ice_flow_seg_info *segs,
		      u8 segs_cnt, bool symm, struct ice_flow_prof **prof);

The call below passes segs_cnt = 1 and symm = false, so the trailing false
is the symmetric-hash flag rather than an action count.  The text looks
carried over from the older ice_fdir_set_hw_fltr_rule() comment.

+	err = ice_flow_add_prof(hw, ICE_BLK_ACL, ICE_FLOW_RX, seg, 1, false,
+				&prof);
+	if (err)
+		goto free_acl_prof;
+
+	hw_prof->seg = seg;
+	hw_prof->prof_id = prof->id;
+	hw->acl_prof[fltr_type] = hw_prof;
+	return 0;

[Severity: High]
What serializes this read-check-allocate-publish of hw->acl_prof[] against
ACL teardown?  ice_add_ntuple_ethtool() returns into
ice_acl_add_rule_ethtool() before mutex_lock(&hw->fdir_fltr_lock) is ever
taken, and ice_deinit_acl() frees the same state without any lock:

ice_remove()
    devl_lock()
    ice_unload()
        ice_deinit_features()
            ice_deinit_acl()
                ice_acl_rem_flows(hw);     /* kfree(prof->seg) */
                ice_acl_destroy_tbl(hw);   /* kfree(hw->acl_tbl) */
                kfree(hw_prof->seg); kfree(hw_prof);
                devm_kfree(dev, hw->acl_prof);
        ice_unregister_netdev(vsi);         /* only afterwards */

The ethtool path holds rtnl, ice_unload() holds devl_lock, so there is no
common lock and the netdev is still registered while the free happens.  Can
an ethtool -N with a partial mask racing device removal or a devlink reload
store hw_prof into the already freed hw->acl_prof array, or walk a freed
hw->acl_tbl->scens list in ice_flow_sel_acl_scen()?

The equivalent FDIR state is protected by hw->fdir_fltr_lock (taken by
ice_vsi_manage_fdir() and ice_vsi_manage_acl()); should the ACL profile
path and ice_deinit_acl() take that mutex too?

+
+free_acl_prof:
+	kfree(hw_prof);
+free_seg:
+	kfree(seg);
+
+	return err;
+}

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
At this commit, does this report success for a rule that does not filter
anything?  ice_acl_add_rule_ethtool() only reaches
ice_acl_prof_add_ethtool(), which creates the flow profile and returns 0
with no flow entry, no action, no VSI association and no insertion into
hw->fdir_list_head.

So ETHTOOL_SRXCLSRLINS would succeed while the rule is invisible to
ETHTOOL_GRXCLSRULE and ETHTOOL_GRXCLSRLALL and cannot be deleted, even
though the delete gate in ice_del_ntuple_ethtool() is relaxed for acl_tbl
devices in the same patch.

This is completed later in the series ("ice: program ACL entry"), where
ice_acl_add_rule_ethtool() builds the ice_ntuple_fltr, sets the action,
programs the TCAM entry with ice_flow_add_entry() and links the filter into
hw->fdir_list_head under hw->fdir_fltr_lock, so the gap is confined to the
intermediate patches.  Would it be worth folding the dispatch into the
patch that actually programs the entry?

+
+	/* Only fdir filters below */
+	if (!test_bit(ICE_FLAG_FD_ENA, pf->flags))
+		return -EOPNOTSUPP;
+
+	ret = ice_cfg_fdir_xtrct_seq(pf, fsp, &userdata);
+	if (ret)
+		return ret;
+

[ ... ]

[Severity: Low]
This isn't a bug, but does the kernel-doc match the code?  The parameter type
is enum ice_flex_mdid_pkt_flags, whose members select successive 16-bit
packet-flag ranges rather than carrying a value:

enum ice_flex_mdid_pkt_flags {
	ICE_RX_MDID_PKT_FLAGS_15_0	= 20,
	ICE_RX_MDID_PKT_FLAGS_31_16,
	ICE_RX_MDID_PKT_FLAGS_47_32,
	ICE_RX_MDID_PKT_FLAGS_63_48,
};

And the body fills exactly one field-vector word, not a DWORD chunk:

	params->es[idx].prot_id = ICE_PROT_META_ID;
	params->es[idx].off = flags;
	params->es_cnt++;

+ *
+ * Return: 0 on success, negative on error
+ */
+static int ice_flow_xtract_pkt_flags(struct ice_hw *hw,
+				     struct ice_flow_prof_params *params,
+				     enum ice_flex_mdid_pkt_flags flags)
+{

[ ... ]

[ ... ]

+static int
+ice_flow_acl_def_entry_frmt(struct ice_flow_prof_params *params)
+{
+	u16 index, range_idx = 0;
+
+	index = ICE_AQC_ACL_PROF_BYTE_SEL_START_IDX;
+
+	for (int i = 0; i < params->prof->segs_cnt; i++) {
+		struct ice_flow_seg_info *seg = &params->prof->segs[i];
+		unsigned long match = seg->match;
+		int j;
+
+		for_each_set_bit(j, &match, ICE_FLOW_FIELD_IDX_MAX) {

[Severity: Medium]
Can this read past the match local on 32-bit builds?  seg->match is declared
u64 in struct ice_flow_seg_info, and ICE_FLOW_FIELD_IDX_MAX is 58, so with
BITS_PER_LONG == 32 the copy into unsigned long drops match bits 32 through
57, and find_next_bit() is told the bitmap is 58 bits wide while only 4
bytes of storage exist, so it dereferences (&match)[1].

The sibling loop in ice_flow_create_xtrct_seq() keeps the u64 and casts the
address instead:

		u64 match = params->prof->segs[i].match;
		...
		for_each_set_bit(j, (unsigned long *)&match,
				 ICE_FLOW_FIELD_IDX_MAX) {

Besides the out-of-bounds read, would stale stack bits picked up above bit 31
be treated as real match fields here, inflating index and range_idx and so
producing a spurious -EINVAL or a wrong params->entry_length that
ice_flow_sel_acl_scen() then uses for scenario selection?  ICE has no 64BIT
dependency in drivers/net/ethernet/intel/Kconfig, so i386 builds reach this.

+			struct ice_flow_fld_info *fld = &seg->fields[j];
+
+			fld->entry.mask = ICE_FLOW_FLD_OFF_INVAL;
+

[ ... ]

+	/* Store # bytes required for entry for later use */
+	params->entry_length = index - ICE_AQC_ACL_PROF_BYTE_SEL_START_IDX;
+
+	return 0;
+}
+

[ ... ]

This is the teardown referenced in the locking question on
ice_acl_prof_add_ethtool() above.

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