Re: [PATCH net-next v2 07/12] ice: use plain alloc/dealloc for ice_ntuple_fltr

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

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

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).

- [Medium] ice_add_ntuple_ethtool()
  (drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c:2262) now…

Pre-existing issues:
- [High] hw->fdir_list_head is documented as being protected by
  hw->fdir_fltr_lock (see the comment on ice_fdir_del_all_fltrs(),…

--

ice: use plain alloc/dealloc for ice_ntuple_fltr

This converts struct ice_ntuple_fltr from devm_kzalloc()/devm_kfree() to
kzalloc_obj()/kfree(), on the grounds that the filter objects are always
torn down explicitly on device remove.  Four call sites in
ice_ethtool_ntuple.c are updated.

A couple of questions below about the teardown assumption and about the
readers of hw->fdir_list_head.

[Severity: High]
This isn't a bug introduced by this patch, but since this patch is now the
code doing the freeing, is the reader side of hw->fdir_list_head safe
against these kfree() calls?

The comment on ice_fdir_del_all_fltrs() states the list is protected by
hw->fdir_fltr_lock, and the writers do take it (ice_ntuple_update_list_entry()
via ice_add_ntuple_ethtool()/ice_del_ntuple_ethtool(), ice_vsi_manage_fdir(),
ice_remove_q_channels()).  But ice_fdir_replay_fltrs() walks the same list with
no lock at all:

drivers/net/ethernet/intel/ice/ice_ethtool_ntuple.c:ice_fdir_replay_fltrs() {
	list_for_each_entry(f_rule, &hw->fdir_list_head, fltr_node) {
		int err;

		if (f_rule->acl_fltr)
			continue;

		err = ice_fdir_write_all_fltr(pf, f_rule, true);
		...
	}
}

and it runs from the service task, ice_reset_subtask() -> ice_rebuild() ->
ice_fdir_replay_fltrs(), without rtnl_lock and without hw->fdir_fltr_lock.

The reset-in-progress test in ice_del_ntuple_ethtool() and at the top of
ice_ntuple_update_list_entry() is evaluated before the slow part of the
writer: ice_fdir_write_all_fltr() -> ice_fdir_write_fltr() does GFP_KERNEL
allocations and HW programming and can sleep for an unbounded time, and only
after that does the writer run:

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

So can the replay walk end up dereferencing a node that was just kfree()d,
including reading fltr_node.next after list_del() has set it to
LIST_POISON1?

The mirror case exists too: ice_add_ntuple_ethtool() calls
ice_fdir_find_fltr_by_idx(), which walks the list, before
mutex_lock(&hw->fdir_fltr_lock), so it can traverse nodes that
ice_prepare_for_reset() -> ice_remove_q_channels() -> ice_fdir_del_all_fltrs()
is freeing in the service task.

This same shape is still present at the tip of the series, with
ice_fdir_replay_fltrs() still lockless.

[Severity: Medium]
The commit message says:

    All such objects are being removed on device remove via
    ice_deinit_features() -> ice_deinit_fdir() -> ice_vsi_manage_fdir() ->
    ice_fdir_del_all_fltrs()

Is that always true?  ice_deinit_features() bails out first thing in Safe
Mode:

drivers/net/ethernet/intel/ice/ice_main.c:ice_deinit_features() {
	if (ice_is_safe_mode(pf))
		return;

	ice_deinit_lag(pf);
	...
	ice_deinit_fdir(pf);
	...
}

A device that was fully featured when the filters were installed can later
move into Safe Mode at runtime: ice_rebuild() reloads the package after
CORER/GLOBR,

		else
			/* Reload DDP Package after CORER/GLOBR reset */
			ice_load_pkg(NULL, pf);

and when ice_init_pkg() fails, ice_load_pkg() does:

	if (!ice_is_init_pkg_successful(state)) {
		/* Safe Mode */
		clear_bit(ICE_FLAG_ADV_FEATURES, pf->flags);
		return;
	}

with ice_is_safe_mode() defined as !test_bit(ICE_FLAG_ADV_FEATURES,
pf->flags).  ICE_FLAG_FD_ENA is not cleared there, so the entries stay on
hw->fdir_list_head (ice_fdir_replay_fltrs() keeps them, and
ice_fdir_del_all_fltrs() only runs from ice_prepare_for_reset() when ADQ
channels exist).

In that state, does unbind leak every installed struct ice_ntuple_fltr, since
ice_deinit_fdir() -> ice_vsi_manage_fdir() -> ice_fdir_del_all_fltrs() is
never reached and devres no longer reclaims the objects?  The same structure
is present at the tip of the series, so nothing later frees them either.

[ ... ]

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