[PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2021-08-09

[PATCH net 0/4][pull request] Intel Wired LAN Driver Updates 2021-08-09

[Tony Nguyen]

This series contains updates to ice and iavf drivers.

Ani prevents the ice driver from accidentally being probed to a virtual
function and stops processing of VF messages when VFs are being torn
down.

Brett prevents the ice driver from deleting is own MAC address.

Fahad ensures the RSS LUT and key are always set following reset for
iavf.

The following are changes since commit d09c548dbf3b31cb07bba562e0f452edfa01efe3:
  net: sched: act_mirred: Reset ct info when mirror/redirect skb
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Anirudh Venkataramanan (2):
  ice: Prevent probing virtual functions
  ice: Stop processing VF messages during teardown

Brett Creeley (1):
  ice: don't remove netdev->dev_addr from uc sync list

Md Fahad Iqbal Polash (1):
  iavf: Set RSS LUT and key in reset handle path

 drivers/net/ethernet/intel/iavf/iavf_main.c   | 13 +++++----
 drivers/net/ethernet/intel/ice/ice.h          |  1 +
 drivers/net/ethernet/intel/ice/ice_main.c     | 28 +++++++++++++------
 .../net/ethernet/intel/ice/ice_virtchnl_pf.c  |  7 +++++
 4 files changed, 36 insertions(+), 13 deletions(-)

-- 
2.26.2

[PATCH net 1/4] ice: Prevent probing virtual functions

[Tony Nguyen]

From: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>

The userspace utility "driverctl" can be used to change/override the
system's default driver choices. This is useful in some situations
(buggy driver, old driver missing a device ID, trying a workaround,
etc.) where the user needs to load a different driver.

However, this is also prone to user error, where a driver is mapped
to a device it's not designed to drive. For example, if the ice driver
is mapped to driver iavf devices, the ice driver crashes.

Add a check to return an error if the ice driver is being used to
probe a virtual function.

Fixes: 837f08fdecbe ("ice: Add basic driver framework for Intel(R) E800 Series")
Signed-off-by: Anirudh Venkataramanan <anirudh.venkataramanan@intel.com>
Tested-by: Gurucharan G <gurucharanx.g@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_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index ef8d1815af56..a9506041eb42 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4194,6 +4194,11 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
 	struct ice_hw *hw;
 	int i, err;
 
+	if (pdev->is_virtfn) {
+		dev_err(dev, "can't probe a virtual function\n");
+		return -EINVAL;
+	}
+
 	/* this driver uses devres, see
 	 * Documentation/driver-api/driver-model/devres.rst
 	 */
-- 
2.26.2

Re: [PATCH net 1/4] ice: Prevent probing virtual functions

[Jakub Kicinski]

On Mon,  9 Aug 2021 10:13:59 -0700 Tony Nguyen wrote:
> The userspace utility "driverctl" can be used to change/override the
> system's default driver choices. This is useful in some situations
> (buggy driver, old driver missing a device ID, trying a workaround,
> etc.) where the user needs to load a different driver.
> 
> However, this is also prone to user error, where a driver is mapped
> to a device it's not designed to drive. For example, if the ice driver
> is mapped to driver iavf devices, the ice driver crashes.
> 
> Add a check to return an error if the ice driver is being used to
> probe a virtual function.

This seems too selective. Also drivers should not trust the HW is sane
and crash the kernel. Please fix the crashes and fail probe gracefully
in all cases without resorting to duct tape.

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

[Tony Nguyen]

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(+)

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;
-- 
2.26.2

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

[Jakub Kicinski]

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...

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

[Venkataramanan, Anirudh]

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

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

[Jonathan Toppins]

On 8/9/21 1:14 PM, Tony Nguyen wrote:
> 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>

Assuming only the commit message is fixed up.

Tested-by: Jonathan Toppins <jtoppins@redhat.com>

[PATCH net 3/4] ice: don't remove netdev->dev_addr from uc sync list

[Tony Nguyen]

From: Brett Creeley <brett.creeley@intel.com>

In some circumstances, such as with bridging, it's possible that the
stack will add the device's own MAC address to its unicast address list.

If, later, the stack deletes this address, the driver will receive a
request to remove this address.

The driver stores its current MAC address as part of the VSI MAC filter
list instead of separately. So, this causes a problem when the device's
MAC address is deleted unexpectedly, which results in traffic failure in
some cases.

The following configuration steps will reproduce the previously
mentioned problem:

> ip link set eth0 up
> ip link add dev br0 type bridge
> ip link set br0 up
> ip addr flush dev eth0
> ip link set eth0 master br0
> echo 1 > /sys/class/net/br0/bridge/vlan_filtering
> modprobe -r veth
> modprobe -r bridge
> ip addr add 192.168.1.100/24 dev eth0

The following ping command fails due to the netdev->dev_addr being
deleted when removing the bridge module.
> ping <link partner>

Fix this by making sure to not delete the netdev->dev_addr during MAC
address sync. After fixing this issue it was noticed that the
netdev_warn() in .set_mac was overly verbose, so make it at
netdev_dbg().

Also, there is a possibility of a race condition between .set_mac and
.set_rx_mode. Fix this by calling netif_addr_lock_bh() and
netif_addr_unlock_bh() on the device's netdev when the netdev->dev_addr
is going to be updated in .set_mac.

Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
Signed-off-by: Brett Creeley <brett.creeley@intel.com>
Tested-by: Liang Li <liali@redhat.com>
Tested-by: Gurucharan G <gurucharanx.g@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index a9506041eb42..fe2ded775f25 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -191,6 +191,14 @@ static int ice_add_mac_to_unsync_list(struct net_device *netdev, const u8 *addr)
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	struct ice_vsi *vsi = np->vsi;
 
+	/* Under some circumstances, we might receive a request to delete our
+	 * own device address from our uc list. Because we store the device
+	 * address in the VSI's MAC filter list, we need to ignore such
+	 * requests and not delete our device address from this list.
+	 */
+	if (ether_addr_equal(addr, netdev->dev_addr))
+		return 0;
+
 	if (ice_fltr_add_mac_to_list(vsi, &vsi->tmp_unsync_list, addr,
 				     ICE_FWD_TO_VSI))
 		return -EINVAL;
@@ -5124,7 +5132,7 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
 		return -EADDRNOTAVAIL;
 
 	if (ether_addr_equal(netdev->dev_addr, mac)) {
-		netdev_warn(netdev, "already using mac %pM\n", mac);
+		netdev_dbg(netdev, "already using mac %pM\n", mac);
 		return 0;
 	}
 
@@ -5135,6 +5143,7 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
 		return -EBUSY;
 	}
 
