[PATCH net 0/4] macvlan: Fix some issues with changing mac addresses

[PATCH net 0/4] macvlan: Fix some issues with changing mac addresses

[Vladislav Yasevich]

There are some issues in macvlan wrt to changing it's mac address.
* An error is returned in the specified address is the same as an already
  assigned address.
* In passthru mode, the mac address of the macvlan device doesn't change.
* After changing the mac address of a passthru macvlan and then removing it,
  the mac address of the physical device remains changed.

This patch series attempts to resolve these issues.

Thanks
-vlad

Vladislav Yasevich (4):
  macvlan: Do not return error when setting the same mac address
  macvlan: Fix passthru macvlan mac address inheritance
  macvlan: convert port passthru to flags.
  macvlan: Let passthru macvlan correctly restore lower mac address

 drivers/net/macvlan.c | 85 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 71 insertions(+), 14 deletions(-)

-- 
2.9.4

[PATCH net 1/4] macvlan: Do not return error when setting the same mac address

[Vladislav Yasevich]

The user currently gets an EBUSY error when attempting to set
the mac address on a macvlan device to the same value.

This should really be a no-op as nothing changes.  Catch
the condition and return early.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 67bf7eb..de214fb 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -703,6 +703,10 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
+	/* If the addresses are the same, this is a no-op */
+	if (ether_addr_equal(dev->dev_addr, addr->sa_data))
+		return 0;
+
 	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
 		dev_set_mac_address(vlan->lowerdev, addr);
 		return 0;
-- 
2.9.4

[PATCH net 2/4] macvlan: Fix passthru macvlan mac address inheritance

[Vladislav Yasevich]

When a lower device of the passthru macvlan changes it's address,
passthru macvlan is supposed to change it's own address as well.
However, that doesn't happen correctly because the check in
macvlan_addr_busy() will catch the fact that the lower level
(port) mac address is the same as the address we are trying to
assign to the macvlan, and return an error.  As a result,
the address of the passthru macvlan device is never changed.

The same thing happens when the user attempts to change the
mac address of the passthru macvlan.

The simple solution appears to be to not check against
the lower device in case of passthru macvlan device, since
the 2 addresses are _supposed_ to be the same.

