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

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

[Ding Tianhong]

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.

Set same mac address to the same netdev is unnecessary, so fix it.

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

Ding Tianhong (4):
  macvlan: don't update the uc and vlan list for L2 forwarding offload
  net: dev: don't set the same mac address for netdev
  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        | 25 +++++++++++++++++++++----
 2 files changed, 27 insertions(+), 6 deletions(-)

-- 
1.8.0

[PATCH net-next 1/4] macvlan: don't update the uc and vlan list for L2 forwarding offload

[Ding Tianhong]

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 <dingtianhong@huawei.com>
---
 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

[PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Ding Tianhong]

Most of netdev just like bond, team, vlan will set the mac address
and propagate to the upperdev or lowerdev regardless the mac address
is  same or not, I could not find that the same mac address could
make affect, so add equal check when set mac address.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 5367bfb..4008a51 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 		return -EINVAL;
 	if (!netif_device_present(dev))
 		return -ENODEV;
+	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
+		return 0;
 	err = ops->ndo_set_mac_address(dev, sa);
 	if (err)
 		return err;
-- 
1.8.0

[PATCH net-next 3/4] net: dev: revert the mac address when notifier failed

[Ding Tianhong]

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 <dingtianhong@huawei.com>
---
 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 4008a51..fc07b8f 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)
@@ -5572,13 +5573,27 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
 		return -ENODEV;
 	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
 		return 0;
+
+	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

[PATCH net-next 4/4] macvlan: don't set the same mac address for non-passthru mode

[Ding Tianhong]

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 <dingtianhong@huawei.com>
---
 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

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Toshiaki Makita]

(2014/06/05 15:50), Ding Tianhong wrote:
> Most of netdev just like bond, team, vlan will set the mac address
> and propagate to the upperdev or lowerdev regardless the mac address
> is  same or not, I could not find that the same mac address could
> make affect, so add equal check when set mac address.
> 
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5367bfb..4008a51 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>  		return -EINVAL;
>  	if (!netif_device_present(dev))
>  		return -ENODEV;
> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
> +		return 0;
>  	err = ops->ndo_set_mac_address(dev, sa);
>  	if (err)
>  		return err;
> 

Bridge uses addr_assign_type to check if bridge_id can be propageted by
bridge ports. If user set mac address, and even if it is the same as
current one, bridge uses the fact that the mac address is set by user.

Although I'm not aware of a driver that needs calling of
ndo_set_mac_address() for the same mac address, this change looks a bit
risky to me.
(For example, old bridge code needed this call because it managed
BR_SET_MAC_ADDR in bridge flags.)

Thanks,
Toshiaki Makita

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Ding Tianhong]

On 2014/6/5 17:09, Toshiaki Makita wrote:
> (2014/06/05 15:50), Ding Tianhong wrote:
>> Most of netdev just like bond, team, vlan will set the mac address
>> and propagate to the upperdev or lowerdev regardless the mac address
>> is  same or not, I could not find that the same mac address could
>> make affect, so add equal check when set mac address.
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>  net/core/dev.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 5367bfb..4008a51 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>  		return -EINVAL;
>>  	if (!netif_device_present(dev))
>>  		return -ENODEV;
>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>> +		return 0;
>>  	err = ops->ndo_set_mac_address(dev, sa);
>>  	if (err)
>>  		return err;
>>
> 
> Bridge uses addr_assign_type to check if bridge_id can be propageted by
> bridge ports. If user set mac address, and even if it is the same as
> current one, bridge uses the fact that the mac address is set by user.
> 

OK

> Although I'm not aware of a driver that needs calling of
> ndo_set_mac_address() for the same mac address, this change looks a bit
> risky to me.
> (For example, old bridge code needed this call because it managed
> BR_SET_MAC_ADDR in bridge flags.)
> 
Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.

Ding 

> Thanks,
> Toshiaki Makita
> 

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Toshiaki Makita]

(2014/06/05 18:50), Ding Tianhong wrote:
> On 2014/6/5 17:09, Toshiaki Makita wrote:
>> (2014/06/05 15:50), Ding Tianhong wrote:
>>> Most of netdev just like bond, team, vlan will set the mac address
>>> and propagate to the upperdev or lowerdev regardless the mac address
>>> is  same or not, I could not find that the same mac address could
>>> make affect, so add equal check when set mac address.
>>>
>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>> ---
>>>  net/core/dev.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 5367bfb..4008a51 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>>  		return -EINVAL;
>>>  	if (!netif_device_present(dev))
>>>  		return -ENODEV;
>>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>> +		return 0;
>>>  	err = ops->ndo_set_mac_address(dev, sa);
>>>  	if (err)
>>>  		return err;
>>>
>>
>> Bridge uses addr_assign_type to check if bridge_id can be propageted by
>> bridge ports. If user set mac address, and even if it is the same as
>> current one, bridge uses the fact that the mac address is set by user.
>>
> 
> OK
> 
>> Although I'm not aware of a driver that needs calling of
>> ndo_set_mac_address() for the same mac address, this change looks a bit
>> risky to me.
>> (For example, old bridge code needed this call because it managed
>> BR_SET_MAC_ADDR in bridge flags.)
>>
> Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
> for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
> I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.