+	netif_addr_lock_bh(netdev);
 	/* Clean up old MAC filter. Not an error if old filter doesn't exist */
 	status = ice_fltr_remove_mac(vsi, netdev->dev_addr, ICE_FWD_TO_VSI);
 	if (status && status != ICE_ERR_DOES_NOT_EXIST) {
@@ -5144,30 +5153,28 @@ static int ice_set_mac_address(struct net_device *netdev, void *pi)
 
 	/* Add filter for new MAC. If filter exists, return success */
 	status = ice_fltr_add_mac(vsi, mac, ICE_FWD_TO_VSI);
-	if (status == ICE_ERR_ALREADY_EXISTS) {
+	if (status == ICE_ERR_ALREADY_EXISTS)
 		/* Although this MAC filter is already present in hardware it's
 		 * possible in some cases (e.g. bonding) that dev_addr was
 		 * modified outside of the driver and needs to be restored back
 		 * to this value.
 		 */
-		memcpy(netdev->dev_addr, mac, netdev->addr_len);
 		netdev_dbg(netdev, "filter for MAC %pM already exists\n", mac);
-		return 0;
-	}
-
-	/* error if the new filter addition failed */
-	if (status)
+	else if (status)
+		/* error if the new filter addition failed */
 		err = -EADDRNOTAVAIL;
 
 err_update_filters:
 	if (err) {
 		netdev_err(netdev, "can't set MAC %pM. filter update failed\n",
 			   mac);
+		netif_addr_unlock_bh(netdev);
 		return err;
 	}
 
 	/* change the netdev's MAC address */
 	memcpy(netdev->dev_addr, mac, netdev->addr_len);
+	netif_addr_unlock_bh(netdev);
 	netdev_dbg(vsi->netdev, "updated MAC address to %pM\n",
 		   netdev->dev_addr);
 
-- 
2.26.2

Re: [PATCH net 3/4] ice: don't remove netdev->dev_addr from uc sync list

[Jonathan Toppins]

On 8/9/21 1:14 PM, Tony Nguyen wrote:
> From: Brett Creeley <brett.creeley@intel.com>
> 
> In some circumstances, such as with bridging, it's possible that the
> stack will add the device's own MAC address to its unicast address list.
> 
> If, later, the stack deletes this address, the driver will receive a
> request to remove this address.
> 
> The driver stores its current MAC address as part of the VSI MAC filter
> list instead of separately. So, this causes a problem when the device's
> MAC address is deleted unexpectedly, which results in traffic failure in
> some cases.
> 
> The following configuration steps will reproduce the previously
> mentioned problem:
> 
>> ip link set eth0 up
>> ip link add dev br0 type bridge
>> ip link set br0 up
>> ip addr flush dev eth0
>> ip link set eth0 master br0
>> echo 1 > /sys/class/net/br0/bridge/vlan_filtering
>> modprobe -r veth
>> modprobe -r bridge
>> ip addr add 192.168.1.100/24 dev eth0
> 
> The following ping command fails due to the netdev->dev_addr being
> deleted when removing the bridge module.
>> ping <link partner>
> 
> Fix this by making sure to not delete the netdev->dev_addr during MAC
> address sync. After fixing this issue it was noticed that the
> netdev_warn() in .set_mac was overly verbose, so make it at
> netdev_dbg().
> 
> Also, there is a possibility of a race condition between .set_mac and
> .set_rx_mode. Fix this by calling netif_addr_lock_bh() and
> netif_addr_unlock_bh() on the device's netdev when the netdev->dev_addr
> is going to be updated in .set_mac.
> 
> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> Tested-by: Liang Li <liali@redhat.com>
> Tested-by: Gurucharan G <gurucharanx.g@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Tested-by: Jonathan Toppins <jtoppins@redhat.com>

[PATCH net 4/4] iavf: Set RSS LUT and key in reset handle path

[Tony Nguyen]

From: Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>

iavf driver should set RSS LUT and key unconditionally in reset
path. Currently, the driver does not do that. This patch fixes
this issue.

Fixes: 2c86ac3c7079 ("i40evf: create a generic config RSS function")
Signed-off-by: Md Fahad Iqbal Polash <md.fahad.iqbal.polash@intel.com>
Tested-by: Konrad Jankowski <konrad0.jankowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 44bafedd09f2..244ec74ceca7 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1506,11 +1506,6 @@ static int iavf_reinit_interrupt_scheme(struct iavf_adapter *adapter)
 	set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
 
 	iavf_map_rings_to_vectors(adapter);
-
-	if (RSS_AQ(adapter))
-		adapter->aq_required |= IAVF_FLAG_AQ_CONFIGURE_RSS;
-	else
-		err = iavf_init_rss(adapter);
 err:
 	return err;
 }
@@ -2200,6 +2195,14 @@ static void iavf_reset_task(struct work_struct *work)
 			goto reset_err;
 	}
 
+	if (RSS_AQ(adapter)) {
+		adapter->aq_required |= IAVF_FLAG_AQ_CONFIGURE_RSS;
+	} else {
+		err = iavf_init_rss(adapter);
+		if (err)
+			goto reset_err;
+	}
+
 	adapter->aq_required |= IAVF_FLAG_AQ_GET_CONFIG;
 	adapter->aq_required |= IAVF_FLAG_AQ_MAP_VECTORS;
 
-- 
2.26.2