Fixes: e289fd281 (macvlan: fix the problem when mac address changes for passthru mode)
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index de214fb..a735a64 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -185,7 +185,8 @@ static bool macvlan_addr_busy(const struct macvlan_port *port,
 	 * currently in use by the underlying device or
 	 * another macvlan.
 	 */
-	if (ether_addr_equal_64bits(port->dev->dev_addr, addr))
+	if (!port->passthru &&
+	    ether_addr_equal_64bits(port->dev->dev_addr, addr))
 		return true;
 
 	if (macvlan_hash_lookup(port, addr))
-- 
2.9.4

[PATCH net 3/4] macvlan: convert port passthru to flags.

[Vladislav Yasevich]

Convert the port passthru boolean into flags with access functions.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index a735a64..eb956ff 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -39,13 +39,15 @@
 #define MACVLAN_HASH_SIZE	(1<<MACVLAN_HASH_BITS)
 #define MACVLAN_BC_QUEUE_LEN	1000
 
+#define MACVLAN_F_PASSTHRU	1
+
 struct macvlan_port {
 	struct net_device	*dev;
 	struct hlist_head	vlan_hash[MACVLAN_HASH_SIZE];
 	struct list_head	vlans;
 	struct sk_buff_head	bc_queue;
 	struct work_struct	bc_work;
-	bool 			passthru;
+	u32			flags;
 	int			count;
 	struct hlist_head	vlan_source_hash[MACVLAN_HASH_SIZE];
 	DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ);
@@ -66,6 +68,16 @@ struct macvlan_skb_cb {
 
 static void macvlan_port_destroy(struct net_device *dev);
 
+static inline bool macvlan_passthru(const struct macvlan_port *port)
+{
+	return port->flags & MACVLAN_F_PASSTHRU;
+}
+
+static inline void macvlan_set_passthru(struct macvlan_port *port)
+{
+	port->flags |= MACVLAN_F_PASSTHRU;
+}
+
 /* Hash Ethernet address */
 static u32 macvlan_eth_hash(const unsigned char *addr)
 {
@@ -185,7 +197,7 @@ static bool macvlan_addr_busy(const struct macvlan_port *port,
 	 * currently in use by the underlying device or
 	 * another macvlan.
 	 */
-	if (!port->passthru &&
+	if (!macvlan_passthru(port) &&
 	    ether_addr_equal_64bits(port->dev->dev_addr, addr))
 		return true;
 
@@ -446,7 +458,7 @@ static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 	}
 
 	macvlan_forward_source(skb, port, eth->h_source);
-	if (port->passthru)
+	if (macvlan_passthru(port))
 		vlan = list_first_or_null_rcu(&port->vlans,
 					      struct macvlan_dev, list);
 	else
@@ -575,7 +587,7 @@ static int macvlan_open(struct net_device *dev)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (vlan->port->passthru) {
+	if (macvlan_passthru(vlan->port)) {
 		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC)) {
 			err = dev_set_promiscuity(lowerdev, 1);
 			if (err < 0)
@@ -650,7 +662,7 @@ static int macvlan_stop(struct net_device *dev)
 	dev_uc_unsync(lowerdev, dev);
 	dev_mc_unsync(lowerdev, dev);
 
-	if (vlan->port->passthru) {
+	if (macvlan_passthru(vlan->port)) {
 		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC))
 			dev_set_promiscuity(lowerdev, -1);
 		goto hash_del;
@@ -683,7 +695,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 		if (macvlan_addr_busy(vlan->port, addr))
 			return -EBUSY;
 
-		if (!vlan->port->passthru) {
+		if (!macvlan_passthru(vlan->port)) {
 			err = dev_uc_add(lowerdev, addr);
 			if (err)
 				return err;
@@ -933,7 +945,7 @@ static int macvlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	/* Support unicast filter only on passthru devices.
 	 * Multicast filter should be allowed on all devices.
 	 */
-	if (!vlan->port->passthru && is_unicast_ether_addr(addr))
+	if (!macvlan_passthru(vlan->port) && is_unicast_ether_addr(addr))
 		return -EOPNOTSUPP;
 
 	if (flags & NLM_F_REPLACE)
@@ -957,7 +969,7 @@ static int macvlan_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 	/* Support unicast filter only on passthru devices.
 	 * Multicast filter should be allowed on all devices.
 	 */
-	if (!vlan->port->passthru && is_unicast_ether_addr(addr))
+	if (!macvlan_passthru(vlan->port) && is_unicast_ether_addr(addr))
 		return -EOPNOTSUPP;
 
 	if (is_unicast_ether_addr(addr))
@@ -1125,7 +1137,6 @@ static int macvlan_port_create(struct net_device *dev)
 	if (port == NULL)
 		return -ENOMEM;
 
-	port->passthru = false;
 	port->dev = dev;
 	INIT_LIST_HEAD(&port->vlans);
 	for (i = 0; i < MACVLAN_HASH_SIZE; i++)
@@ -1331,7 +1342,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	port = macvlan_port_get_rtnl(lowerdev);
 
 	/* Only 1 macvlan device can be created in passthru mode */
-	if (port->passthru) {
+	if (macvlan_passthru(port)) {
 		/* The macvlan port must be not created this time,
 		 * still goto destroy_macvlan_port for readability.
 		 */
@@ -1357,7 +1368,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 			err = -EINVAL;
 			goto destroy_macvlan_port;
 		}
-		port->passthru = true;
+		macvlan_set_passthru(port);
 		eth_hw_addr_inherit(dev, lowerdev);
 	}
 
@@ -1439,7 +1450,7 @@ static int macvlan_changelink(struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_FLAGS]) {
 		__u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
 		bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
-		if (vlan->port->passthru && promisc) {
+		if (macvlan_passthru(vlan->port) && promisc) {
 			int err;
 
 			if (flags & MACVLAN_FLAG_NOPROMISC)
@@ -1602,7 +1613,7 @@ static int macvlan_device_event(struct notifier_block *unused,
 		}
 		break;
 	case NETDEV_CHANGEADDR:
-		if (!port->passthru)
+		if (!macvlan_passthru(port))
 			return NOTIFY_DONE;
 
 		vlan = list_first_entry_or_null(&port->vlans,
-- 
2.9.4

[PATCH net 4/4] macvlan: Let passthru macvlan correctly restore lower mac address

[Vladislav Yasevich]

Passthru macvlans directly change the mac address of the lower
level device.  That's OK, but after the macvlan is deleted,
the lower device is left with changed address and one needs to
reboot to bring back the origina HW addresses.

This scenario is actually quite common with passthru macvtap devices.

This patch attempts to solve this, by storing the mac address
of the lower device in macvlan_port structure and keeping track of
it through the changes.

After this patch, any changes to the lower device mac address
done trough the macvlan device, will be reverted back.  Any
changes done directly to the lower device mac address will be kept.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index eb956ff..c551165 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -40,6 +40,7 @@
 #define MACVLAN_BC_QUEUE_LEN	1000
 
 #define MACVLAN_F_PASSTHRU	1
+#define MACVLAN_F_ADDRCHANGE	2
 
 struct macvlan_port {
 	struct net_device	*dev;
@@ -51,6 +52,7 @@ struct macvlan_port {
 	int			count;
 	struct hlist_head	vlan_source_hash[MACVLAN_HASH_SIZE];
 	DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ);
+	unsigned char           perm_addr[ETH_ALEN];
 };
 
 struct macvlan_source_entry {
@@ -78,6 +80,21 @@ static inline void macvlan_set_passthru(struct macvlan_port *port)
 	port->flags |= MACVLAN_F_PASSTHRU;
 }
 
+static inline bool macvlan_addr_change(const struct macvlan_port *port)
+{
+	return port->flags & MACVLAN_F_ADDRCHANGE;
+}
+
+static inline void macvlan_set_addr_change(struct macvlan_port *port)
+{
+	port->flags |= MACVLAN_F_ADDRCHANGE;
+}
+
+static inline void macvlan_clear_addr_change(struct macvlan_port *port)
+{
+	port->flags &= ~MACVLAN_F_ADDRCHANGE;
+}
+
 /* Hash Ethernet address */
 static u32 macvlan_eth_hash(const unsigned char *addr)
 {
@@ -193,11 +210,11 @@ static void macvlan_hash_change_addr(struct macvlan_dev *vlan,
 static bool macvlan_addr_busy(const struct macvlan_port *port,
 			      const unsigned char *addr)
 {
-	/* Test to see if the specified multicast address is
+	/* Test to see if the specified address is
 	 * currently in use by the underlying device or
 	 * another macvlan.
 	 */
-	if (!macvlan_passthru(port) &&
+	if (!macvlan_passthru(port) && !macvlan_addr_change(port) &&
 	    ether_addr_equal_64bits(port->dev->dev_addr, addr))
 		return true;
 
@@ -685,6 +702,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 	struct net_device *lowerdev = vlan->lowerdev;
+	struct macvlan_port *port = vlan->port;
 	int err;
 
 	if (!(dev->flags & IFF_UP)) {
@@ -695,7 +713,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 		if (macvlan_addr_busy(vlan->port, addr))
 			return -EBUSY;
 
-		if (!macvlan_passthru(vlan->port)) {
+		if (!macvlan_passthru(port)) {
 			err = dev_uc_add(lowerdev, addr);
 			if (err)
 				return err;
@@ -705,6 +723,15 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
 
 		macvlan_hash_change_addr(vlan, addr);
 	}
+	if (macvlan_passthru(port) && !macvlan_addr_change(port)) {
+		/* Since addr_change isn't set, we are here due to lower
+		 * device change.  Save the lower-dev address so we can
+		 * restore it later.
+		 */
+		ether_addr_copy(vlan->port->perm_addr,
+				dev->dev_addr);
+	}
+	macvlan_clear_addr_change(port);
 	return 0;
 }
 
@@ -721,6 +748,7 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
 		return 0;
 
 	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
+		macvlan_set_addr_change(vlan->port);
 		dev_set_mac_address(vlan->lowerdev, addr);
 		return 0;
 	}
@@ -1138,6 +1166,7 @@ static int macvlan_port_create(struct net_device *dev)
 		return -ENOMEM;
 
 	port->dev = dev;
+	ether_addr_copy(port->perm_addr, dev->dev_addr);
 	INIT_LIST_HEAD(&port->vlans);
 	for (i = 0; i < MACVLAN_HASH_SIZE; i++)
 		INIT_HLIST_HEAD(&port->vlan_hash[i]);
@@ -1177,6 +1206,18 @@ static void macvlan_port_destroy(struct net_device *dev)
 		kfree_skb(skb);
 	}
 
+	/* If the lower device address has been changed by passthru
+	 * macvlan, put it back.
+	 */
+	if (macvlan_passthru(port) &&
+	    !ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
+		struct sockaddr sa;
+
+		sa.sa_family = port->dev->type;
+		memcpy(&sa.sa_data, port->perm_addr, port->dev->addr_len);
+		dev_set_mac_address(port->dev, &sa);
+	}
+
 	kfree(port);
 }
 
-- 
2.9.4

Re: [PATCH net 4/4] macvlan: Let passthru macvlan correctly restore lower mac address

[Girish Moodalbail]

Sorry, it took sometime to wrap around this patch series since they all change 
one file and at times the same function :).


On 6/16/17 6:36 AM, Vladislav Yasevich wrote:
> Passthru macvlans directly change the mac address of the lower
> level device.  That's OK, but after the macvlan is deleted,
> the lower device is left with changed address and one needs to
> reboot to bring back the origina HW addresses.

s/origina/original/

>
> This scenario is actually quite common with passthru macvtap devices.
>
> This patch attempts to solve this, by storing the mac address
> of the lower device in macvlan_port structure and keeping track of
> it through the changes.
>
> After this patch, any changes to the lower device mac address
> done trough the macvlan device, will be reverted back.  Any
> changes done directly to the lower device mac address will be kept.
>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  drivers/net/macvlan.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index eb956ff..c551165 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -40,6 +40,7 @@
>  #define MACVLAN_BC_QUEUE_LEN	1000
>
>  #define MACVLAN_F_PASSTHRU	1
> +#define MACVLAN_F_ADDRCHANGE	2
>
>  struct macvlan_port {
>  	struct net_device	*dev;
> @@ -51,6 +52,7 @@ struct macvlan_port {
>  	int			count;
>  	struct hlist_head	vlan_source_hash[MACVLAN_HASH_SIZE];
>  	DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ);
> +	unsigned char           perm_addr[ETH_ALEN];
>  };
>
>  struct macvlan_source_entry {
> @@ -78,6 +80,21 @@ static inline void macvlan_set_passthru(struct macvlan_port *port)
>  	port->flags |= MACVLAN_F_PASSTHRU;
>  }
>
> +static inline bool macvlan_addr_change(const struct macvlan_port *port)
> +{
> +	return port->flags & MACVLAN_F_ADDRCHANGE;
> +}
> +
> +static inline void macvlan_set_addr_change(struct macvlan_port *port)
> +{
> +	port->flags |= MACVLAN_F_ADDRCHANGE;
> +}
> +
> +static inline void macvlan_clear_addr_change(struct macvlan_port *port)
> +{
> +	port->flags &= ~MACVLAN_F_ADDRCHANGE;
> +}
> +
>  /* Hash Ethernet address */
>  static u32 macvlan_eth_hash(const unsigned char *addr)
>  {
> @@ -193,11 +210,11 @@ static void macvlan_hash_change_addr(struct macvlan_dev *vlan,
>  static bool macvlan_addr_busy(const struct macvlan_port *port,
>  			      const unsigned char *addr)
>  {
> -	/* Test to see if the specified multicast address is
> +	/* Test to see if the specified address is
>  	 * currently in use by the underlying device or
>  	 * another macvlan.
>  	 */
> -	if (!macvlan_passthru(port) &&
> +	if (!macvlan_passthru(port) && !macvlan_addr_change(port) &&
>  	    ether_addr_equal_64bits(port->dev->dev_addr, addr))
>  		return true;
>
> @@ -685,6 +702,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>  {
>  	struct macvlan_dev *vlan = netdev_priv(dev);
>  	struct net_device *lowerdev = vlan->lowerdev;
> +	struct macvlan_port *port = vlan->port;
>  	int err;
>
>  	if (!(dev->flags & IFF_UP)) {
> @@ -695,7 +713,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>  		if (macvlan_addr_busy(vlan->port, addr))
>  			return -EBUSY;
>
> -		if (!macvlan_passthru(vlan->port)) {
> +		if (!macvlan_passthru(port)) {
>  			err = dev_uc_add(lowerdev, addr);
>  			if (err)
>  				return err;
> @@ -705,6 +723,15 @@ static int macvlan_sync_address(struct net_device *dev, unsigned char *addr)
>
>  		macvlan_hash_change_addr(vlan, addr);
>  	}
> +	if (macvlan_passthru(port) && !macvlan_addr_change(port)) {
> +		/* Since addr_change isn't set, we are here due to lower
> +		 * device change.  Save the lower-dev address so we can
> +		 * restore it later.
> +		 */
> +		ether_addr_copy(vlan->port->perm_addr,
> +				dev->dev_addr);

Did you meant to copy `addr' here? Since dev->dev_addr is that of the macvlan 
device whilst `addr' is from the lower parent device.


Thanks,
~Girish

Re: [PATCH net 4/4] macvlan: Let passthru macvlan correctly restore lower mac address

[Vlad Yasevich]

On 06/17/2017 12:35 AM, Girish Moodalbail wrote:
> Sorry, it took sometime to wrap around this patch series since they all change one file
> and at times the same function :).
> 
> 
> On 6/16/17 6:36 AM, Vladislav Yasevich wrote:
>> Passthru macvlans directly change the mac address of the lower
>> level device.  That's OK, but after the macvlan is deleted,
>> the lower device is left with changed address and one needs to
>> reboot to bring back the origina HW addresses.
> 
> s/origina/original/
> 
>>
>> This scenario is actually quite common with passthru macvtap devices.
>>
>> This patch attempts to solve this, by storing the mac address
>> of the lower device in macvlan_port structure and keeping track of
>> it through the changes.
>>
>> After this patch, any changes to the lower device mac address
>> done trough the macvlan device, will be reverted back.  Any
>> changes done directly to the lower device mac address will be kept.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  drivers/net/macvlan.c | 47 ++++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
>> index eb956ff..c551165 100644
>> --- a/drivers/net/macvlan.c
>> +++ b/drivers/net/macvlan.c
>> @@ -40,6 +40,7 @@
>>  #define MACVLAN_BC_QUEUE_LEN    1000
>>
>>  #define MACVLAN_F_PASSTHRU    1
>> +#define MACVLAN_F_ADDRCHANGE    2
>>
>>  struct macvlan_port {
>>      struct net_device    *dev;
>> @@ -51,6 +52,7 @@ struct macvlan_port {
>>      int            count;
>>      struct hlist_head    vlan_source_hash[MACVLAN_HASH_SIZE];
>>      DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ);
>> +    unsigned char           perm_addr[ETH_ALEN];
>>  };
>>
>>  struct macvlan_source_entry {
>> @@ -78,6 +80,21 @@ static inline void macvlan_set_passthru(struct macvlan_port *port)
>>      port->flags |= MACVLAN_F_PASSTHRU;
>>  }
>>
>> +static inline bool macvlan_addr_change(const struct macvlan_port *port)
>> +{
>> +    return port->flags & MACVLAN_F_ADDRCHANGE;
>> +}
>> +
>> +static inline void macvlan_set_addr_change(struct macvlan_port *port)
>> +{
>> +    port->flags |= MACVLAN_F_ADDRCHANGE;
>> +}
>> +
>> +static inline void macvlan_clear_addr_change(struct macvlan_port *port)
>> +{
>> +    port->flags &= ~MACVLAN_F_ADDRCHANGE;
>> +}
>> +
>>  /* Hash Ethernet address */
>>  static u32 macvlan_eth_hash(const unsigned char *addr)
>>  {
>> @@ -193,11 +210,11 @@ static void macvlan_hash_change_addr(struct macvlan_dev *vlan,
>>  static bool macvlan_addr_busy(const struct macvlan_port *port,
>>                    const unsigned char *addr)
>>  {
>> -    /* Test to see if the specified multicast address is
>> +    /* Test to see if the specified address is
>>       * currently in use by the underlying device or
>>       * another macvlan.
>>       */
>> -    if (!macvlan_passthru(port) &&
>> +    if (!macvlan_passthru(port) && !macvlan_addr_change(port) &&
>>          ether_addr_equal_64bits(port->dev->dev_addr, addr))
>>          return true;
>>
>> @@ -685,6 +702,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned
>> char *addr)
>>  {
>>      struct macvlan_dev *vlan = netdev_priv(dev);
>>      struct net_device *lowerdev = vlan->lowerdev;
>> +    struct macvlan_port *port = vlan->port;
>>      int err;
>>
>>      if (!(dev->flags & IFF_UP)) {
>> @@ -695,7 +713,7 @@ static int macvlan_sync_address(struct net_device *dev, unsigned
>> char *addr)
>>          if (macvlan_addr_busy(vlan->port, addr))
>>              return -EBUSY;
>>
>> -        if (!macvlan_passthru(vlan->port)) {
>> +        if (!macvlan_passthru(port)) {
>>              err = dev_uc_add(lowerdev, addr);
>>              if (err)
>>                  return err;
>> @@ -705,6 +723,15 @@ static int macvlan_sync_address(struct net_device *dev, unsigned
>> char *addr)
>>
>>          macvlan_hash_change_addr(vlan, addr);
>>      }
>> +    if (macvlan_passthru(port) && !macvlan_addr_change(port)) {
>> +        /* Since addr_change isn't set, we are here due to lower
>> +         * device change.  Save the lower-dev address so we can
>> +         * restore it later.
>> +         */
>> +        ether_addr_copy(vlan->port->perm_addr,
>> +                dev->dev_addr);
> 
> Did you meant to copy `addr' here? Since dev->dev_addr is that of the macvlan device
> whilst `addr' is from the lower parent device.
> 

At this point, it doesn't really matter since dev_addr has already been set in
hash_change_addr().  However, I see your point, and the intent would be clarified
if I used lower_dev->addr.

Thanks
-vlad
> 
> Thanks,
> ~Girish
> 
> 

Re: [PATCH net 0/4] macvlan: Fix some issues with changing mac addresses

[Girish Moodalbail]

On 6/16/17 6:36 AM, Vladislav Yasevich wrote:
> There are some issues in macvlan wrt to changing it's mac address.
> * An error is returned in the specified address is the same as an already
>   assigned address.
> * In passthru mode, the mac address of the macvlan device doesn't change.
> * After changing the mac address of a passthru macvlan and then removing it,
>   the mac address of the physical device remains changed.
>
> This patch series attempts to resolve these issues.
>
> Thanks
> -vlad
>
> Vladislav Yasevich (4):
>   macvlan: Do not return error when setting the same mac address
>   macvlan: Fix passthru macvlan mac address inheritance
>   macvlan: convert port passthru to flags.

Above 3 patches looks good to me, so

Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>


>   macvlan: Let passthru macvlan correctly restore lower mac address

However, I have few questions/comments on the above patch.

thanks,
~Girish

>
>  drivers/net/macvlan.c | 85 ++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 71 insertions(+), 14 deletions(-)
>