[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-08-11 (ice)

[PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-08-11 (ice)

[Tony Nguyen]

This series contains updates to ice driver only.

Benjamin corrects a misplaced parenthesis for a WARN_ON check.

Michal removes WARN_ON from a check as its recoverable and not
warranting of a call trace.

The following are changes since commit 84ba28901629cd3aa3b24d359bc4da3ac24c2329:
  Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Benjamin Mikailenko (1):
  ice: Fix VSI rebuild WARN_ON check for VF

Michal Jaron (1):
  ice: Fix call trace with null VSI during VF reset

 drivers/net/ethernet/intel/ice/ice_lib.c    | 2 +-
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

-- 
2.35.1

[PATCH net 1/2] ice: Fix VSI rebuild WARN_ON check for VF

[Tony Nguyen]

From: Benjamin Mikailenko <benjamin.mikailenko@intel.com>

In commit b03d519d3460 ("ice: store VF pointer instead of VF ID")
WARN_ON checks were added to validate the vsi->vf pointer and
catch programming errors. However, one check to vsi->vf was missed.
This caused a call trace when resetting VFs.

Fix ice_vsi_rebuild by encompassing VF pointer in WARN_ON check.

Fixes: b03d519d3460 ("ice: store VF pointer instead of VF ID")
Signed-off-by: Benjamin Mikailenko <benjamin.mikailenko@intel.com>
Tested-by: Marek Szlosek <marek.szlosek@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index a830f7f9aed0..0d4dbca88964 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3181,7 +3181,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, bool init_vsi)
 
 	pf = vsi->back;
 	vtype = vsi->type;
-	if (WARN_ON(vtype == ICE_VSI_VF) && !vsi->vf)
+	if (WARN_ON(vtype == ICE_VSI_VF && !vsi->vf))
 		return -EINVAL;
 
 	ice_vsi_init_vlan_ops(vsi);
-- 
2.35.1

[PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

[Tony Nguyen]

From: Michal Jaron <michalx.jaron@intel.com>

During stress test with attaching and detaching VF from KVM and
simultaneously changing VFs spoofcheck and trust there was a
call trace in ice_reset_vf that VF's VSI is null.

[145237.352797] WARNING: CPU: 46 PID: 840629 at drivers/net/ethernet/intel/ice/ice_vf_lib.c:508 ice_reset_vf+0x3d6/0x410 [ice]
[145237.352851] Modules linked in: ice(E) vfio_pci vfio_pci_core vfio_virqfd vfio_iommu_type1 vfio iavf dm_mod xt_CHECKSUM xt_MASQUERADE
xt_conntrack ipt_REJECT nf_reject_ipv4 nft_compat nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 nf_tables nfnetlink tun
 bridge stp llc sunrpc intel_rapl_msr intel_rapl_common sb_edac x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm iTCO_wdt iTC
O_vendor_support irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel rapl ipmi_si intel_cstate ipmi_devintf joydev intel_uncore m
ei_me ipmi_msghandler i2c_i801 pcspkr mei lpc_ich ioatdma i2c_smbus acpi_pad acpi_power_meter ip_tables xfs libcrc32c i2c_algo_bit drm_sh
mem_helper drm_kms_helper sd_mod t10_pi crc64_rocksoft syscopyarea crc64 sysfillrect sg sysimgblt fb_sys_fops drm i40e ixgbe ahci libahci
 libata crc32c_intel mdio dca wmi fuse [last unloaded: ice]
[145237.352917] CPU: 46 PID: 840629 Comm: kworker/46:2 Tainted: G S      W I E     5.19.0-rc6+ #24
[145237.352921] Hardware name: Intel Corporation S2600WTT/S2600WTT, BIOS SE5C610.86B.01.01.0008.021120151325 02/11/2015
[145237.352923] Workqueue: ice ice_service_task [ice]
[145237.352948] RIP: 0010:ice_reset_vf+0x3d6/0x410 [ice]
[145237.352984] Code: 30 ec f3 cc e9 28 fd ff ff 0f b7 4b 50 48 c7 c2 48 19 9c c0 4c 89 ee 48 c7 c7 30 fe 9e c0 e8 d1 21 9d cc 31 c0 e9 a
9 fe ff ff <0f> 0b b8 ea ff ff ff e9 c1 fc ff ff 0f 0b b8 fb ff ff ff e9 91 fe
[145237.352987] RSP: 0018:ffffb453e257fdb8 EFLAGS: 00010246
[145237.352990] RAX: ffff8bd0040181c0 RBX: ffff8be68db8f800 RCX: 0000000000000000
[145237.352991] RDX: 000000000000ffff RSI: 0000000000000000 RDI: ffff8be68db8f800
[145237.352993] RBP: ffff8bd0040181c0 R08: 0000000000001000 R09: ffff8bcfd520e000
[145237.352995] R10: 0000000000000000 R11: 00008417b5ab0bc0 R12: 0000000000000005
[145237.352996] R13: ffff8bcee061c0d0 R14: ffff8bd004019640 R15: 0000000000000000
[145237.352998] FS:  0000000000000000(0000) GS:ffff8be5dfb00000(0000) knlGS:0000000000000000
[145237.353000] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[145237.353002] CR2: 00007fd81f651d68 CR3: 0000001a0fe10001 CR4: 00000000001726e0
[145237.353003] Call Trace:
[145237.353008]  <TASK>
[145237.353011]  ice_process_vflr_event+0x8d/0xb0 [ice]
[145237.353049]  ice_service_task+0x79f/0xef0 [ice]
[145237.353074]  process_one_work+0x1c8/0x390
[145237.353081]  ? process_one_work+0x390/0x390
[145237.353084]  worker_thread+0x30/0x360
[145237.353087]  ? process_one_work+0x390/0x390
[145237.353090]  kthread+0xe8/0x110
[145237.353094]  ? kthread_complete_and_exit+0x20/0x20
[145237.353097]  ret_from_fork+0x22/0x30
[145237.353103]  </TASK>

Remove WARN_ON() from check if VSI is null in ice_reset_vf.
Add "VF is already removed\n" in dev_dbg().

This WARN_ON() is unnecessary and causes call trace, despite that
call trace, driver still works. There is no need for this warn
because this piece of code is responsible for disabling VF's Tx/Rx
queues when VF is disabled, but when VF is already removed there
is no need to do reset or disable queues.

Fixes: efe41860008e ("ice: Fix memory corruption in VF driver")
Signed-off-by: Michal Jaron <michalx.jaron@intel.com>
Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Tested-by: Marek Szlosek <marek.szlosek@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index 8fd7c3e37f5e..76f70fe1d998 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -571,8 +571,10 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
 
 	if (ice_is_vf_disabled(vf)) {
 		vsi = ice_get_vf_vsi(vf);
-		if (WARN_ON(!vsi))
+		if (!vsi) {
+			dev_dbg(dev, "VF is already removed\n");
 			return -EINVAL;
+		}
 		ice_vsi_stop_lan_tx_rings(vsi, ICE_NO_RESET, vf->vf_id);
 		ice_vsi_stop_all_rx_rings(vsi);
 		dev_dbg(dev, "VF is already disabled, there is no need for resetting it, telling VM, all is fine %d\n",
-- 
2.35.1

Re: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

[Jakub Kicinski]

On Thu, 11 Aug 2022 09:17:14 -0700 Tony Nguyen wrote:
> This WARN_ON() is unnecessary and causes call trace, despite that
> call trace, driver still works. There is no need for this warn
> because this piece of code is responsible for disabling VF's Tx/Rx
> queues when VF is disabled, but when VF is already removed there
> is no need to do reset or disable queues.

Can't you flush the service work when disabling VFs instead?
Seems better to try to keep the system in a consistent state
than add "if NULL return;" in random places :S

Re: [PATCH net 0/2][pull request] Intel Wired LAN Driver Updates 2022-08-11 (ice)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by Tony Nguyen <anthony.l.nguyen@intel.com>:

On Thu, 11 Aug 2022 09:17:12 -0700 you wrote:
> This series contains updates to ice driver only.
> 
> Benjamin corrects a misplaced parenthesis for a WARN_ON check.
> 
> Michal removes WARN_ON from a check as its recoverable and not
> warranting of a call trace.
> 
> [...]

Here is the summary with links:
  - [net,1/2] ice: Fix VSI rebuild WARN_ON check for VF
    https://git.kernel.org/netdev/net/c/7fe05e125d5f
  - [net,2/2] ice: Fix call trace with null VSI during VF reset
    https://git.kernel.org/netdev/net/c/cf90b74341ee

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

RE: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

[Sokolowski, Jan]

I'd like to send this response in Michal Jaron's name, as he currently cannot respond to this email.

Generally you are right, it is better idea to try to keep the system in a consistent state than adding "if NULL return;" but I don't think it will work here. This "if NULL return;" is here because of race between two resets and I don't think we can guarantee that this race will be not present if we flush the service work before reset. The problem is that vf reset is called in the same time from vf on vm and from pf. When reset from vf is called and reset form pf don't clear rings yet we must go into ice_reset_vf and clear those rings without triggering second reset. If we don't clear rings there we may trigger page_frag_cache_drain crash related to writing data to unmapped queues. In such cases if there are no vsis we don't need to do this and this WARN_ON is not necessary, but we need to check it anyway.

 
-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: Saturday, August 13, 2022 2:13 AM
To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Jaron, MichalX <michalx.jaron@intel.com>; netdev@vger.kernel.org; Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; Szlosek, Marek <marek.szlosek@intel.com>
Subject: Re: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

On Thu, 11 Aug 2022 09:17:14 -0700 Tony Nguyen wrote:
> This WARN_ON() is unnecessary and causes call trace, despite that
> call trace, driver still works. There is no need for this warn
> because this piece of code is responsible for disabling VF's Tx/Rx
> queues when VF is disabled, but when VF is already removed there
> is no need to do reset or disable queues.

Can't you flush the service work when disabling VFs instead?
Seems better to try to keep the system in a consistent state
than add "if NULL return;" in random places :S

Re: [PATCH net 2/2] ice: Fix call trace with null VSI during VF reset

[Jakub Kicinski]

On Wed, 17 Aug 2022 13:31:00 +0000 Sokolowski, Jan wrote:
> I'd like to send this response in Michal Jaron's name, as he currently cannot respond to this email.
> 
> Generally you are right, it is better idea to try to keep the system
> in a consistent state than adding "if NULL return;" but I don't think
> it will work here. This "if NULL return;" is here because of race
> between two resets and I don't think we can guarantee that this race
> will be not present if we flush the service work before reset. The
> problem is that vf reset is called in the same time from vf on vm and
> from pf. When reset from vf is called and reset form pf don't clear
> rings yet we must go into ice_reset_vf and clear those rings without
> triggering second reset. If we don't clear rings there we may trigger
> page_frag_cache_drain crash related to writing data to unmapped
> queues. In such cases if there are no vsis we don't need to do this
> and this WARN_ON is not necessary, but we need to check it anyway.

Hm, I assumed there's some synchronization here which we can use 
to prevent the race. After all _something_ must ensure the pointer
returned from ice_get_vf_vsi() will not go away, for instance.

But I'm obviously speculating and Dave already applied the patches 
so moving on.. :)