[PATCH net-next v2 0/3] macvlan: fix some problem if mac address changes

The macvlan may return failed when handle the NETDEV_CHANGEMTU message, and
the current code could not process the error value, so modify the
dev_set_mac_address to process the return value for notification chain,
revert the old mac address if set new mac address failed.

The macvlan and lowerdev should not have the same mac address for non-passthru
mode, so add restriction for that.

v1->v2: some drivers need to call ndo_set_mac_address() even though the same
        mac address, so remove the equal check and allow to set same mac address.

Ding Tianhong (3):
  macvlan: don't update the uc and vlan list for L2 forwarding offload
  net: dev: revert the mac address when notifier failed
  macvlan: don't set the same mac address for non-passthru mode

 drivers/net/macvlan.c |  8 ++++++--
 net/core/dev.c        | 23 +++++++++++++++++++----
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
1.8.0

The macvlan should have the same mac address only for passthru mode,
so the underlying device couldn't set a new address if it is in use
by macvlan for non-passthru mode, otherwise it will break the work
mechanism.

Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/macvlan.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index c3a54a6..edf1a1c 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c

@@ -1185,8 +1185,12 @@ static int macvlan_device_event(struct notifier_block *unused,
 		}
 		break;
 	case NETDEV_CHANGEADDR:
-		if (!port->passthru)
+		if (!port->passthru) {
+			if (macvlan_hash_lookup(port, dev->dev_addr))
+				return NOTIFY_BAD;
+
 			return NOTIFY_DONE;
+		}
 
 		vlan = list_first_entry_or_null(&port->vlans,
 						struct macvlan_dev,

-- 
1.8.0

When set a new mac address to a netdev, the dev should propagate
to the upperdev or lowerdev and make sure the new mac address could
work well with other devs, otherwise the new mac address shouldn't
be set and revert the old mac address.

Signed-off-by: Ding Tianhong <redacted>
---
 net/core/dev.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5367bfb..b0fb3dd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -5562,6 +5562,7 @@ EXPORT_SYMBOL(dev_set_group);
 int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
+	struct sockaddr old_sa;
 	int err;
 
 	if (!ops->ndo_set_mac_address)

@@ -5570,13 +5571,27 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+
+	old_sa.sa_family = dev->type;
+	ether_addr_copy(old_sa.sa_data, dev->dev_addr);
+
 	err = ops->ndo_set_mac_address(dev, sa);
 	if (err)
 		return err;
-	dev->addr_assign_type = NET_ADDR_SET;
-	call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
-	add_device_randomness(dev->dev_addr, dev->addr_len);
-	return 0;
+
+	err = call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+	err = notifier_to_errno(err);
+	if (err) {
+		/* setting mac address back and notify everyone again,
+		 * so that they have a chance to revert changes.
+		 */
+		ops->ndo_set_mac_address(dev, &old_sa);
+		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+	} else {
+		dev->addr_assign_type = NET_ADDR_SET;
+		add_device_randomness(dev->dev_addr, dev->addr_len);
+	}
+	return err;
 }
 EXPORT_SYMBOL(dev_set_mac_address);
 

-- 
1.8.0

If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 453d55a..c3a54a6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c

