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

[Ding Tianhong <dingtianhong@huawei.com>]

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
>>>>
>>>
>>>
>>> .
>>>
>>
>>
> 
> 
> .
>