* [PATCH] net/bonding: fix propagation of user-specified bond MAC @ 2015-06-05 22:24 Jarod Wilson 2015-06-06 13:29 ` Nikolay Aleksandrov 0 siblings, 1 reply; 3+ messages in thread From: Jarod Wilson @ 2015-06-05 22:24 UTC (permalink / raw) To: linux-kernel Cc: Jarod Wilson, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, open list:BONDING DRIVER Its possible for users to specify their own MAC address for a bonded link, and this used to work, until sometime in 2013... First, commit 409cc1f8a changed a condition to set the bond's mac to a slave device's, dropping the is_zero_ether_addr() check in favor of using bond->dev_addr_from_first. Next, commit 6c8c4e4c2 added a bond->slave_cnt == 0 condition. Then, commit 97a1e6396 removed dev_addr_from_first and keyed off of bond->dev->addr_assign_type. The other contitional in the check to call bond_set_dev_addr() has gone through some permutations, finally landing at the following check: if (!bond_has_slaves(bond) && bond->dev->addr_assign_type == NET_ADDR_RANDOM) bond_set_dev_addr(bond->dev, slave_dev); When the bond is initially brought up, with no active slaves, it gets assigned a random address, and addr_assign_type is set to NET_ADDR_RANDOM. Next up though, the user can provide their own MAC, which ultimately calls bond_set_mac_address(). However, because addr_assign_type isn't touched, the above conditions are still met, and the slave's MAC overwrites the user-provided MAC. The simple fix is to set addr_assign_type = NET_ADDR_SET at the tail end of bond_set_mac_address() doing its thing, and user-specified MAC addresses no longer get overwritten. Nb: this is slightly tricky to test on current Fedora, as nmcli seems to be braindead when it comes to setting a MAC address for a bond. I can do a "nmcli con edit bond0", set ethernet.mac-address "xx:yy:zz:aa:bb:cc", but it doesn't ever seem to do anything, and it doesn't persist to the next boot. Manual tinkering was required to verify the issue and the fix using ip link set commands. CC: Jay Vosburgh <j.vosburgh@gmail.com> CC: Veaceslav Falico <vfalico@gmail.com> CC: Andy Gospodarek <gospo@cumulusnetworks.com> CC: netdev@vger.kernel.org (open list:BONDING DRIVER) Signed-off-by: Jarod Wilson <jarod@redhat.com> --- drivers/net/bonding/bond_main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 19eb990..2aa5b5f 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -3544,6 +3544,7 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr) /* success */ memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len); + bond_dev->addr_assign_type = NET_ADDR_SET; return 0; unwind: -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net/bonding: fix propagation of user-specified bond MAC 2015-06-05 22:24 [PATCH] net/bonding: fix propagation of user-specified bond MAC Jarod Wilson @ 2015-06-06 13:29 ` Nikolay Aleksandrov [not found] ` <55738E41.1020702@redhat.com> 0 siblings, 1 reply; 3+ messages in thread From: Nikolay Aleksandrov @ 2015-06-06 13:29 UTC (permalink / raw) To: Jarod Wilson Cc: linux-kernel, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, open list:BONDING DRIVER On Sat, Jun 6, 2015 at 12:24 AM, Jarod Wilson <jarod@redhat.com> wrote: > Its possible for users to specify their own MAC address for a bonded link, > and this used to work, until sometime in 2013... > > First, commit 409cc1f8a changed a condition to set the bond's mac to a > slave device's, dropping the is_zero_ether_addr() check in favor of using > bond->dev_addr_from_first. > > Next, commit 6c8c4e4c2 added a bond->slave_cnt == 0 condition. > > Then, commit 97a1e6396 removed dev_addr_from_first and keyed off of > bond->dev->addr_assign_type. > > The other contitional in the check to call bond_set_dev_addr() has gone > through some permutations, finally landing at the following check: > > if (!bond_has_slaves(bond) && > bond->dev->addr_assign_type == NET_ADDR_RANDOM) > bond_set_dev_addr(bond->dev, slave_dev); > > When the bond is initially brought up, with no active slaves, it gets > assigned a random address, and addr_assign_type is set to NET_ADDR_RANDOM. > Next up though, the user can provide their own MAC, which ultimately calls > bond_set_mac_address(). However, because addr_assign_type isn't touched, > the above conditions are still met, and the slave's MAC overwrites the > user-provided MAC. > > The simple fix is to set addr_assign_type = NET_ADDR_SET at the tail end > of bond_set_mac_address() doing its thing, and user-specified MAC > addresses no longer get overwritten. > > Nb: this is slightly tricky to test on current Fedora, as nmcli seems to > be braindead when it comes to setting a MAC address for a bond. I can do a > "nmcli con edit bond0", set ethernet.mac-address "xx:yy:zz:aa:bb:cc", but > it doesn't ever seem to do anything, and it doesn't persist to the next > boot. Manual tinkering was required to verify the issue and the fix using > ip link set commands. > > CC: Jay Vosburgh <j.vosburgh@gmail.com> > CC: Veaceslav Falico <vfalico@gmail.com> > CC: Andy Gospodarek <gospo@cumulusnetworks.com> > CC: netdev@vger.kernel.org (open list:BONDING DRIVER) > Signed-off-by: Jarod Wilson <jarod@redhat.com> > --- Hi Jarod, When I did 97a1e6396, I tested all of these cases successfully and they still work. in net/core/dev.c, dev_set_mac_address() we have: dev->addr_assign_type = NET_ADDR_SET; So it's actually changed when the user sets the mac and you don't have to do it in bond_set_mac_address(). Just to confirm, I tried this just now: # modprobe bonding # ip l sh bond0 9: bond0: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default link/ether d2:62:c7:90:93:b9 brd ff:ff:ff:ff:ff:ff # ip l set dev bond0 address 00:11:22:33:44:55 # ip l sh bond0 9: bond0: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff # ifenslave bond0 enp6s0 # ip l sh bond0 9: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff The user-specified mac address is kept. Cheers, Nik ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <55738E41.1020702@redhat.com>]
[parent not found: <5575E828.9060900@redhat.com>]
* Re: [PATCH] net/bonding: fix propagation of user-specified bond MAC [not found] ` <5575E828.9060900@redhat.com> @ 2015-06-09 12:49 ` Nikolay Aleksandrov 0 siblings, 0 replies; 3+ messages in thread From: Nikolay Aleksandrov @ 2015-06-09 12:49 UTC (permalink / raw) To: Jarod Wilson Cc: linux-kernel, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, open list:BONDING DRIVER On Mon, Jun 8, 2015 at 9:08 PM, Jarod Wilson <jarod@redhat.com> wrote: > On 6/6/2015 8:20 PM, Jarod Wilson wrote: >> >> On 6/6/2015 9:29 AM, Nikolay Aleksandrov wrote: >>> >>> On Sat, Jun 6, 2015 at 12:24 AM, Jarod Wilson <jarod@redhat.com> wrote: >>>> >>>> Its possible for users to specify their own MAC address for a bonded >>>> link, >>>> and this used to work, until sometime in 2013... >>>> >>>> First, commit 409cc1f8a changed a condition to set the bond's mac to a >>>> slave device's, dropping the is_zero_ether_addr() check in favor of >>>> using >>>> bond->dev_addr_from_first. >>>> >>>> Next, commit 6c8c4e4c2 added a bond->slave_cnt == 0 condition. >>>> >>>> Then, commit 97a1e6396 removed dev_addr_from_first and keyed off of >>>> bond->dev->addr_assign_type. >>>> >>>> The other contitional in the check to call bond_set_dev_addr() has gone >>>> through some permutations, finally landing at the following check: >>>> >>>> if (!bond_has_slaves(bond) && >>>> bond->dev->addr_assign_type == NET_ADDR_RANDOM) >>>> bond_set_dev_addr(bond->dev, slave_dev); >>>> >>>> When the bond is initially brought up, with no active slaves, it gets >>>> assigned a random address, and addr_assign_type is set to >>>> NET_ADDR_RANDOM. >>>> Next up though, the user can provide their own MAC, which ultimately >>>> calls >>>> bond_set_mac_address(). However, because addr_assign_type isn't touched, >>>> the above conditions are still met, and the slave's MAC overwrites the >>>> user-provided MAC. >>>> >>>> The simple fix is to set addr_assign_type = NET_ADDR_SET at the tail end >>>> of bond_set_mac_address() doing its thing, and user-specified MAC >>>> addresses no longer get overwritten. >>>> >>>> Nb: this is slightly tricky to test on current Fedora, as nmcli seems to >>>> be braindead when it comes to setting a MAC address for a bond. I can >>>> do a >>>> "nmcli con edit bond0", set ethernet.mac-address "xx:yy:zz:aa:bb:cc", >>>> but >>>> it doesn't ever seem to do anything, and it doesn't persist to the next >>>> boot. Manual tinkering was required to verify the issue and the fix >>>> using >>>> ip link set commands. >>>> >>>> CC: Jay Vosburgh <j.vosburgh@gmail.com> >>>> CC: Veaceslav Falico <vfalico@gmail.com> >>>> CC: Andy Gospodarek <gospo@cumulusnetworks.com> >>>> CC: netdev@vger.kernel.org (open list:BONDING DRIVER) >>>> Signed-off-by: Jarod Wilson <jarod@redhat.com> >>>> --- >>> >>> >>> Hi Jarod, >>> When I did 97a1e6396, I tested all of these cases successfully and >>> they still work. >>> in net/core/dev.c, dev_set_mac_address() we have: >>> dev->addr_assign_type = NET_ADDR_SET; >>> So it's actually changed when the user sets the mac and you don't have >>> to do it in >>> bond_set_mac_address(). Just to confirm, I tried this just now: >>> # modprobe bonding >>> # ip l sh bond0 >>> 9: bond0: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc >>> noqueue state DOWN mode DEFAULT group default >>> link/ether d2:62:c7:90:93:b9 brd ff:ff:ff:ff:ff:ff >>> # ip l set dev bond0 address 00:11:22:33:44:55 >>> # ip l sh bond0 >>> 9: bond0: <NO-CARRIER,BROADCAST,MULTICAST,MASTER,UP> mtu 1500 qdisc >>> noqueue state DOWN mode DEFAULT group default >>> link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff >>> # ifenslave bond0 enp6s0 >>> # ip l sh bond0 >>> 9: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 qdisc >>> noqueue state UP mode DEFAULT group default >>> link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff >>> >>> The user-specified mac address is kept. >> >> >> Hrm. I was definitely able to make it fail. I may have been using a >> combination of ip link set and nmcli though, and the bug could be in >> nmcli, since it seems to completely lose track of a configured mac for a >> bond. It definitely reproduces 100% of the time with an older kernel you >> used to work on. ;) >> >> It may require a specific chain of commands to reproduce, so I think its >> still probably a good idea to set _SET in bond_set_mac_address() for the >> sake of completeness/consistency. >> >> I'll see if I can manage to re-reproduce it again with exact chain of >> commands on Monday... > > > I'll just wipe the egg off my face now. I can't come up with a sequence that > fails if I don't involve nmcli. > > It *does* fail on some older kernels with bonding driver backports, but > they're lacking some assignments of NET_ADDR_SET (will fix that), and I keep > getting the eth0 address when networkmangler brings up the link, but I think > that can be attributed to nm not actually paying attention to a > user-specified mac address. > > So I guess this really does nothing in practice... (Even with this change, > nmcli continues to ignore a user-specified ethernet.mac-address). But then > why do the address copy in bond_set_mac_address() at all? Remnants of the > past? > > > -- > Jarod Wilson > jarod@redhat.com We need to copy the mac address in the ndo_set_mac_address() function because dev_set_mac_address() doesn't do it for us. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-09 12:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-05 22:24 [PATCH] net/bonding: fix propagation of user-specified bond MAC Jarod Wilson
2015-06-06 13:29 ` Nikolay Aleksandrov
[not found] ` <55738E41.1020702@redhat.com>
[not found] ` <5575E828.9060900@redhat.com>
2015-06-09 12:49 ` Nikolay Aleksandrov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).