Though I don't know why you think this is safe, looking over some
drivers, br2684_mac_addr() seems to use a logic similar to old bridge's.

Thanks,
Toshiaki Makita

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Ding Tianhong]

On 2014/6/5 18:51, Toshiaki Makita wrote:
> (2014/06/05 18:50), Ding Tianhong wrote:
>> On 2014/6/5 17:09, Toshiaki Makita wrote:
>>> (2014/06/05 15:50), Ding Tianhong wrote:
>>>> Most of netdev just like bond, team, vlan will set the mac address
>>>> and propagate to the upperdev or lowerdev regardless the mac address
>>>> is  same or not, I could not find that the same mac address could
>>>> make affect, so add equal check when set mac address.
>>>>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> ---
>>>>  net/core/dev.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 5367bfb..4008a51 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>>>  		return -EINVAL;
>>>>  	if (!netif_device_present(dev))
>>>>  		return -ENODEV;
>>>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>>> +		return 0;
>>>>  	err = ops->ndo_set_mac_address(dev, sa);
>>>>  	if (err)
>>>>  		return err;
>>>>
>>>
>>> Bridge uses addr_assign_type to check if bridge_id can be propageted by
>>> bridge ports. If user set mac address, and even if it is the same as
>>> current one, bridge uses the fact that the mac address is set by user.
>>>
>>
>> OK
>>
>>> Although I'm not aware of a driver that needs calling of
>>> ndo_set_mac_address() for the same mac address, this change looks a bit
>>> risky to me.
>>> (For example, old bridge code needed this call because it managed
>>> BR_SET_MAC_ADDR in bridge flags.)
>>>
>> Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
>> for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
>> I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.
> 
> Though I don't know why you think this is safe, looking over some
> drivers, br2684_mac_addr() seems to use a logic similar to old bridge's.
> 

Hm, I miss that, this improvement is risky for some drivers, I will remove this patch
and resend the series later. thanks. 

Ding

> Thanks,
> Toshiaki Makita
> 
> .
> 

Re: [PATCH net-next 1/4] macvlan: don't update the uc and vlan list for L2 forwarding offload

[Vlad Yasevich]

On 06/05/2014 02:50 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.
> 

Are you sure about this?  How would the lower dev receive traffic
destined to the new HW address if it is not in the device unicast filter
list?

-vlad

> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  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 {
> 

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Vlad Yasevich]

On 06/05/2014 05:50 AM, Ding Tianhong wrote:
> On 2014/6/5 17:09, Toshiaki Makita wrote:
>> (2014/06/05 15:50), Ding Tianhong wrote:
>>> Most of netdev just like bond, team, vlan will set the mac address
>>> and propagate to the upperdev or lowerdev regardless the mac address
>>> is  same or not, I could not find that the same mac address could
>>> make affect, so add equal check when set mac address.
>>>
>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>> ---
>>>  net/core/dev.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index 5367bfb..4008a51 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>>  		return -EINVAL;
>>>  	if (!netif_device_present(dev))
>>>  		return -ENODEV;
>>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>> +		return 0;
>>>  	err = ops->ndo_set_mac_address(dev, sa);
>>>  	if (err)
>>>  		return err;
>>>
>>
>> Bridge uses addr_assign_type to check if bridge_id can be propageted by
>> bridge ports. If user set mac address, and even if it is the same as
>> current one, bridge uses the fact that the mac address is set by user.
>>
> 
> OK
> 
>> Although I'm not aware of a driver that needs calling of
>> ndo_set_mac_address() for the same mac address, this change looks a bit
>> risky to me.
>> (For example, old bridge code needed this call because it managed
>> BR_SET_MAC_ADDR in bridge flags.)
>>
> Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
> for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
> I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.
> 

Ok so may be a check like:
    /* If the address has already been set by user and it
     * is the same as before, don't do anything.
     */
    if (dev->addr_assign_type == NET_ADDR_SET &&
        ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
            return 0;

This way, if the device does it's own address management (like bridge),
the user can still pin the mac address, but subsequent setting to the
same value don't cause all sorts of propagation.

-vlad

> Ding 
> 
>> Thanks,
>> Toshiaki Makita
>>
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net-next 1/4] macvlan: don't update the uc and vlan list for L2 forwarding offload

[John Fastabend]

