On Sun, Sep 13, 2020 at 01:23:59PM +0200, Klemens Nanni wrote:
> On Sun, Sep 13, 2020 at 11:31:12AM +0100, Stuart Henderson wrote:
> > On 2020/09/13 11:12, Stuart Henderson wrote:
> > > This has been tried before, I forget what but there were problems
> >
> > from chat logs when I tried this before:
> >
> > 14:52 < sthen> if i kill the if_down, no crash, but the mac address doesn't
> > get updated so i end up with the same one on em0, em1, trunk0
> Thanks for mentioning it, I'll look into whether I can fix this or we
> have revert my commit to avoid breakage around MAC addresses.
>
> I'd expect such information to be present in CVS log or code (comments).
I checked the code path for changing MACs in trunk(4) at port removal:
trunk_port_destroy(), which used to pull the port interface down, calls
trunk_port_lladdr() which calls if.c:ifnewlladdr() which looks like
void
ifnewlladdr(struct ifnet *ifp)
{
#ifdef INET6
struct ifaddr *ifa;
#endif
struct ifreq ifrq;
short up;
int s;
s = splnet();
up = ifp->if_flags & IFF_UP;
if (up) {
/* go down for a moment... */
ifp->if_flags &= ~IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
}
ifp->if_flags |= IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
#ifdef INET6
...
#endif
if (!up) {
/* go back down */
ifp->if_flags &= ~IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t)&ifrq);
}
splx(s);
}
So there's a dance around UP interfaces already; CVS log dates this
code back to 2010 when deraadt rearanged code into ifnewlladdr(), the
previous if.c revision also head this dance around UP.
The if_down() line I removed from trunk(4) dates back to if_trunk.c r1.1
from 2005.
Here's some practical tests further indicating that my diff does not
break anything and whatever sthen encountered at whatever time in the
past is no longer an issue:
With my commit in -CURRENT, the MAC address *is* being restored on my
physical interfaces:
$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 04:f0:21:30:37:de
$ doas ifconfig trunk0 trunkport em0 trunkport athn0
$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0:
flags=8b43<UP,BROADCAST,RUNNING,PROMISC,ALLMULTI,SIMPLEX,MULTICAST> mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8943<UP,BROADCAST,RUNNING,PROMISC,SIMPLEX,MULTICAST> mtu
1500
lladdr 3c:97:0e:6e:e9:1b
$ doas ifconfig trunk0 destroy
$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 04:f0:21:30:37:de
Observe how UP is set on the physical interfaces, i.e. my diff is in.
MAC addresses of physical interfaces are properly restored.
They are also restored when I create the same trunk0 interface but
generate a random MAC for it as well, i.e. overwriting MACs for all
ports while in the trunk:
$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 04:f0:21:30:37:de
$ doas ifconfig trunk0 trunkport em0 trunkport athn0 lladdr random
$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0:
flags=8b43<UP,BROADCAST,RUNNING,PROMISC,ALLMULTI,SIMPLEX,MULTICAST> mtu 1500
lladdr 88:c2:6a:b2:21:41
athn0: flags=8943<UP,BROADCAST,RUNNING,PROMISC,SIMPLEX,MULTICAST> mtu
1500
lladdr 88:c2:6a:b2:21:41
$ doas ifconfig trunk0 destroy
$ { ifconfig em0 ; ifconfig athn0 ; } | grep -e flags= -e lladdr
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> mtu 1500
lladdr 04:f0:21:30:37:de
I also tested this with vether(4) ports under trunk just to check if
there's anything different for pseudo interfaces, but they behave the
same as em(4) and athn(4) for me regardless of `lladdr random' on trunk.
Am I missing anything?
