| Message ID | 1401951028-9800-3-git-send-email-dingtianhong@huawei.com |
|---|---|
| State | New |
| Headers | show |
(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 -- 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
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 > -- 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
(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 -- 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
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 > > . > -- 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
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 > -- 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
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 >> > > > . > -- 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
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 >>> >> >> >> . >> > > -- 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
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 >>>> >>> >>> >>> . >>> >> >> > > > . > -- 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
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;
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(+)