@@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (!(dev->flags & IFF_UP)) {
+	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
 		/* Just copy in the new address */
 		ether_addr_copy(dev->dev_addr, addr);
 	} else {

-- 
1.8.0

On 06/05/2014 08:01 AM, Ding Tianhong wrote:

If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Same issue as in v1.  Please explain how this actually works, since the
new address never makes it to the hw.

-vlad

On 2014/6/5 23:09, Vlad Yasevich wrote:

On 06/05/2014 08:01 AM, Ding Tianhong wrote:

quoted

If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Same issue as in v1.  Please explain how this actually works, since the
new address never makes it to the hw.

-vlad

quoted

Sorry, I miss the first feedback of yours, so didn't fix the issue from v1, I will think about it and than answer it, thanks.

Ding

quoted

Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 453d55a..c3a54a6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c

@@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (!(dev->flags & IFF_UP)) {
+	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
 		/* Just copy in the new address */
 		ether_addr_copy(dev->dev_addr, addr);
 	} else {

On 2014/6/5 23:09, Vlad Yasevich wrote:

On 06/05/2014 08:01 AM, Ding Tianhong wrote:

quoted

If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Same issue as in v1.  Please explain how this actually works, since the
new address never makes it to the hw.

Hi, vlad:

The fwd only support 82599 nic, and when fwd_priv is true, this means the lowerdev support L2 forwarding offload, and will set the macvlan
to the ixgbe_fwd_adapter->netdev, and then it is clear that the lowerdev could know that the the upper device is macvlan and could handle
the skb to the upperdev.

If I miss something, please remind me, thanks.

Ding

-vlad

quoted

Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 453d55a..c3a54a6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c

@@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (!(dev->flags & IFF_UP)) {
+	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
 		/* Just copy in the new address */
 		ether_addr_copy(dev->dev_addr, addr);
 	} else {

On 06/05/2014 11:45 PM, Ding Tianhong wrote:

On 2014/6/5 23:09, Vlad Yasevich wrote:

quoted

On 06/05/2014 08:01 AM, Ding Tianhong wrote:

quoted

If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Same issue as in v1.  Please explain how this actually works, since the
new address never makes it to the hw.

Hi, vlad:

The fwd only support 82599 nic, and when fwd_priv is true, this means the lowerdev support L2 forwarding offload, and will set the macvlan
to the ixgbe_fwd_adapter->netdev, and then it is clear that the lowerdev could know that the the upper device is macvlan and could handle
the skb to the upperdev.

If I miss something, please remind me, thanks.

You've just changed the mac address of the macvlan device, but the
address is not written to the card's RAR registers.
So what allows the card to receive the traffic and place it into the
queue in the first place?

I can see it possibly working if the card is in promiscuous mode.

Also, I think John is right in that if we simply do dev_uc_add()
to add the address to the card, we'll unmap the address from the vmdqs
and lose the acceleration.  This needs more thought.

-vlad

Ding

quoted

-vlad

quoted

Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 453d55a..c3a54a6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c

@@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (!(dev->flags & IFF_UP)) {
+	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
 		/* Just copy in the new address */
 		ether_addr_copy(dev->dev_addr, addr);
 	} else {

On 2014/6/7 1:11, Vlad Yasevich wrote:

On 06/05/2014 11:45 PM, Ding Tianhong wrote:

quoted

On 2014/6/5 23:09, Vlad Yasevich wrote:

quoted

On 06/05/2014 08:01 AM, Ding Tianhong wrote:

quoted

If lowerdev supports L2 forwarding offload, no need to set mac address
to uc list and vlan list, so also don't do that when the macvlan mac address
changes.

Same issue as in v1.  Please explain how this actually works, since the
new address never makes it to the hw.

Hi, vlad:

The fwd only support 82599 nic, and when fwd_priv is true, this means the lowerdev support L2 forwarding offload, and will set the macvlan
to the ixgbe_fwd_adapter->netdev, and then it is clear that the lowerdev could know that the the upper device is macvlan and could handle
the skb to the upperdev.

If I miss something, please remind me, thanks.

You've just changed the mac address of the macvlan device, but the
address is not written to the card's RAR registers.
So what allows the card to receive the traffic and place it into the
queue in the first place?

I can see it possibly working if the card is in promiscuous mode.

Also, I think John is right in that if we simply do dev_uc_add()
to add the address to the card, we'll unmap the address from the vmdqs
and lose the acceleration.  This needs more thought.

Yes, I miss it, the new mac address should be set to the lowerdev by ndo_dfwd_add_station(),
otherwise the lowerdev still use the old mac address and the macvlan could not work again,
so I need to rethink this patch and resend, thanks, vlad.

Ding

-vlad

quoted

Ding

quoted

-vlad

quoted

Signed-off-by: Ding Tianhong <redacted>
---
 drivers/net/macvlan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 453d55a..c3a54a6 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c

@@ -515,7 +515,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (!(dev->flags & IFF_UP)) {
+	if (!(dev->flags & IFF_UP) || vlan->fwd_priv) {
 		/* Just copy in the new address */
 		ether_addr_copy(dev->dev_addr, addr);
 	} else {

.