Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[netdev-bot+sinfo]

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.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Linkui Xiao]

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, netdev-bot+sinfo@kernel.org 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.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Tony Nguyen]

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, netdev-bot+sinfo@kernel.org 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?

Thanks,
Tony

>> The evaluation is done by an LLM so it may be wrong, if you think
>> that is the case please reply and explain.
> 

Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Przemek Kitszel]

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, netdev-bot+sinfo@kernel.org 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().

Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Linkui Xiao]

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, netdev-bot+sinfo@kernel.org 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/20261008125754.3520773-2-xiaolinkui@126.com/

Re: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Tomasz Lichwala]

On 28.09.2026 08:53, Linkui Xiao wrote:
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice_start_vfs() attaches every VF it brings up to the eswitch with
> ice_eswitch_attach_vf(), but the teardown path only undoes the queue
> mappings and the VF VSI. Nothing calls ice_eswitch_detach_vf() for the
> VFs that were attached before the failure, and the caller, ice_ena_vfs(),
> goes straight to ice_free_vf_entries(), which drops its reference to
> every VF.
> 
> The port representors created for those VFs therefore outlive the
> failed VF creation:
> 
>   - the representor netdev stays registered and its devlink port stays
>     registered too. That port is embedded in struct ice_vf, so it ends
>     up pointing into the memory that ice_sriov_free_vf() releases;
> 
>   - repr->vf keeps pointing at the freed struct ice_vf, and repr->src_vsi
>     at the VF VSI that ice_vf_vsi_release() tore down. The leftover
>     netdev is still visible to the user, so even a plain
>     "ip -s link show" of it reaches ice_repr_get_stats64(), which calls
>     repr->ops.ready() -> ice_check_vf_ready_for_cfg(repr->vf) and then
>     reads repr->src_vsi through ice_update_eth_stats();
> 
>   - the virtchnl ops of that VF, which ice_repr_add_vf() replaced with
>     ice_virtchnl_set_repr_ops(), are never handed back to
>     ice_virtchnl_set_dflt_ops();
> 
>   - pf->eswitch.reprs never becomes empty, so ice_eswitch_detach() never
>     calls ice_eswitch_disable_switchdev(). pf->eswitch.is_running stays
>     true, with the bridge offloads and the devlink rate topology still
>     up, and ice_eswitch_release_env() is skipped, so the uplink VSI is
>     left in the switchdev configuration that ice_eswitch_setup_env()
>     gave it.
> 
> Detach the representor in the teardown loop the way ice_free_vfs() does,
> ahead of ice_vf_vsi_release(), because ice_repr_rem_vf() and
> ice_eswitch_release_repr() both need repr->src_vsi to still be valid.
> Every VF the teardown loop walks completed ice_eswitch_attach_vf()
> successfully, and ice_eswitch_detach_vf() already returns early for a VF
> without a representor, so no extra condition is needed.
> 
> Hold vf->cfg_lock across the teardown of each VF as well, like
> ice_free_vfs() does. The VFs unwound here are the ones that already
> reached set_bit(ICE_VF_STATE_INIT), which is exactly what
> ice_check_vf_ready_for_cfg() checks, so a host administrator can still
> run "ip link set dev <pf> vf N ..." and a VF can still send a mailbox
> message while the loop walks them. Both paths take cfg_lock and then run
> ice_reset_vf(), which gets to ice_eswitch_update_repr() and writes
> through the representor that is being freed, or reach
> ice_vc_process_vf_msg() reading vf->virtchnl_ops while
> ice_virtchnl_set_dflt_ops() hands them back.
> 
> Fixes: fff292b47ac1 ("ice: add VF representors one by one")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> v1:
> - Link: https://lore.kernel.org/netdev/20260921031616.3390259-1-xiaolinkui@126.com/
> 
> Changes in v2:
> - Hold vf->cfg_lock across the teardown of each VF, the way ice_free_vfs()
>   does, so that a concurrent VF reconfiguration cannot walk through the
>   representor and the virtchnl ops that are being torn down.
>   (Sashiko AI review)
> - Patch 2/2 is new and returns the VF MSI-X window that the same failure path
>   reserves. That is a separate, pre-existing bug, so it is not folded in here.
>   (Sashiko AI review)
> - Not carrying over the Reviewed-by from Aleksandr Loktionov, as the code
>   changed after his review.
> 
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de0215596..95abc6704820 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -508,8 +508,13 @@ static int ice_start_vfs(struct ice_pf *pf)
>  		if (it_cnt == 0)
>  			break;
>  
> +		mutex_lock(&vf->cfg_lock);
> +
> +		ice_eswitch_detach_vf(pf, vf);
>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
> +		mutex_unlock(&vf->cfg_lock);
> +
>  		it_cnt--;
>  	}
>  


