Re: trunk: keep interface up on port removal
On Mon, Sep 14, 2020 at 10:57:16AM +0200, Klemens Nanni wrote: > I tested removing a single port from trunk and observed that both > interfaces do end up with the same MAC address, but this happens without > my diff already - I still don't see any behaviour after my diff wrt. MAC > addresses or anything else but the UP flag on port interfaces. I did sloppy testing... tl;dr: MACs have been restored properly before my diff already. I looked at this yet again because removing a single port and destroying the trunk use the same code path, hence it didn't make much sense to me for them to behave differently on second thought. No matter which port is removed, its MAC is reset to what it was before becoming a trunk port. I refrained from inlining supporting shell code/output this time, please test yourself to confirm.
Re: trunk: keep interface up on port removal
On 2020/09/14 10:57, Klemens Nanni wrote: > On Sun, Sep 13, 2020 at 06:44:13PM +0100, Stuart Henderson wrote: > > I can't test at the moment, but the other case is removing a port from > > the trunk without destroying the trunk interface itself. That's almost > > certainly what I was testing at the time. > Right, that's different from destroying the trunk. > > > The other thing to be aware of is that you may then end up with two > > separate interfaces with the same MAC. This may cause some problems for > > incoming traffic now we don't use weak host model on non-router systems. > > Not sure there is much we can do about that though. > I tested removing a single port from trunk and observed that both > interfaces do end up with the same MAC address, but this happens without > my diff already - I still don't see any behaviour after my diff wrt. MAC > addresses or anything else but the UP flag on port interfaces. > Thanks, that sounds like it's alright then.
Re: trunk: keep interface up on port removal
On Sun, Sep 13, 2020 at 06:44:13PM +0100, Stuart Henderson wrote: > I can't test at the moment, but the other case is removing a port from > the trunk without destroying the trunk interface itself. That's almost > certainly what I was testing at the time. Right, that's different from destroying the trunk. > The other thing to be aware of is that you may then end up with two > separate interfaces with the same MAC. This may cause some problems for > incoming traffic now we don't use weak host model on non-router systems. > Not sure there is much we can do about that though. I tested removing a single port from trunk and observed that both interfaces do end up with the same MAC address, but this happens without my diff already - I still don't see any behaviour after my diff wrt. MAC addresses or anything else but the UP flag on port interfaces.
Re: trunk: keep interface up on port removal
On 2020/09/13 14:47, Klemens Nanni wrote:
> 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:
This was in 2015 btw.
> 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 mtu 1500
> lladdr 3c:97:0e:6e:e9:1b
> athn0: flags=8843 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 mtu 1500
> lladdr 3c:97:0e:6e:e9:1b
> athn0: flags=8943 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 mtu 1500
> lladdr 3c:97:0e:6e:e9:1b
> athn0: flags=8843 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 mtu 1500
> lladdr 3c:97:0e:6e:e9:1b
> athn0: flags=8843 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 mtu 1500
> lladdr 88:c2:6a:b2:21:41
> athn0: flags=8943 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 mtu 1500
> lladdr 3c:97:0e:6e:e9:1b
> athn0: flags=8843 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?
I can't test at the moment, but the other case is removing a port from
the trunk without destroying the trunk interface itself. That's almost
certainly what I was testing at the time.
The other thing to be aware of is that you may then end up with two
separate interfaces with the same MAC. This may cause some problems for
incoming traffic now we don't use weak host model on non-router systems.
Not sure there is much we can do about that though.
Re: trunk: keep interface up on port removal
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));
}
ifp->if_flags |= IFF_UP;
ifrq.ifr_flags = ifp->if_flags;
(*ifp->if_ioctl)(ifp, SIOCSIFFLAGS, (caddr_t));
#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));
}
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 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 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 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8943 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 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 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 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 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 mtu 1500
lladdr 88:c2:6a:b2:21:41
athn0: flags=8943 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 mtu 1500
lladdr 3c:97:0e:6e:e9:1b
athn0: flags=8843 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?
Re: trunk: keep interface up on port removal
On 2020/09/13 13:23, 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). > It's not in CVS log because it was tested and didn't work so wasn't committed. I don't know if there's precedent for documenting everything somebody might try but fails in a code comment, feels like the tree would be littered with possibly outdated comments if that's done regularly?
Re: trunk: keep interface up on port removal
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).
Re: trunk: keep interface up on port removal
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 > Ah, that seems bad. And in the presence of port-sec or mac security on switches, this could provide a poor experience? Is this the right time to change things?
Re: trunk: keep interface up on port removal
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
Re: trunk: keep interface up on port removal
This has been tried before, I forget what but there were problems -- Sent from a phone, apologies for poor formatting. On 12 September 2020 21:16:31 Alexander Bluhm wrote: OK bluhm@ On Sat, Sep 12, 2020 at 05:49:52PM +0200, Klemens Nanni wrote: Index: if_trunk.c === RCS file: /cvs/src/sys/net/if_trunk.c,v retrieving revision 1.149 diff -u -p -r1.149 if_trunk.c --- if_trunk.c 28 Jul 2020 09:52:32 - 1.149 +++ if_trunk.c 12 Sep 2020 15:41:14 - @@ -423,10 +423,6 @@ trunk_port_destroy(struct trunk_port *tp /* Remove multicast addresses from this port */ trunk_ether_cmdmulti(tp, SIOCDELMULTI); - /* Port has to be down */ - if (ifp->if_flags & IFF_UP) - if_down(ifp); - ifpromisc(ifp, 0); if (tr->tr_port_destroy != NULL)
Re: trunk: keep interface up on port removal
OK bluhm@ On Sat, Sep 12, 2020 at 05:49:52PM +0200, Klemens Nanni wrote: > Index: if_trunk.c > === > RCS file: /cvs/src/sys/net/if_trunk.c,v > retrieving revision 1.149 > diff -u -p -r1.149 if_trunk.c > --- if_trunk.c28 Jul 2020 09:52:32 - 1.149 > +++ if_trunk.c12 Sep 2020 15:41:14 - > @@ -423,10 +423,6 @@ trunk_port_destroy(struct trunk_port *tp > /* Remove multicast addresses from this port */ > trunk_ether_cmdmulti(tp, SIOCDELMULTI); > > - /* Port has to be down */ > - if (ifp->if_flags & IFF_UP) > - if_down(ifp); > - > ifpromisc(ifp, 0); > > if (tr->tr_port_destroy != NULL)
Re: trunk: keep interface up on port removal
Seems reasonable. OK On Sat, Sep 12, 2020 at 05:49:52PM +0200, Klemens Nanni wrote: > Unconfiguring a member interface from trunk(4) or simply destroying the > trunk pulls the member down for no reason, both comment and code are > there since import, but I see no justification for doing so. > > aggr(4) does not pull its member down upon removal either. > > I came across this after > > $ doas ifconfig trunk0 destroy > $ doas sh /etc/netstart trunk0 > > yielded no network and I had to manually pull up members. > > Feedback? OK? > > > Index: if_trunk.c > === > RCS file: /cvs/src/sys/net/if_trunk.c,v > retrieving revision 1.149 > diff -u -p -r1.149 if_trunk.c > --- if_trunk.c28 Jul 2020 09:52:32 - 1.149 > +++ if_trunk.c12 Sep 2020 15:41:14 - > @@ -423,10 +423,6 @@ trunk_port_destroy(struct trunk_port *tp > /* Remove multicast addresses from this port */ > trunk_ether_cmdmulti(tp, SIOCDELMULTI); > > - /* Port has to be down */ > - if (ifp->if_flags & IFF_UP) > - if_down(ifp); > - > ifpromisc(ifp, 0); > > if (tr->tr_port_destroy != NULL) > -- I'm not entirely sure you are real.
trunk: keep interface up on port removal
Unconfiguring a member interface from trunk(4) or simply destroying the trunk pulls the member down for no reason, both comment and code are there since import, but I see no justification for doing so. aggr(4) does not pull its member down upon removal either. I came across this after $ doas ifconfig trunk0 destroy $ doas sh /etc/netstart trunk0 yielded no network and I had to manually pull up members. Feedback? OK? Index: if_trunk.c === RCS file: /cvs/src/sys/net/if_trunk.c,v retrieving revision 1.149 diff -u -p -r1.149 if_trunk.c --- if_trunk.c 28 Jul 2020 09:52:32 - 1.149 +++ if_trunk.c 12 Sep 2020 15:41:14 - @@ -423,10 +423,6 @@ trunk_port_destroy(struct trunk_port *tp /* Remove multicast addresses from this port */ trunk_ether_cmdmulti(tp, SIOCDELMULTI); - /* Port has to be down */ - if (ifp->if_flags & IFF_UP) - if_down(ifp); - ifpromisc(ifp, 0); if (tr->tr_port_destroy != NULL)