On 6/5/2014 6:52 AM, Vlad Yasevich wrote:
> On 06/05/2014 02:50 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.
>>
>
> Are you sure about this?  How would the lower dev receive traffic
> destined to the new HW address if it is not in the device unicast filter
> list?

I don't think the offload path works as it is either though (need to
test). The issue is sync'ing with the lowerdev doesn't give the lowerdev
any information on which fwd_priv to set so the lowerdev sets the normal
path with the address. It won't "break" traffic just receive flows wont
use the direct hardware queues. This will be a bigger issue once we get
support for metering and hardware ACLs.

Agreed though, Ding how does this work? Looks like a step back to me. At
very least can we get the commit message to explain this a bit better.

.John

>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>   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 {
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Ding Tianhong]

On 2014/6/5 22:06, Vlad Yasevich wrote:
> On 06/05/2014 05:50 AM, Ding Tianhong wrote:
>> On 2014/6/5 17:09, Toshiaki Makita wrote:
>>> (2014/06/05 15:50), Ding Tianhong wrote:
>>>> Most of netdev just like bond, team, vlan will set the mac address
>>>> and propagate to the upperdev or lowerdev regardless the mac address
>>>> is  same or not, I could not find that the same mac address could
>>>> make affect, so add equal check when set mac address.
>>>>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> ---
>>>>  net/core/dev.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index 5367bfb..4008a51 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>>>  		return -EINVAL;
>>>>  	if (!netif_device_present(dev))
>>>>  		return -ENODEV;
>>>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>>> +		return 0;
>>>>  	err = ops->ndo_set_mac_address(dev, sa);
>>>>  	if (err)
>>>>  		return err;
>>>>
>>>
>>> Bridge uses addr_assign_type to check if bridge_id can be propageted by
>>> bridge ports. If user set mac address, and even if it is the same as
>>> current one, bridge uses the fact that the mac address is set by user.
>>>
>>
>> OK
>>
>>> Although I'm not aware of a driver that needs calling of
>>> ndo_set_mac_address() for the same mac address, this change looks a bit
>>> risky to me.
>>> (For example, old bridge code needed this call because it managed
>>> BR_SET_MAC_ADDR in bridge flags.)
>>>
>> Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
>> for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
>> I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.
>>
> 
> Ok so may be a check like:
>     /* If the address has already been set by user and it
>      * is the same as before, don't do anything.
>      */
>     if (dev->addr_assign_type == NET_ADDR_SET &&
>         ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>             return 0;
> 
> This way, if the device does it's own address management (like bridge),
> the user can still pin the mac address, but subsequent setting to the
> same value don't cause all sorts of propagation.
> 

Great solution for br, but I think it couldn't fix the problem in br2684_mac_addr() just like Toshiaki said, I am not
sure how many place in the kernel use like this.

Ding 

> -vlad
> 
>> Ding 
>>
>>> Thanks,
>>> Toshiaki Makita
>>>
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> 
> .
> 

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Vlad Yasevich]