Reviewed-by: Tomasz Lichwala <tomasz.lichwala@linux.intel.com>

RE: [PATCH iwl-net v2 1/2] ice: detach the VF representor when ice_start_vfs() fails

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@126.com>
> Sent: Monday, September 28, 2026 8:53 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Linkui Xiao <xiaolinkui@kylinos.cn>;
> stable@vger.kernel.org
> Subject: [PATCH iwl-net v2 1/2] ice: detach the VF representor when
> ice_start_vfs() fails
> 
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice_start_vfs() attaches every VF it brings up to the eswitch with
> ice_eswitch_attach_vf(), but the teardown path only undoes the queue
> mappings and the VF VSI. Nothing calls ice_eswitch_detach_vf() for the
> VFs that were attached before the failure, and the caller,
> ice_ena_vfs(), goes straight to ice_free_vf_entries(), which drops its
> reference to every VF.
> 
> The port representors created for those VFs therefore outlive the
> failed VF creation:
> 
>   - the representor netdev stays registered and its devlink port stays
>     registered too. That port is embedded in struct ice_vf, so it ends
>     up pointing into the memory that ice_sriov_free_vf() releases;
> 
>   - repr->vf keeps pointing at the freed struct ice_vf, and repr-
> >src_vsi
>     at the VF VSI that ice_vf_vsi_release() tore down. The leftover
>     netdev is still visible to the user, so even a plain
>     "ip -s link show" of it reaches ice_repr_get_stats64(), which
> calls
>     repr->ops.ready() -> ice_check_vf_ready_for_cfg(repr->vf) and then
>     reads repr->src_vsi through ice_update_eth_stats();
> 
>   - the virtchnl ops of that VF, which ice_repr_add_vf() replaced with
>     ice_virtchnl_set_repr_ops(), are never handed back to
>     ice_virtchnl_set_dflt_ops();
> 
>   - pf->eswitch.reprs never becomes empty, so ice_eswitch_detach()
> never
>     calls ice_eswitch_disable_switchdev(). pf->eswitch.is_running
> stays
>     true, with the bridge offloads and the devlink rate topology still
>     up, and ice_eswitch_release_env() is skipped, so the uplink VSI is
>     left in the switchdev configuration that ice_eswitch_setup_env()
>     gave it.
> 
> Detach the representor in the teardown loop the way ice_free_vfs()
> does, ahead of ice_vf_vsi_release(), because ice_repr_rem_vf() and
> ice_eswitch_release_repr() both need repr->src_vsi to still be valid.
> Every VF the teardown loop walks completed ice_eswitch_attach_vf()
> successfully, and ice_eswitch_detach_vf() already returns early for a
> VF without a representor, so no extra condition is needed.
> 
> Hold vf->cfg_lock across the teardown of each VF as well, like
> ice_free_vfs() does. The VFs unwound here are the ones that already
> reached set_bit(ICE_VF_STATE_INIT), which is exactly what
> ice_check_vf_ready_for_cfg() checks, so a host administrator can still
> run "ip link set dev <pf> vf N ..." and a VF can still send a mailbox
> message while the loop walks them. Both paths take cfg_lock and then
> run ice_reset_vf(), which gets to ice_eswitch_update_repr() and writes
> through the representor that is being freed, or reach
> ice_vc_process_vf_msg() reading vf->virtchnl_ops while
> ice_virtchnl_set_dflt_ops() hands them back.
> 
> Fixes: fff292b47ac1 ("ice: add VF representors one by one")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> v1:
> - Link: https://lore.kernel.org/netdev/20260921031616.3390259-1-
> xiaolinkui@126.com/
> 
> Changes in v2:
> - Hold vf->cfg_lock across the teardown of each VF, the way
> ice_free_vfs()
>   does, so that a concurrent VF reconfiguration cannot walk through
> the
>   representor and the virtchnl ops that are being torn down.
>   (Sashiko AI review)
> - Patch 2/2 is new and returns the VF MSI-X window that the same
> failure path
>   reserves. That is a separate, pre-existing bug, so it is not folded
> in here.
>   (Sashiko AI review)
> - Not carrying over the Reviewed-by from Aleksandr Loktionov, as the
> code
>   changed after his review.
> 
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de0215596..95abc6704820 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -508,8 +508,13 @@ static int ice_start_vfs(struct ice_pf *pf)
>  		if (it_cnt == 0)
>  			break;
> 
> +		mutex_lock(&vf->cfg_lock);
> +
> +		ice_eswitch_detach_vf(pf, vf);
>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
> +		mutex_unlock(&vf->cfg_lock);
> +
>  		it_cnt--;
>  	}
> 
> --
> 2.25.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [PATCH iwl-net v2 2/2] ice: release the VF MSI-X window when ice_start_vfs() fails

