Skip to contentSkip to conversation

i40e: fix mac filter delete when setting mac address

PATCH In review Stefan Assmann 3 participants 3 messages Dec 4, 2018

Subsystems (3)
Networking Network drivers Intel Ethernet
lore
Share & tools
lore

Stefan Assmann

author
#0 lore raw

PATCH 1/1 i40e: fix mac filter delete when setting mac address

A previous commit moved the ether_addr_copy() in i40e_set_mac() before the mac filter del/add to avoid a race. However it wasn't taken into account that this alters the mac address being handed to i40e_del_mac_filter().

Also changed i40e_add_mac_filter() to operate on netdev->dev_addr, hopefully that makes the code easier to read.

Fixes 458867b2ca0c ("i40e: don't remove netdev->dev_addr when syncing uc list")
Signed-off-by Stefan Assmann <[email protected]>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +7 -7
index 6d5b13f69dec..901ef799d2ad 100644
@@ -1546,17 +1546,17 @@ static int i40e_set_mac(struct net_device *netdev, void *p)
 		netdev_info(netdev, "set new mac address %pM\n", addr->sa_data);
 
 	/* Copy the address first, so that we avoid a possible race with
-	 * .set_rx_mode(). If we copy after changing the address in the filter
-	 * list, we might open ourselves to a narrow race window where
-	 * .set_rx_mode could delete our dev_addr filter and prevent traffic
-	 * from passing.
+	 * .set_rx_mode().
+	 * - Remove old address from MAC filter
+	 * - Copy new address
+	 * - Add new address to MAC filter
 	 */
-	ether_addr_copy(netdev->dev_addr, addr->sa_data);
-
 	spin_lock_bh(&vsi->mac_filter_hash_lock);
 	i40e_del_mac_filter(vsi, netdev->dev_addr);
-	i40e_add_mac_filter(vsi, addr->sa_data);
+	ether_addr_copy(netdev->dev_addr, addr->sa_data);
+	i40e_add_mac_filter(vsi, netdev->dev_addr);
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
+
 	if (vsi->type == I40E_VSI_MAIN) {
 		i40e_status ret;
Signature · 1 line
2.19.2
Tested-by on v1 1/1
#1 lore raw
Quoted 16 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On Behalf Of Stefan Assmann
Sent: Tuesday, December 4, 2018 6:19 AM
To: intel-wired-lan at lists.osuosl.org
Cc: netdev at vger.kernel.org; davem at davemloft.net; sassmann at kpanic.de
Subject: [Intel-wired-lan] [PATCH] i40e: fix mac filter delete when setting mac address

A previous commit moved the ether_addr_copy() in i40e_set_mac() before the mac filter del/add to avoid a race. However it wasn't taken into account that this alters the mac address being handed to i40e_del_mac_filter().

Also changed i40e_add_mac_filter() to operate on netdev->dev_addr, hopefully that makes the code easier to read.

Quoted 7 lines
Fixes 458867b2ca0c ("i40e: don't remove netdev->dev_addr when syncing

uc list")

Signed-off-by Stefan Assmann <[email protected]>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Tested-by Andrew Bowers <[email protected]>
Acked-by on v1 1/1
#2 lore raw
In reply to Stefan Assmann
Quoted 9 lines

-----Original Message-----
From: Stefan Assmann [mailto:sassmann at kpanic.de]
Sent: Tuesday, December 04, 2018 6:19 AM
To: intel-wired-lan at lists.osuosl.org
Cc: netdev at vger.kernel.org; davem at davemloft.net; Kirsher, Jeffrey T <[email protected]>; Keller, Jacob E <[email protected]>; sassmann at kpanic.de
Subject: [PATCH] i40e: fix mac filter delete when setting mac address

Quoted 4 lines

A previous commit moved the ether_addr_copy() in i40e_set_mac() before the mac filter del/add to avoid a race. However it wasn't taken into account that this alters the mac address being handed to i40e_del_mac_filter().

Woops!

Quoted 4 lines

Also changed i40e_add_mac_filter() to operate on netdev->dev_addr, hopefully that makes the code easier to read.

Makes sense.

Quoted 2 lines
Fixes 458867b2ca0c ("i40e: don't remove netdev->dev_addr when syncing uc list")

Good catch. This looks correct to me.

Acked-by Jacob Keller <[email protected]>
Quoted 38 lines
Signed-off-by Stefan Assmann <[email protected]>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +7 -8
index 6d5b13f69dec..901ef799d2ad 100644
@@ -1546,17 +1546,17 @@ static int i40e_set_mac(struct net_device *netdev, void
 *p)
 		netdev_info(netdev, "set new mac address %pM\n", addr->sa_data);
 
 	/* Copy the address first, so that we avoid a possible race with
-	 * .set_rx_mode(). If we copy after changing the address in the filter
-	 * list, we might open ourselves to a narrow race window where
-	 * .set_rx_mode could delete our dev_addr filter and prevent traffic
-	 * from passing.
+	 * .set_rx_mode().
+	 * - Remove old address from MAC filter
+	 * - Copy new address
+	 * - Add new address to MAC filter
 	 */
-	ether_addr_copy(netdev->dev_addr, addr->sa_data);
-
 	spin_lock_bh(&vsi->mac_filter_hash_lock);
 	i40e_del_mac_filter(vsi, netdev->dev_addr);
-	i40e_add_mac_filter(vsi, addr->sa_data);
+	ether_addr_copy(netdev->dev_addr, addr->sa_data);
+	i40e_add_mac_filter(vsi, netdev->dev_addr);
 	spin_unlock_bh(&vsi->mac_filter_hash_lock);
+
 	if (vsi->type == I40E_VSI_MAIN) {
 		i40e_status ret;
 
--

2.19.2