On 2026/10/7 19:11, Przemek Kitszel wrote:
On 9/29/26 8:11 PM, Tony Nguyen wrote:
On 9/28/2026 12:14 AM, Linkui Xiao wrote:
Hi,
Thanks for the review. Please find the missing information below.
- How the issue was discovered:
Found during manual code inspection of the ice VF setup/teardown
error paths. It was not reported by syzbot, a static analysis tool,
or an LLM scan, and was not hit in production.
- Whether the issue was actually triggered:
Not actually triggered. It is a theoretical error-path cleanup
issue found by inspection; no stack trace or error message was observed.
- Hardware tested:
Not tested on real hardware. The change was only compile-tested;
no affected Intel NIC/firmware test was performed.
The same applies to patch 2/2: it was also found by the same manual
code inspection, was not triggered at runtime, and was only compile-
tested, not tested on real hardware.
If a v3 is needed for other review reasons, I will include this
information in the commit messages.
Thanks,
Linkui Xiao
On 2026/9/28 14:59, [email protected] wrote:
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- How the issue was discovered, e.g. hit in production, hit during
development, syzbot report, manual code inspection, LLM or static
analysis tool scan.
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
- What hardware the change was tested on. For driver fixes please
mention the device (and if relevant firmware version) used for
testing, or say that the change was not tested on real hardware.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
Hi Jakub,
For patches that go through iwl-net, how did you want this handled? It
says to not repost just to address the above, but since this will get
submitted later, did you want the commit updated to make the latter
check happy, did you want me to copy/paste the response to the commit,
or something else?
In general, I would say it's up to you Tony.
Perhaps we could decide based on the amount of additional changes.
For this particular patch sashiko already posted complains:
https://sashiko.dev/#/patchset/20260928065306.1514795-1-xiaolinkui%40126.com?part=1
I've encountered the same problem (although during my OOT encounters),
and the solution is to detach/attach VF representors outside of
vf->cfg_lock. More rationale by AI follows:
ice_reset_all_vfs() and ice_free_vfs() call ice_eswitch_detach() and
ice_eswitch_attach() with vf->cfg_lock held. Both take the devlink
instance lock and then register or unregister the representor netdev,
which takes RTNL. ndo_set_vf_mac(), ndo_set_vf_vlan() and the
representor's ethtool reset take vf->cfg_lock under RTNL, so the order
is inverted. The ice_check_vf_ready_for_cfg() check in those callers
runs before vf->cfg_lock is taken, so it doesn't keep them away from a
reset that is already in progress.
Detach before taking vf->cfg_lock and attach after releasing it.
Representors are added and removed only under pf->vfs.table_lock, and
ice_start_vfs() already attaches without vf->cfg_lock. Both functions
set ICE_VF_DIS first, so a concurrent ice_reset_vf() bails out before it
reaches ice_eswitch_update_repr().
Thanks. That is the AB-BA Sashiko flagged, and v3 does exactly that. The
series is three patches now, reposted as a new thread whose cover is
[PATCH iwl-net v3 0/3] ice: fix VF representor lock ordering and teardown
error paths
with the per-patch changes under "Changes in v3:" in each patch:
1/3 ice: attach and detach VF representors outside of vf->cfg_lock (new)
2/3 ice: detach the VF representor when ice_start_vfs() fails (was 1/2)
3/3 ice: free the VF MSI-X vectors when VF start fails (was 2/2)
1/3 is new and is the "same problem" you describe. ice_free_vfs() and
ice_reset_all_vfs() are where the inversion was introduced: fff292b47ac1
turned the single ice_eswitch_release() that ice_free_vfs() made before its
loop into a per-VF detach under cfg_lock, and c9663f79cd82 did the same to
the ice_eswitch_rebuild() call in ice_reset_all_vfs(). 1/3 moves the detach
ahead of cfg_lock in both loops, and in ice_reset_all_vfs() the attach back
after the unlock. It also takes cfg_lock once before the detach, so that a
reset which is already inside the lock is waited out instead of being left
with a window: ICE_VF_DIS, raised before both loops, keeps later resets out,
but not one that got in ahead of it, and that one can reach
ice_eswitch_update_repr() while ice_repr_destroy() is freeing the
representor it looks up, which is free_netdev() + kfree() with no RCU grace
period. 2/3 is the original ice_start_vfs() fix with the same reordering,
plus the disabled marking described below. 3/3 is the MSI-X leak, the same
three calls, with its Fixes: tag corrected. Tony, if you would rather keep
the -net series at two patches, say so and I will drop 1/3.
The edge that had to go is cfg_lock -> RTNL. ice_eswitch_detach_vf() takes
devl_lock() and then RTNL through ice_repr_rem_vf() -> unregister_netdev(),
while __ice_set_vf_mac(), ice_set_vf_port_vlan() and the representor's
ethtool reset take cfg_lock with RTNL already held. The representor itself
does not need cfg_lock there: it is created and destroyed under
pf->vfs.table_lock, and ice_free_vfs(), ice_reset_all_vfs() and
ice_start_vfs() all run with that lock held.
On the ice_start_vfs() path I kept cfg_lock around the rest of the teardown,
because the ICE_VF_DIS half of the rationale does not hold there.
ice_free_vfs() and ice_reset_all_vfs() raise ICE_VF_DIS before they
touch any
VF, the SR-IOV enable path never does (the clear_bit() in ice_ena_vfs() has
no matching set), and the VFs unwound by that loop already reached
ICE_VF_STATE_INIT, so "ip link set dev <pf> vf N ..." still passes
ice_check_vf_ready_for_cfg() and can run ice_reset_vf() against the VSI that
ice_vf_vsi_release() is tearing down.
That is why 2/3 marks the VF disabled under cfg_lock before the detach. As
you said, ice_check_vf_ready_for_cfg() runs before the lock is taken, so it
does not keep a reset that is already in progress away; the check that does
keep one out is the ice_is_vf_disabled() test ice_reset_vf() makes after
taking cfg_lock. Taking cfg_lock once before the detach waits that one out
while its representor is still attached, and the bit keeps later ones from
reaching ice_eswitch_update_repr(). Without it a reset already past its own
check could xa_load() the representor while ice_repr_destroy() frees it, and
that is free_netdev() + kfree(), with no RCU grace period in between.
v3 also carries the origin and testing information netdev-bot asked for, in
the commit messages, since the series is being reposted anyway. 2/3 changed
code, and in 3/3 the call in the teardown loop moved inside cfg_lock, so I
did not carry over the Reviewed-by tags from Tomasz Lichwala and Aleksandr
Loktionov. Tomasz's nit on the eswitch attach failure path is in: the IRQs
are freed before the VSI is released there, and the teardown loop keeps that
order too. The release_vsi label of ice_init_vf_vsi_res() still
releases the
VSI first, because the !vsi path has to skip that call.
The v3 series was posted here:
https://lore.kernel.org/netdev/[email protected]/