[Tomasz Lichwala]

On 28.09.2026 08:53, Linkui Xiao wrote:

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 95abc6704820..df84dbe12cba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -446,8 +446,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>  		return -ENOMEM;
>  
>  	vsi = ice_vf_vsi_setup(vf);
> -	if (!vsi)
> -		return -ENOMEM;
> +	if (!vsi) {
> +		err = -ENOMEM;
> +		goto free_irqs;
> +	}
>  
>  	err = ice_vf_init_host_cfg(vf, vsi);
>  	if (err)
> @@ -457,6 +459,9 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>  
>  release_vsi:
>  	ice_vf_vsi_release(vf);
> +free_irqs:
> +	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
> +
>  	return err;
>  }
>  
> @@ -490,6 +495,8 @@ static int ice_start_vfs(struct ice_pf *pf)
>  				dev_err(ice_pf_to_dev(pf), "Failed to attach VF %d to eswitch, error %d",
>  					vf->vf_id, retval);
>  				ice_vf_vsi_release(vf);
> +				ice_virt_free_irqs(pf, vf->first_vector_idx,
> +						   vf->num_msix);

Nit: For consistency with the teardown: loop (which frees IRQs before releasing the VSI), consider swapping the order here too - currently this branch releases the VSI first, then frees IRQs, the opposite order. Not functionally significant, just readability.

>  				goto teardown;
>  			}
>  		}

Reviewed-by: Tomasz Lichwala <tomasz.lichwala@linux.intel.com>

Re: [PATCH iwl-net v2 2/2] ice: release the VF MSI-X window when ice_start_vfs() fails

[Linkui Xiao]

On 2026/9/28 21:08, Tomasz Lichwala wrote:
> 
> 
> On 28.09.2026 08:53, Linkui Xiao wrote:
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> index 95abc6704820..df84dbe12cba 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> @@ -446,8 +446,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>>   		return -ENOMEM;
>>   
>>   	vsi = ice_vf_vsi_setup(vf);
>> -	if (!vsi)
>> -		return -ENOMEM;
>> +	if (!vsi) {
>> +		err = -ENOMEM;
>> +		goto free_irqs;
>> +	}
>>   
>>   	err = ice_vf_init_host_cfg(vf, vsi);
>>   	if (err)
>> @@ -457,6 +459,9 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>>   
>>   release_vsi:
>>   	ice_vf_vsi_release(vf);
>> +free_irqs:
>> +	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
>> +
>>   	return err;
>>   }
>>   
>> @@ -490,6 +495,8 @@ static int ice_start_vfs(struct ice_pf *pf)
>>   				dev_err(ice_pf_to_dev(pf), "Failed to attach VF %d to eswitch, error %d",
>>   					vf->vf_id, retval);
>>   				ice_vf_vsi_release(vf);
>> +				ice_virt_free_irqs(pf, vf->first_vector_idx,
>> +						   vf->num_msix);
> 
> Nit: For consistency with the teardown: loop (which frees IRQs before releasing the VSI), consider swapping the order here too - currently this branch releases the VSI first, then frees IRQs, the opposite order. Not functionally significant, just readability.

Thanks for the review, Tomasz.

Agreed this is only a readability nit. The two calls are independent and
the current order is functionally safe, so I'll keep v2 as is.

No v3 planned for this change.

Thanks,
Linkui
> 
>>   				goto teardown;
>>   			}
>>   		}
> 
> Reviewed-by: Tomasz Lichwala <tomasz.lichwala@linux.intel.com>
> 

