[Intel-wired-lan] [net PATCH 0/2] ice/i40e: stop disabling VFs due to PF error responses

[Intel-wired-lan] [net PATCH 0/2] ice/i40e: stop disabling VFs due to PF error responses

[Jacob Keller]

The ice and i40e drivers have similar logic to detect PF error responses to
the VFs over the Virtchnl interface. This logic counts the number of
sequential error responses sent to a VF and logs each error response as a
dev_info to the kernel log. If more than 10 error responses are sent in a
row, the PF driver disables the VF in question.

This logic has existed since the early days of i40e VF implementation, and
was carried forward into ice. The logic appears to be some form of malicious
VF detection. However, it doesn't actually fit in with the hardware based
malicious VF detection available in the device hardware.

In fact, the logic has had some adjustments in the i40e driver to reduce the
messaging from dev_err to dev_info, and to allow specific messages to skip
the check.

These changes clearly highlight the problematic nature of the check.
Additionally, there is no requirement for this behavior in our product
design. Other driver implementations such as the FreeBSD drivers do not
implement this behavior.

Disabling VFs is typically not what system administrators what to happen,
and this behavior is non-intuitive and not documented. Since it clearly
causes issues and isn't connected to the proper malicious driver detection,
remove this check from both ice and i40e.

Jacob Keller (2):
  i40e: stop disabling VFs due to PF error responses
  ice: stop disabling VFs due to PF error responses

 .../net/ethernet/intel/i40e/i40e_debugfs.c    |  6 +-
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 57 +++----------------
 .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  5 --
 .../net/ethernet/intel/ice/ice_virtchnl_pf.c  | 18 ------
 .../net/ethernet/intel/ice/ice_virtchnl_pf.h  |  3 -
 5 files changed, 9 insertions(+), 80 deletions(-)


base-commit: 35410c10918a84c9685cd644686ca637bb3c046b
-- 
2.35.1.129.gb80121027d12

[Intel-wired-lan] [net PATCH 1/2] i40e: stop disabling VFs due to PF error responses

[Jacob Keller]

The i40e_vc_send_msg_to_vf_ex (and its wrapper i40e_vc_send_msg_to_vf)
function has logic to detect "failure" responses sent to the VF. If a VF
is sent more than I40E_DEFAULT_NUM_INVALID_MSGS_ALLOWED, then the VF is
marked as disabled. In either case, a dev_info message is printed
stating that a VF opcode failed.

This logic originates from the early implementation of VF support in
commit 5c3c48ac6bf5 ("i40e: implement virtual device interface").

That commit did not go far enough. The "logic" for this behavior seems
to be that error responses somehow indicate a malicious VF. This is not
really true. The PF might be sending an error for any number of reasons
such as lacking resources, an unsupported operation, etc. This does not
indicate a malicious VF. We already have a separate robust malicious VF
detection which relies on hardware logic to detect and prevent a variety
of behaviors.

