diff mbox series

[net,2/4] ice: Stop processing VF messages during teardown

Message ID 20210809171402.17838-3-anthony.l.nguyen@intel.com
State New
Headers show
Series Intel Wired LAN Driver Updates 2021-08-09 | expand

Commit Message

Tony Nguyen Aug. 9, 2021, 5:14 p.m. UTC 
From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>

When VFs are setup and torn down in quick succession, it is possible
that a VF is torn down by the PF while the VF's virtchnl requests are
still in the PF's mailbox ring. Processing the VF's virtchnl request
when the VF itself doesn't exist results in undefined behavior. Fix
this by adding a check to stop processing virtchnl requests when VF
teardown is in progress.

Fixes: ddf30f7ff840 ("ice: Add handler to configure SR-IOV")
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h             | 1 +
 drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c | 7 +++++++
 2 files changed, 8 insertions(+)

Comments

Anirudh Venkataramanan Aug. 10, 2021, 5:42 p.m. UTC | #1 
On Mon, 2021-08-09 at 15:58 -0700, Jakub Kicinski wrote:
> On Mon,  9 Aug 2021 10:14:00 -0700 Tony Nguyen wrote:

> > When VFs are setup and torn down in quick succession, it is

> > possible

> > that a VF is torn down by the PF while the VF's virtchnl requests

> > are

> > still in the PF's mailbox ring. Processing the VF's virtchnl

> > request

> > when the VF itself doesn't exist results in undefined behavior. Fix

> > this by adding a check to stop processing virtchnl requests when VF

> > teardown is in progress.

> 

> What is "undefined behavior" in this context? Please improve the

> commit

> message. It should describe misbehavior visible to the user, failing

> that what will happen from kernel/device perspective. Or state that

> it's

> just a "fix" to align with some internal driver <> firmware spec...


Three different call traces were reported, and that's the reason I
chose to say "undefined behavior" which I suppose isn't very helpful.
Will update the commit message to include more details.

Ani
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index a450343fbb92..eadcb9958346 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -234,6 +234,7 @@  enum ice_pf_state {
 	ICE_VFLR_EVENT_PENDING,
 	ICE_FLTR_OVERFLOW_PROMISC,
 	ICE_VF_DIS,
+	ICE_VF_DEINIT_IN_PROGRESS,
 	ICE_CFG_BUSY,
 	ICE_SERVICE_SCHED,
 	ICE_SERVICE_DIS,
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
index 2826570dab51..e93430ab37f1 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
@@ -615,6 +615,8 @@  void ice_free_vfs(struct ice_pf *pf)
 	struct ice_hw *hw = &pf->hw;
 	unsigned int tmp, i;
 
+	set_bit(ICE_VF_DEINIT_IN_PROGRESS, pf->state);
+
 	if (!pf->vf)
 		return;
 
@@ -680,6 +682,7 @@  void ice_free_vfs(struct ice_pf *pf)
 				i);
 
 	clear_bit(ICE_VF_DIS, pf->state);
+	clear_bit(ICE_VF_DEINIT_IN_PROGRESS, pf->state);
 	clear_bit(ICE_FLAG_SRIOV_ENA, pf->flags);
 }
 
@@ -4415,6 +4418,10 @@  void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event)
 	struct device *dev;
 	int err = 0;
 
+	/* if de-init is underway, don't process messages from VF */
+	if (test_bit(ICE_VF_DEINIT_IN_PROGRESS, pf->state))
+		return;
+
 	dev = ice_pf_to_dev(pf);
 	if (ice_validate_vf_id(pf, vf_id)) {
 		err = -EINVAL;