RE: [PATCH iwl-net v2 2/2] ice: release the VF MSI-X window when ice_start_vfs() fails

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@126.com>
> Sent: Monday, September 28, 2026 8:53 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Linkui Xiao <xiaolinkui@kylinos.cn>;
> stable@vger.kernel.org
> Subject: [PATCH iwl-net v2 2/2] ice: release the VF MSI-X window when
> ice_start_vfs() fails
> 
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice_init_vf_vsi_res() reserves the VF MSI-X window with
> ice_virt_get_irqs(), which does bitmap_set() on the PF-wide
> pf->virt_irq_tracker.bm, but nothing hands that window back when VF
> creation fails. ice_virt_free_irqs() is not called anywhere on this
> failure path: not from the release_vsi label of ice_init_vf_vsi_res(),
> not from the ice_eswitch_attach_vf() failure branch, and not from the
> teardown loop of ice_start_vfs(), which only undoes the queue mappings
> and the VF VSI. The caller does not help either, because
> err_unroll_vf_entries -> ice_free_vf_entries() -> ice_put_vf() ->
> ice_sriov_free_vf() only does mutex_destroy() and kfree_rcu().
> 
> The tracker is allocated once per PF in ice_init_virt_irq_tracker()
> and freed only in ice_deinit_virt_irq_tracker(), and
> ice_set_per_vf_res() sizes the VFs from pf-
> >virt_irq_tracker.num_entries rather than from the number of free
> bits. So each failed "echo N > sriov_numvfs" leaks the window reserved
> for every VF that got as far as ice_init_vf_vsi_res(), and once the
> tracker is exhausted ice_virt_get_irqs() keeps returning -ENOENT,
> which means SR-IOV cannot be enabled again without reloading the
> driver. The hardware and the software bookkeeping also end up
> disagreeing, because ice_dis_vf_mappings() clears
> VPINT_ALLOC/VPINT_ALLOC_PCI and re-points GLINT_VECT2FUNC of exactly
> those vectors back at the PF.
> 
> Return the window on the failure paths, in the order ice_free_vfs()
> uses: ice_virt_free_irqs() after ice_eswitch_detach_vf() in the
> teardown loop, and right after the VF VSI is released in the two
> branches that fail before the loop can reach that VF.
> 
> The missing release is older than this change: the teardown loop has
> never returned the window. It turns into an accumulating leak because
> the tracker is now PF-wide and outlives a single SR-IOV enable.
> 
> Fixes: a203163274a4 ("ice: simplify VF MSI-X managing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> Changes in v2:
> - New patch. Returns the VF MSI-X window that ice_init_vf_vsi_res()
> reserves
>   when ice_start_vfs() fails. The missing release is older than the
>   representor bug that patch 1/2 fixes, so it stays a separate patch.
>   (Sashiko AI review)
> 
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 95abc6704820..df84dbe12cba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -446,8 +446,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>  		return -ENOMEM;
> 
>  	vsi = ice_vf_vsi_setup(vf);
> -	if (!vsi)
> -		return -ENOMEM;
> +	if (!vsi) {
> +		err = -ENOMEM;
> +		goto free_irqs;
> +	}
> 
>  	err = ice_vf_init_host_cfg(vf, vsi);
>  	if (err)
> @@ -457,6 +459,9 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
> 
>  release_vsi:
>  	ice_vf_vsi_release(vf);
> +free_irqs:
> +	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
> +
>  	return err;
>  }
> 
> @@ -490,6 +495,8 @@ static int ice_start_vfs(struct ice_pf *pf)
>  				dev_err(ice_pf_to_dev(pf), "Failed to
> attach VF %d to eswitch, error %d",
>  					vf->vf_id, retval);
>  				ice_vf_vsi_release(vf);
> +				ice_virt_free_irqs(pf, vf-
> >first_vector_idx,
> +						   vf->num_msix);
>  				goto teardown;
>  			}
>  		}
> @@ -511,6 +518,7 @@ static int ice_start_vfs(struct ice_pf *pf)
>  		mutex_lock(&vf->cfg_lock);
> 
>  		ice_eswitch_detach_vf(pf, vf);
> +		ice_virt_free_irqs(pf, vf->first_vector_idx, vf-
> >num_msix);
>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
>  		mutex_unlock(&vf->cfg_lock);
> --
> 2.25.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>