On 06/05/2014 11:54 PM, Ding Tianhong wrote:
> On 2014/6/5 22:06, Vlad Yasevich wrote:
>> On 06/05/2014 05:50 AM, Ding Tianhong wrote:
>>> On 2014/6/5 17:09, Toshiaki Makita wrote:
>>>> (2014/06/05 15:50), Ding Tianhong wrote:
>>>>> Most of netdev just like bond, team, vlan will set the mac address
>>>>> and propagate to the upperdev or lowerdev regardless the mac address
>>>>> is  same or not, I could not find that the same mac address could
>>>>> make affect, so add equal check when set mac address.
>>>>>
>>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>>> ---
>>>>>  net/core/dev.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>> index 5367bfb..4008a51 100644
>>>>> --- a/net/core/dev.c
>>>>> +++ b/net/core/dev.c
>>>>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>>>>  		return -EINVAL;
>>>>>  	if (!netif_device_present(dev))
>>>>>  		return -ENODEV;
>>>>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>>>> +		return 0;
>>>>>  	err = ops->ndo_set_mac_address(dev, sa);
>>>>>  	if (err)
>>>>>  		return err;
>>>>>
>>>>
>>>> Bridge uses addr_assign_type to check if bridge_id can be propageted by
>>>> bridge ports. If user set mac address, and even if it is the same as
>>>> current one, bridge uses the fact that the mac address is set by user.
>>>>
>>>
>>> OK
>>>
>>>> Although I'm not aware of a driver that needs calling of
>>>> ndo_set_mac_address() for the same mac address, this change looks a bit
>>>> risky to me.
>>>> (For example, old bridge code needed this call because it managed
>>>> BR_SET_MAC_ADDR in bridge flags.)
>>>>
>>> Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
>>> for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
>>> I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.
>>>
>>
>> Ok so may be a check like:
>>     /* If the address has already been set by user and it
>>      * is the same as before, don't do anything.
>>      */
>>     if (dev->addr_assign_type == NET_ADDR_SET &&
>>         ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>             return 0;
>>
>> This way, if the device does it's own address management (like bridge),
>> the user can still pin the mac address, but subsequent setting to the
>> same value don't cause all sorts of propagation.
>>
> 
> Great solution for br, but I think it couldn't fix the problem in br2684_mac_addr() just like Toshiaki said, I am not
> sure how many place in the kernel use like this.
> 

Sure it would.  The first time through, we always call
ndo_set_mac_address(), so br2684_mac_addr() would get called.

The second time through, we check to see if the user is setting
the same address and skip it if it's the same.  As far as br2684
is concerned, the address is still the same and the flag is still
set.

-vlad

> Ding 
> 
>> -vlad
>>
>>> Ding 
>>>
>>>> Thanks,
>>>> Toshiaki Makita
>>>>
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>>
>> .
>>
> 
> 

Re: [PATCH net-next 2/4] net: dev: don't set the same mac address for netdev

[Ding Tianhong]

On 2014/6/6 22:09, Vlad Yasevich wrote:
> On 06/05/2014 11:54 PM, Ding Tianhong wrote:
>> On 2014/6/5 22:06, Vlad Yasevich wrote:
>>> On 06/05/2014 05:50 AM, Ding Tianhong wrote:
>>>> On 2014/6/5 17:09, Toshiaki Makita wrote:
>>>>> (2014/06/05 15:50), Ding Tianhong wrote:
>>>>>> Most of netdev just like bond, team, vlan will set the mac address
>>>>>> and propagate to the upperdev or lowerdev regardless the mac address
>>>>>> is  same or not, I could not find that the same mac address could
>>>>>> make affect, so add equal check when set mac address.
>>>>>>
>>>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>>>> ---
>>>>>>  net/core/dev.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>>>> index 5367bfb..4008a51 100644
>>>>>> --- a/net/core/dev.c
>>>>>> +++ b/net/core/dev.c
>>>>>> @@ -5570,6 +5570,8 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa)
>>>>>>  		return -EINVAL;
>>>>>>  	if (!netif_device_present(dev))
>>>>>>  		return -ENODEV;
>>>>>> +	if (ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>>>>> +		return 0;
>>>>>>  	err = ops->ndo_set_mac_address(dev, sa);
>>>>>>  	if (err)
>>>>>>  		return err;
>>>>>>
>>>>>
>>>>> Bridge uses addr_assign_type to check if bridge_id can be propageted by
>>>>> bridge ports. If user set mac address, and even if it is the same as
>>>>> current one, bridge uses the fact that the mac address is set by user.
>>>>>
>>>>
>>>> OK
>>>>
>>>>> Although I'm not aware of a driver that needs calling of
>>>>> ndo_set_mac_address() for the same mac address, this change looks a bit
>>>>> risky to me.
>>>>> (For example, old bridge code needed this call because it managed
>>>>> BR_SET_MAC_ADDR in bridge flags.)
>>>>>
>>>> Except the old bridge, I still don't think any other driver need to call ndo_set_mac_address()
>>>> for the same mac address, if the dev_set_mac_address() don't do anything for the same address,
>>>> I think some drivers should ignore the same mac address themselves just like bonding, macvlan, vlan and so on.
>>>>
>>>
>>> Ok so may be a check like:
>>>     /* If the address has already been set by user and it
>>>      * is the same as before, don't do anything.
>>>      */
>>>     if (dev->addr_assign_type == NET_ADDR_SET &&
>>>         ether_addr_equal_64bits(dev->dev_addr, sa->sa_data))
>>>             return 0;
>>>
>>> This way, if the device does it's own address management (like bridge),
>>> the user can still pin the mac address, but subsequent setting to the
>>> same value don't cause all sorts of propagation.
>>>
>>
>> Great solution for br, but I think it couldn't fix the problem in br2684_mac_addr() just like Toshiaki said, I am not
>> sure how many place in the kernel use like this.
>>
> 
> Sure it would.  The first time through, we always call
> ndo_set_mac_address(), so br2684_mac_addr() would get called.
> 
> The second time through, we check to see if the user is setting
> the same address and skip it if it's the same.  As far as br2684
> is concerned, the address is still the same and the flag is still
> set.
> 
> -vlad
> 

Yep, you are right, the NET_ADDR_SET could show that the dev has calling ndo_set_mac_addreee() once
and decide whether to set a same mac address again.

I will resend this patch, thanks for your advise.

Ding

>> Ding 
>>
>>> -vlad
>>>
>>>> Ding 
>>>>
>>>>> Thanks,
>>>>> Toshiaki Makita
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>> .
>>>
>>
>>
> 
> 
> .
>