[Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr from uc sync list

[Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr from uc sync list

[Brett Creeley]

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.

Fix this by making sure to not delete the netdev->dev_addr during
MAC address sync.

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.

Also, change the netdev_warn() to netdev_dbg() when the device is
already using the requested mac in .set_mac. The dev_warn() was causing
a lot of unnecessary noise when configuring/unconfiguring bridging and
provides no benefit.

Lastly, instead of using memcpy() to save the netdev->dev_addr, use
ether_addr_copy() in .set_mac.

Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
Signed-off-by: Brett Creeley <brett.creeley@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 25 +++++++++++++++--------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index ef8d1815af56..259d73e353bb 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;
@@ -5119,7 +5127,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;
 	}
 
@@ -5130,6 +5138,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) {
@@ -5139,30 +5148,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);
+	ether_addr_copy(netdev->dev_addr, mac);
+	netif_addr_unlock_bh(netdev);
 	netdev_dbg(vsi->netdev, "updated MAC address to %pM\n",
 		   netdev->dev_addr);
 
-- 
2.26.3

[Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr from uc sync list

[G, GurucharanX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Brett Creeley
> Sent: Thursday, July 29, 2021 2:05 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr
> from uc sync list
> 
> 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.
> 
> Fix this by making sure to not delete the netdev->dev_addr during MAC
> address sync.
> 
> 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.
> 
> Also, change the netdev_warn() to netdev_dbg() when the device is already
> using the requested mac in .set_mac. The dev_warn() was causing a lot of
> unnecessary noise when configuring/unconfiguring bridging and provides no
> benefit.
> 
> Lastly, instead of using memcpy() to save the netdev->dev_addr, use
> ether_addr_copy() in .set_mac.
> 
> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump
> version")
> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 25 +++++++++++++++--------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 

Tested-by: Gurucharan  G <Gurucharanx.g@intel.com> (A Contingent Worker at Intel)

[Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr from uc sync list

[Paul Menzel]

Dear Brett,


Am 28.07.21 um 22:34 schrieb Brett Creeley:
> 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.
> 
> Fix this by making sure to not delete the netdev->dev_addr during
> MAC address sync.

Is it easy to reproduce?

> 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.
> 
> Also, change the netdev_warn() to netdev_dbg() when the device is
> already using the requested mac in .set_mac. The dev_warn() was causing
> a lot of unnecessary noise when configuring/unconfiguring bridging and
> provides no benefit.
> 
> Lastly, instead of using memcpy() to save the netdev->dev_addr, use
> ether_addr_copy() in .set_mac.

It?d be great, if you split the three items out into separate patches, 
and submit it in a patch series.


Kind regards,

Paul


> Fixes: e94d44786693 ("ice: Implement filter sync, NDO operations and bump version")
> Signed-off-by: Brett Creeley <brett.creeley@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_main.c | 25 +++++++++++++++--------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index ef8d1815af56..259d73e353bb 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;
> @@ -5119,7 +5127,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;
>   	}
>   
> @@ -5130,6 +5138,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) {
> @@ -5139,30 +5148,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);
> +	ether_addr_copy(netdev->dev_addr, mac);
> +	netif_addr_unlock_bh(netdev);
>   	netdev_dbg(vsi->netdev, "updated MAC address to %pM\n",
>   		   netdev->dev_addr);
>   
> 

[Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr from uc sync list

[Creeley, Brett]

On Thu, 2021-08-05 at 08:51 +0200, Paul Menzel wrote:
> Dear Brett,
> 
> 
> Am 28.07.21 um 22:34 schrieb Brett Creeley:
> > 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.
> > 
> > Fix this by making sure to not delete the netdev->dev_addr during
> > MAC address sync.
> 
> Is it easy to reproduce?

Yes, I will add more details when I re-spin the patch.

> 
> > 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.
> > 
> > Also, change the netdev_warn() to netdev_dbg() when the device is
> > already using the requested mac in .set_mac. The dev_warn() was
> > causing
> > a lot of unnecessary noise when configuring/unconfiguring bridging
> > and
> > provides no benefit.
> > 
> > Lastly, instead of using memcpy() to save the netdev->dev_addr, use
> > ether_addr_copy() in .set_mac.
> 
> It?d be great, if you split the three items out into separate
> patches, 
> and submit it in a patch series.

I'm thinking that this is actually a 2 patch series. With the fix
to not delete the device's address, the netdev_warn() was causing
a lot of noise. Based on this I believe that the netdev_warn() to
netdev_dbg() should be part of the same patch that fixes the
previously mentioned problem.

However, I will send the ether_addr_copy() change to net-next/ as
a separate patch since it is not fixing anything, just a general
improvement.

Hopefully this works for you and thanks for the feedback!

Brett
> 
> 
> Kind regards,
> 
> Paul
> 
<snip>

[Intel-wired-lan] [PATCH net] ice: don't remove netdev->dev_addr from uc sync list

[Paul Menzel]

Dear Brett,


Am 05.08.21 um 20:30 schrieb Creeley, Brett:
> On Thu, 2021-08-05 at 08:51 +0200, Paul Menzel wrote:

>> Am 28.07.21 um 22:34 schrieb Brett Creeley:
>>> 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.
>>>
>>> Fix this by making sure to not delete the netdev->dev_addr during
>>> MAC address sync.
>>
>> Is it easy to reproduce?
> 
> Yes, I will add more details when I re-spin the patch.
> 
>>
>>> 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.
>>>
>>> Also, change the netdev_warn() to netdev_dbg() when the device is
>>> already using the requested mac in .set_mac. The dev_warn() was
>>> causing
>>> a lot of unnecessary noise when configuring/unconfiguring bridging
>>> and
>>> provides no benefit.
>>>
>>> Lastly, instead of using memcpy() to save the netdev->dev_addr, use
>>> ether_addr_copy() in .set_mac.
>>
>> It?d be great, if you split the three items out into separate
>> patches, and submit it in a patch series.
> 
> I'm thinking that this is actually a 2 patch series. With the fix
> to not delete the device's address, the netdev_warn() was causing
> a lot of noise. Based on this I believe that the netdev_warn() to
> netdev_dbg() should be part of the same patch that fixes the
> previously mentioned problem.
> 
> However, I will send the ether_addr_copy() change to net-next/ as
> a separate patch since it is not fixing anything, just a general
> improvement.
> 
> Hopefully this works for you and thanks for the feedback!

Perfect. Thank you.


Kind regards,

Paul