trunk: keep interface up on port removal

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

trunk: keep interface up on port removal

Klemens Nanni-2
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 -0000 1.149
+++ if_trunk.c 12 Sep 2020 15:41:14 -0000
@@ -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)

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Florian Obser-2
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.c 28 Jul 2020 09:52:32 -0000 1.149
> +++ if_trunk.c 12 Sep 2020 15:41:14 -0000
> @@ -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.

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Alexander Bluhm
In reply to this post by Klemens Nanni-2
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 -0000 1.149
> +++ if_trunk.c 12 Sep 2020 15:41:14 -0000
> @@ -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)

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Stuart Henderson
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 <[hidden email]> 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 -0000 1.149
>> +++ if_trunk.c 12 Sep 2020 15:41:14 -0000
>> @@ -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)

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Stuart Henderson
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

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Theo de Raadt-2
Stuart Henderson <[hidden email]> 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?

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Klemens Nanni-2
In reply to this post by Stuart Henderson
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).

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Stuart Henderson
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?

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Klemens Nanni-2
In reply to this post by Klemens Nanni-2
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?

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Stuart Henderson
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<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?

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.

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Klemens Nanni-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

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

Reply | Threaded
Open this post in threaded view
|

Re: trunk: keep interface up on port removal

Klemens Nanni-2
In reply to this post by Klemens Nanni-2
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.