There is no justification for this behavior in the original
implementation. In fact, a later commit 18b7af57d9c1 ("i40e: Lower some
message levels") reduced the opcode failure message from a dev_err to a
dev_info. In addition, recent commit 01cbf50877e6 ("i40e: Fix to not
show opcode msg on unsuccessful VF MAC change") changed the logic to
allow quieting it for expected failures.

That commit prevented this logic from kicking in for specific
circumstances. This change did not go far enough. The behavior is not
documented nor is it part of any requirement for our products. Other
operating systems such as the FreeBSD implementation of our driver do
not include this logic.

It is clear this check does not make sense, and causes problems which
led to ugly workarounds.

Fix this by just removing the entire logic and the need for the
i40e_vc_send_msg_to_vf_ex function.

Fixes: 01cbf50877e6 ("i40e: Fix to not show opcode msg on unsuccessful VF MAC change")
Fixes: 5c3c48ac6bf5 ("i40e: implement virtual device interface")
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 .../net/ethernet/intel/i40e/i40e_debugfs.c    |  6 +-
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 57 +++----------------
 .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  5 --
 3 files changed, 9 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
index 1e57cc8c47d7..9db5001297c7 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
@@ -742,10 +742,8 @@ static void i40e_dbg_dump_vf(struct i40e_pf *pf, int vf_id)
 		vsi = pf->vsi[vf->lan_vsi_idx];
 		dev_info(&pf->pdev->dev, "vf %2d: VSI id=%d, seid=%d, qps=%d\n",
 			 vf_id, vf->lan_vsi_id, vsi->seid, vf->num_queue_pairs);
-		dev_info(&pf->pdev->dev, "       num MDD=%lld, invalid msg=%lld, valid msg=%lld\n",
-			 vf->num_mdd_events,
-			 vf->num_invalid_msgs,
-			 vf->num_valid_msgs);
+		dev_info(&pf->pdev->dev, "       num MDD=%lld\n",
+			 vf->num_mdd_events);
 	} else {
 		dev_info(&pf->pdev->dev, "invalid VF id %d\n", vf_id);
 	}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index dfdb6e786461..2606e8f0f19b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -1917,19 +1917,17 @@ int i40e_pci_sriov_configure(struct pci_dev *pdev, int num_vfs)
 /***********************virtual channel routines******************/
 
 /**
- * i40e_vc_send_msg_to_vf_ex
+ * i40e_vc_send_msg_to_vf
  * @vf: pointer to the VF info
  * @v_opcode: virtual channel opcode
  * @v_retval: virtual channel return value
  * @msg: pointer to the msg buffer
  * @msglen: msg length
- * @is_quiet: true for not printing unsuccessful return values, false otherwise
  *
  * send msg to VF
  **/
-static int i40e_vc_send_msg_to_vf_ex(struct i40e_vf *vf, u32 v_opcode,
-				     u32 v_retval, u8 *msg, u16 msglen,
-				     bool is_quiet)
+static int i40e_vc_send_msg_to_vf(struct i40e_vf *vf, u32 v_opcode,
+				  u32 v_retval, u8 *msg, u16 msglen)
 {
 	struct i40e_pf *pf;
 	struct i40e_hw *hw;
@@ -1944,25 +1942,6 @@ static int i40e_vc_send_msg_to_vf_ex(struct i40e_vf *vf, u32 v_opcode,
 	hw = &pf->hw;
 	abs_vf_id = vf->vf_id + hw->func_caps.vf_base_id;
 
-	/* single place to detect unsuccessful return values */
-	if (v_retval && !is_quiet) {
-		vf->num_invalid_msgs++;
-		dev_info(&pf->pdev->dev, "VF %d failed opcode %d, retval: %d\n",
-			 vf->vf_id, v_opcode, v_retval);
-		if (vf->num_invalid_msgs >
-		    I40E_DEFAULT_NUM_INVALID_MSGS_ALLOWED) {
-			dev_err(&pf->pdev->dev,
-				"Number of invalid messages exceeded for VF %d\n",
-				vf->vf_id);
-			dev_err(&pf->pdev->dev, "Use PF Control I/F to enable the VF\n");
-			set_bit(I40E_VF_STATE_DISABLED, &vf->vf_states);
-		}
-	} else {
-		vf->num_valid_msgs++;
-		/* reset the invalid counter, if a valid message is received. */
-		vf->num_invalid_msgs = 0;
-	}
-
 	aq_ret = i40e_aq_send_msg_to_vf(hw, abs_vf_id,	v_opcode, v_retval,
 					msg, msglen, NULL);
 	if (aq_ret) {
@@ -1975,23 +1954,6 @@ static int i40e_vc_send_msg_to_vf_ex(struct i40e_vf *vf, u32 v_opcode,
 	return 0;
 }
 
-/**
- * i40e_vc_send_msg_to_vf
- * @vf: pointer to the VF info
- * @v_opcode: virtual channel opcode
- * @v_retval: virtual channel return value
- * @msg: pointer to the msg buffer
- * @msglen: msg length
- *
- * send msg to VF
- **/
-static int i40e_vc_send_msg_to_vf(struct i40e_vf *vf, u32 v_opcode,
-				  u32 v_retval, u8 *msg, u16 msglen)
-{
-	return i40e_vc_send_msg_to_vf_ex(vf, v_opcode, v_retval,
-					 msg, msglen, false);
-}
-
 /**
  * i40e_vc_send_resp_to_vf
  * @vf: pointer to the VF info
@@ -2822,7 +2784,6 @@ static int i40e_vc_get_stats_msg(struct i40e_vf *vf, u8 *msg)
  * i40e_check_vf_permission
  * @vf: pointer to the VF info
  * @al: MAC address list from virtchnl
- * @is_quiet: set true for printing msg without opcode info, false otherwise
  *
  * Check that the given list of MAC addresses is allowed. Will return -EPERM
  * if any address in the list is not valid. Checks the following conditions:
@@ -2837,8 +2798,7 @@ static int i40e_vc_get_stats_msg(struct i40e_vf *vf, u8 *msg)
  * addresses might not be accurate.
  **/
 static inline int i40e_check_vf_permission(struct i40e_vf *vf,
-					   struct virtchnl_ether_addr_list *al,
-					   bool *is_quiet)
+					   struct virtchnl_ether_addr_list *al)
 {
 	struct i40e_pf *pf = vf->pf;
 	struct i40e_vsi *vsi = pf->vsi[vf->lan_vsi_idx];
@@ -2846,7 +2806,6 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 	int mac2add_cnt = 0;
 	int i;
 
-	*is_quiet = false;
 	for (i = 0; i < al->num_elements; i++) {
 		struct i40e_mac_filter *f;
 		u8 *addr = al->list[i].addr;
@@ -2870,7 +2829,6 @@ static inline int i40e_check_vf_permission(struct i40e_vf *vf,
 		    !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
 			dev_err(&pf->pdev->dev,
 				"VF attempting to override administratively set MAC address, bring down and up the VF interface to resume normal operation\n");
-			*is_quiet = true;
 			return -EPERM;
 		}
 
@@ -2921,7 +2879,6 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 	    (struct virtchnl_ether_addr_list *)msg;
 	struct i40e_pf *pf = vf->pf;
 	struct i40e_vsi *vsi = NULL;
-	bool is_quiet = false;
 	i40e_status ret = 0;
 	int i;
 
@@ -2938,7 +2895,7 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 	 */
 	spin_lock_bh(&vsi->mac_filter_hash_lock);
 
-	ret = i40e_check_vf_permission(vf, al, &is_quiet);
+	ret = i40e_check_vf_permission(vf, al);
 	if (ret) {
 		spin_unlock_bh(&vsi->mac_filter_hash_lock);
 		goto error_param;
@@ -2976,8 +2933,8 @@ static int i40e_vc_add_mac_addr_msg(struct i40e_vf *vf, u8 *msg)
 
 error_param:
 	/* send the response to the VF */
-	return i40e_vc_send_msg_to_vf_ex(vf, VIRTCHNL_OP_ADD_ETH_ADDR,
-				       ret, NULL, 0, is_quiet);
+	return i40e_vc_send_msg_to_vf(vf, VIRTCHNL_OP_ADD_ETH_ADDR,
+				      ret, NULL, 0);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
index 03c42fd0fea1..a554d0a0b09b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h
@@ -10,8 +10,6 @@
 
 #define I40E_VIRTCHNL_SUPPORTED_QTYPES 2
 
-#define I40E_DEFAULT_NUM_INVALID_MSGS_ALLOWED	10
-
 #define I40E_VLAN_PRIORITY_SHIFT	13
 #define I40E_VLAN_MASK			0xFFF
 #define I40E_PRIORITY_MASK		0xE000
@@ -92,9 +90,6 @@ struct i40e_vf {
 	u8 num_queue_pairs;	/* num of qps assigned to VF vsis */
 	u8 num_req_queues;	/* num of requested qps */
 	u64 num_mdd_events;	/* num of mdd events detected */
-	/* num of continuous malformed or invalid msgs detected */
-	u64 num_invalid_msgs;
-	u64 num_valid_msgs;	/* num of valid msgs detected */
 
 	unsigned long vf_caps;	/* vf's adv. capabilities */
 	unsigned long vf_states;	/* vf's runtime states */

base-commit: 35410c10918a84c9685cd644686ca637bb3c046b
-- 
2.35.1.129.gb80121027d12

[Intel-wired-lan] [net PATCH 1/2] i40e: stop disabling VFs due to PF error responses

[Jankowski, Konrad0]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Jacob Keller
> Sent: Thursday, February 17, 2022 1:52 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Subject: [Intel-wired-lan] [net PATCH 1/2] i40e: stop disabling VFs due to PF
> error responses
> 
> The i40e_vc_send_msg_to_vf_ex (and its wrapper
> i40e_vc_send_msg_to_vf) function has logic to detect "failure" responses
> sent to the VF. If a VF is sent more than
> I40E_DEFAULT_NUM_INVALID_MSGS_ALLOWED, then the VF is marked as
> disabled. In either case, a dev_info message is printed stating that a VF
> opcode failed.
> 
> This logic originates from the early implementation of VF support in commit
> 5c3c48ac6bf5 ("i40e: implement virtual device interface").
> 
> That commit did not go far enough. The "logic" for this behavior seems to be
> that error responses somehow indicate a malicious VF. This is not really true.
> The PF might be sending an error for any number of reasons such as lacking
> resources, an unsupported operation, etc. This does not indicate a malicious
> VF. We already have a separate robust malicious VF detection which relies on
> hardware logic to detect and prevent a variety of behaviors.
> 
> There is no justification for this behavior in the original implementation. In
> fact, a later commit 18b7af57d9c1 ("i40e: Lower some message levels")
> reduced the opcode failure message from a dev_err to a dev_info. In
> addition, recent commit 01cbf50877e6 ("i40e: Fix to not show opcode msg on
> unsuccessful VF MAC change") changed the logic to allow quieting it for
> expected failures.
> 
> That commit prevented this logic from kicking in for specific circumstances.
> This change did not go far enough. The behavior is not documented nor is it
> part of any requirement for our products. Other operating systems such as
> the FreeBSD implementation of our driver do not include this logic.
> 
> It is clear this check does not make sense, and causes problems which led to
> ugly workarounds.
> 
> Fix this by just removing the entire logic and the need for the
> i40e_vc_send_msg_to_vf_ex function.
> 
> Fixes: 01cbf50877e6 ("i40e: Fix to not show opcode msg on unsuccessful VF
> MAC change")
> Fixes: 5c3c48ac6bf5 ("i40e: implement virtual device interface")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  .../net/ethernet/intel/i40e/i40e_debugfs.c    |  6 +-
>  .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 57 +++----------------
>  .../ethernet/intel/i40e/i40e_virtchnl_pf.h    |  5 --
>  3 files changed, 9 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> index 1e57cc8c47d7..9db5001297c7 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c
> @@ -742,10 +742,8 @@ static void i40e_dbg_dump_vf(struct i40e_pf *pf, int
> vf_id)

Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>

[Intel-wired-lan] [net PATCH 2/2] ice: stop disabling VFs due to PF error responses

[Jacob Keller]

The ice_vc_send_msg_to_vf function has logic to detect "failure"
responses being sent to a VF. If a VF is sent more than
ICE_DFLT_NUM_INVAL_MSGS_ALLOWED then the VF is marked as disabled.
Almost identical logic also existed in the i40e driver.

This logic was added to the ice driver in commit 1071a8358a28 ("ice:
Implement virtchnl commands for AVF support") which itself copied from
the i40e implementation in commit 5c3c48ac6bf5 ("i40e: implement virtual
device interface").

Neither commit provides a proper explanation or justification of the
check. In fact, later commits to i40e changed the logic to allow
bypassing the check in some specific instances.

The "logic" for this seems to be that error responses somehow indicate a
malicious VF. This is not really true. The PF might be sending an error
for any number of reasons such as lack of resources, etc.

Additionally, this causes the PF to log an info message for every failed
VF response which may confuse users, and can spam the kernel log.

This behavior is not documented as part of any requirement for our
products and other operating system drivers such as the FreeBSD
implementation of our drivers do not include this type of check.

In fact, the change from dev_err to dev_info in i40e commit 18b7af57d9c1
("i40e: Lower some message levels") explains that these messages
typically don't actually indicate a real issue. It is quite likely that
a user who hits this in practice will be very confused as the VF will be
disabled without an obvious way to recover.

We already have robust malicious driver detection logic using actual
hardware detection mechanisms that detect and prevent invalid device
usage. Remove the logic since its not a documented requirement and the
behavior is not intuitive.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 .../net/ethernet/intel/ice/ice_virtchnl_pf.c   | 18 ------------------
 .../net/ethernet/intel/ice/ice_virtchnl_pf.h   |  3 ---
 2 files changed, 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
index 8a61b23f7cb3..353c2a3755d0 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
@@ -2180,24 +2180,6 @@ ice_vc_send_msg_to_vf(struct ice_vf *vf, u32 v_opcode,
 
 	dev = ice_pf_to_dev(pf);
 
-	/* single place to detect unsuccessful return values */
-	if (v_retval) {
-		vf->num_inval_msgs++;
-		dev_info(dev, "VF %d failed opcode %d, retval: %d\n", vf->vf_id,
-			 v_opcode, v_retval);
-		if (vf->num_inval_msgs > ICE_DFLT_NUM_INVAL_MSGS_ALLOWED) {
-			dev_err(dev, "Number of invalid messages exceeded for VF %d\n",
-				vf->vf_id);
-			dev_err(dev, "Use PF Control I/F to enable the VF\n");
-			set_bit(ICE_VF_STATE_DIS, vf->vf_states);
-			return -EIO;
-		}
-	} else {
-		vf->num_valid_msgs++;
-		/* reset the invalid counter, if a valid message is received. */
-		vf->num_inval_msgs = 0;
-	}
-
 	aq_ret = ice_aq_send_msg_to_vf(&pf->hw, vf->vf_id, v_opcode, v_retval,
 				       msg, msglen, NULL);
 	if (aq_ret && pf->hw.mailboxq.sq_last_status != ICE_AQ_RC_ENOSYS) {
diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h
index 752487a1bdd6..8f27255cc0cc 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.h
@@ -14,7 +14,6 @@
 #define ICE_MAX_MACADDR_PER_VF		18
 
 /* Malicious Driver Detection */
-#define ICE_DFLT_NUM_INVAL_MSGS_ALLOWED		10
 #define ICE_MDD_EVENTS_THRESHOLD		30
 
 /* Static VF transaction/status register def */
@@ -134,8 +133,6 @@ struct ice_vf {
 	unsigned int max_tx_rate;	/* Maximum Tx bandwidth limit in Mbps */
 	DECLARE_BITMAP(vf_states, ICE_VF_STATES_NBITS);	/* VF runtime states */
 
-	u64 num_inval_msgs;		/* number of continuous invalid msgs */
-	u64 num_valid_msgs;		/* number of valid msgs detected */
 	unsigned long vf_caps;		/* VF's adv. capabilities */
 	u8 num_req_qs;			/* num of queue pairs requested by VF */
 	u16 num_mac;
-- 
2.35.1.129.gb80121027d12

[Intel-wired-lan] [net PATCH 2/2] ice: stop disabling VFs due to PF error responses

[Nguyen, Anthony L]

On Wed, 2022-02-16 at 16:51 -0800, Jacob Keller wrote:
> The ice_vc_send_msg_to_vf function has logic to detect "failure"
> responses being sent to a VF. If a VF is sent more than
> ICE_DFLT_NUM_INVAL_MSGS_ALLOWED then the VF is marked as disabled.
> Almost identical logic also existed in the i40e driver.
> 
> This logic was added to the ice driver in commit 1071a8358a28 ("ice:
> Implement virtchnl commands for AVF support") which itself copied
> from
> the i40e implementation in commit 5c3c48ac6bf5 ("i40e: implement
> virtual
> device interface").
> 
> Neither commit provides a proper explanation or justification of the
> check. In fact, later commits to i40e changed the logic to allow
> bypassing the check in some specific instances.
> 
> The "logic" for this seems to be that error responses somehow
> indicate a
> malicious VF. This is not really true. The PF might be sending an
> error
> for any number of reasons such as lack of resources, etc.
> 
> Additionally, this causes the PF to log an info message for every
> failed
> VF response which may confuse users, and can spam the kernel log.
> 
> This behavior is not documented as part of any requirement for our
> products and other operating system drivers such as the FreeBSD
> implementation of our drivers do not include this type of check.
> 
> In fact, the change from dev_err to dev_info in i40e commit
> 18b7af57d9c1
> ("i40e: Lower some message levels") explains that these messages
> typically don't actually indicate a real issue. It is quite likely
> that
> a user who hits this in practice will be very confused as the VF will
> be
> disabled without an obvious way to recover.
> 
> We already have robust malicious driver detection logic using actual
> hardware detection mechanisms that detect and prevent invalid device
> usage. Remove the logic since its not a documented requirement and
> the
> behavior is not intuitive.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

If this is for net, it should have a Fixes: as well.

Thanks,
Tony

[Intel-wired-lan] [net PATCH 2/2] ice: stop disabling VFs due to PF error responses

[Keller, Jacob E]

> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Sent: Thursday, February 17, 2022 9:25 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [net PATCH 2/2] ice: stop disabling VFs due to PF
> error responses
> 
> On Wed, 2022-02-16 at 16:51 -0800, Jacob Keller wrote:
> > The ice_vc_send_msg_to_vf function has logic to detect "failure"
> > responses being sent to a VF. If a VF is sent more than
> > ICE_DFLT_NUM_INVAL_MSGS_ALLOWED then the VF is marked as disabled.
> > Almost identical logic also existed in the i40e driver.
> >
> > This logic was added to the ice driver in commit 1071a8358a28 ("ice:
> > Implement virtchnl commands for AVF support") which itself copied
> > from
> > the i40e implementation in commit 5c3c48ac6bf5 ("i40e: implement
> > virtual
> > device interface").
> >
> > Neither commit provides a proper explanation or justification of the
> > check. In fact, later commits to i40e changed the logic to allow
> > bypassing the check in some specific instances.
> >
> > The "logic" for this seems to be that error responses somehow
> > indicate a
> > malicious VF. This is not really true. The PF might be sending an
> > error
> > for any number of reasons such as lack of resources, etc.
> >
> > Additionally, this causes the PF to log an info message for every
> > failed
> > VF response which may confuse users, and can spam the kernel log.
> >
> > This behavior is not documented as part of any requirement for our
> > products and other operating system drivers such as the FreeBSD
> > implementation of our drivers do not include this type of check.
> >
> > In fact, the change from dev_err to dev_info in i40e commit
> > 18b7af57d9c1
> > ("i40e: Lower some message levels") explains that these messages
> > typically don't actually indicate a real issue. It is quite likely
> > that
> > a user who hits this in practice will be very confused as the VF will
> > be
> > disabled without an obvious way to recover.
> >
> > We already have robust malicious driver detection logic using actual
> > hardware detection mechanisms that detect and prevent invalid device
> > usage. Remove the logic since its not a documented requirement and
> > the
> > behavior is not intuitive.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> 
> If this is for net, it should have a Fixes: as well.
> 
> Thanks,
> Tony

Fixes: 1071a8358a28 ("ice: Implement virtchnl commands for AVF support")

[Intel-wired-lan] [net PATCH 2/2] ice: stop disabling VFs due to PF error responses

[Jankowski, Konrad0]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Jacob Keller
> Sent: Thursday, February 17, 2022 1:52 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Subject: [Intel-wired-lan] [net PATCH 2/2] ice: stop disabling VFs due to PF
> error responses
> 
> The ice_vc_send_msg_to_vf function has logic to detect "failure"
> responses being sent to a VF. If a VF is sent more than
> ICE_DFLT_NUM_INVAL_MSGS_ALLOWED then the VF is marked as disabled.
> Almost identical logic also existed in the i40e driver.
> 
> This logic was added to the ice driver in commit 1071a8358a28 ("ice:
> Implement virtchnl commands for AVF support") which itself copied from the
> i40e implementation in commit 5c3c48ac6bf5 ("i40e: implement virtual device
> interface").
> 
> Neither commit provides a proper explanation or justification of the check. In
> fact, later commits to i40e changed the logic to allow bypassing the check in
> some specific instances.
> 
> The "logic" for this seems to be that error responses somehow indicate a
> malicious VF. This is not really true. The PF might be sending an error for any
> number of reasons such as lack of resources, etc.
> 
> Additionally, this causes the PF to log an info message for every failed VF
> response which may confuse users, and can spam the kernel log.
> 
> This behavior is not documented as part of any requirement for our products
> and other operating system drivers such as the FreeBSD implementation of
> our drivers do not include this type of check.
> 
> In fact, the change from dev_err to dev_info in i40e commit 18b7af57d9c1
> ("i40e: Lower some message levels") explains that these messages typically
> don't actually indicate a real issue. It is quite likely that a user who hits this in
> practice will be very confused as the VF will be disabled without an obvious
> way to recover.
> 
> We already have robust malicious driver detection logic using actual
> hardware detection mechanisms that detect and prevent invalid device
> usage. Remove the logic since its not a documented requirement and the
> behavior is not intuitive.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  .../net/ethernet/intel/ice/ice_virtchnl_pf.c   | 18 ------------------
>  .../net/ethernet/intel/ice/ice_virtchnl_pf.h   |  3 ---
>  2 files changed, 21 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
> index 8a61b23f7cb3..353c2a3755d0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_pf.c
> @@ -2180,24 +2180,6 @@ ice_vc_send_msg_to_vf(struct ice_vf *vf, u32

Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>