OpenBSD: bridge(4), simplify IFT_ETHER checks

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

OpenBSD: bridge(4), simplify IFT_ETHER checks

Eygene Ryabinkin
Good day.

With the hints from Martin Pieuchot had found out that the current
handling of IFT_ETHER for bridge(4) /and nowadays, after de-introduction
of IFT_MPLSTUNNEL/mpw(4) type interfaces, only IFT_ETHER ones can
be attached to the bridge(4)/
 - isn't uniform across both ADD ioctls,
 - has some redundant "if (ifs->if_type == IFT_ETHER)" statements
   both in ioctl and bridge forwarding code paths.

Can the attached patch be reviewed and considered for inclusion
into the main code?

Thanks!
--
rea

0002-bridge-4-simplify-interface-handling-logics.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: bridge(4), simplify IFT_ETHER checks

Martin Pieuchot
On 07/06/19(Fri) 20:55, Eygene Ryabinkin wrote:
> Good day.
>
> With the hints from Martin Pieuchot had found out that the current
> handling of IFT_ETHER for bridge(4) /and nowadays, after de-introduction
> of IFT_MPLSTUNNEL/mpw(4) type interfaces, only IFT_ETHER ones can
> be attached to the bridge(4)/

Can't gif(4) interfaces be attached as well?

>  - isn't uniform across both ADD ioctls,
>  - has some redundant "if (ifs->if_type == IFT_ETHER)" statements
>    both in ioctl and bridge forwarding code paths.
>
> Can the attached patch be reviewed and considered for inclusion
> into the main code?
>
> Thanks!
> --
> rea

> From c5dc1bf7b8875788af3c325e618b08e1ff3d90a9 Mon Sep 17 00:00:00 2001
> From: Eygene Ryabinkin <[hidden email]>
> Date: Fri, 7 Jun 2019 20:23:58 +0300
> Subject: [PATCH 2/2] bridge(4): simplify interface handling logics
>
>  - uniformly check for IFT_ETHER in both add-type ioctls:
>    only Ethernet interfaces can nowadays be bridge(4) members
>
>  - move promiscious mode handling out of IFT_ETHER checks:
>    prior code structure was due to the IFT_MPLSTUNNEL interface
>    types that need no promiscious mode, but now they are gone
>
>  - don't check for IFT_ETHER in the run-time packet handling:
>    this is now completely redundant.
>
> Signed-off-by: Eygene Ryabinkin <[hidden email]>
> ---
>  sys/net/if_bridge.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
> index d64bdc05abb..496ed8a3b97 100644
> --- a/sys/net/if_bridge.c
> +++ b/sys/net/if_bridge.c
> @@ -304,19 +304,18 @@ bridge_ioctl(struct ifnet *ifp, u_long cmd, caddr_t data)
>   break;
>   }
>  
> - if (ifs->if_type == IFT_ETHER) {
> - error = ifpromisc(ifs, 1);
> - if (error != 0)
> - break;
> - } else {
> + if (ifs->if_type != IFT_ETHER) {
>   error = EINVAL;
>   break;
>   }
>  
> + error = ifpromisc(ifs, 1);
> + if (error != 0)
> + break;
> +
>   bif = malloc(sizeof(*bif), M_DEVBUF, M_NOWAIT|M_ZERO);
>   if (bif == NULL) {
> - if (ifs->if_type == IFT_ETHER)
> - ifpromisc(ifs, 0);
> + ifpromisc(ifs, 0);
>   error = ENOMEM;
>   break;
>   }
> @@ -1201,8 +1200,6 @@ bridge_process(struct ifnet *ifp, struct mbuf *m)
>   */
>   bif0 = bif;
>   SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
> - if (bif->ifp->if_type != IFT_ETHER)
> - continue;
>   if (bridge_ourether(bif->ifp, eh->ether_dhost)) {
>   if (bif0->bif_flags & IFBIF_LEARNING)
>   bridge_rtupdate(sc,
> --
> 2.21.0
>

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: bridge(4), simplify IFT_ETHER checks

Eygene Ryabinkin
Martin, good day.

Sun, Jun 09, 2019 at 11:35:48AM -0300, Martin Pieuchot wrote:
> On 07/06/19(Fri) 20:55, Eygene Ryabinkin wrote:
> > With the hints from Martin Pieuchot had found out that the current
> > handling of IFT_ETHER for bridge(4) /and nowadays, after de-introduction
> > of IFT_MPLSTUNNEL/mpw(4) type interfaces, only IFT_ETHER ones can
> > be attached to the bridge(4)/
>
> Can't gif(4) interfaces be attached as well?

Reading bridge_ioctl() in /sys/net/if_bridge.c and examining
paths for SIOCBRDGADD/SIOCBRDGADDL and SIOCBRDGADDS I see the
following blocks:
{{{ ADD/ADDL
                if (ifs->if_type == IFT_ETHER) {
                        error = ifpromisc(ifs, 1);
                        if (error != 0)
                                break;
                } else {
                        error = EINVAL;
                        break;
                }
}}}
{{{ ADDS
                if (ifs->if_type != IFT_ETHER) {
                        error = EINVAL;
                        break;
                }
}}}
So, ifs->if_type != IFT_ETHER will result in EINVAL.  Moreover,
reading gif(4) yields
{{{
     Previously, gif supported RFC 3378 EtherIP tunnels over bridge(4)
     interfaces.  This is now handled by etherip(4).
}}}
and this provides the explicit explanation that gif(4) used to work
with bridge(4), but now etherip(4) took over for Ethernet tunnels
and other gif(4) types seem to have no Ethernet-compatible MACs,
so they can't be used with bridge(4).
--
rea

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: bridge(4), simplify IFT_ETHER checks

Stuart Henderson
In reply to this post by Martin Pieuchot
On 2019/06/09 11:35, Martin Pieuchot wrote:
> On 07/06/19(Fri) 20:55, Eygene Ryabinkin wrote:
> > Good day.
> >
> > With the hints from Martin Pieuchot had found out that the current
> > handling of IFT_ETHER for bridge(4) /and nowadays, after de-introduction
> > of IFT_MPLSTUNNEL/mpw(4) type interfaces, only IFT_ETHER ones can
> > be attached to the bridge(4)/
>
> Can't gif(4) interfaces be attached as well?

gif(4) can't any more. Some of the gre(4) family can, but only the
ethernet-ish ones - there is this existing check in if_bridge.c when
the port is added to the bridge:

                if (ifs->if_type == IFT_ETHER) {
                        error = ifpromisc(ifs, 1);
                        if (error != 0)
                                break;
                } else {
                        error = EINVAL;
                        break;
                }

> > From c5dc1bf7b8875788af3c325e618b08e1ff3d90a9 Mon Sep 17 00:00:00 2001
> > From: Eygene Ryabinkin <[hidden email]>
> > Date: Fri, 7 Jun 2019 20:23:58 +0300
> > Subject: [PATCH 2/2] bridge(4): simplify interface handling logics
> >
> >  - uniformly check for IFT_ETHER in both add-type ioctls:
> >    only Ethernet interfaces can nowadays be bridge(4) members
> >
> >  - move promiscious mode handling out of IFT_ETHER checks:
> >    prior code structure was due to the IFT_MPLSTUNNEL interface
> >    types that need no promiscious mode, but now they are gone
> >
> >  - don't check for IFT_ETHER in the run-time packet handling:
> >    this is now completely redundant.

I don't think I have a good place to test bridge(4) diffs at the moment
but this looks correct to me.

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: bridge(4), simplify IFT_ETHER checks

Martin Pieuchot
In reply to this post by Eygene Ryabinkin
On 09/06/19(Sun) 18:41, Eygene Ryabinkin wrote:

> Martin, good day.
>
> Sun, Jun 09, 2019 at 11:35:48AM -0300, Martin Pieuchot wrote:
> > On 07/06/19(Fri) 20:55, Eygene Ryabinkin wrote:
> > > With the hints from Martin Pieuchot had found out that the current
> > > handling of IFT_ETHER for bridge(4) /and nowadays, after de-introduction
> > > of IFT_MPLSTUNNEL/mpw(4) type interfaces, only IFT_ETHER ones can
> > > be attached to the bridge(4)/
> >
> > Can't gif(4) interfaces be attached as well?
>
> Reading bridge_ioctl() in /sys/net/if_bridge.c and examining
> paths for SIOCBRDGADD/SIOCBRDGADDL and SIOCBRDGADDS I see the
> following blocks:
> {{{ ADD/ADDL
>                 if (ifs->if_type == IFT_ETHER) {
>                         error = ifpromisc(ifs, 1);
>                         if (error != 0)
>                                 break;
>                 } else {
>                         error = EINVAL;
>                         break;
>                 }
> }}}
> {{{ ADDS
>                 if (ifs->if_type != IFT_ETHER) {
>                         error = EINVAL;
>                         break;
>                 }
> }}}
> So, ifs->if_type != IFT_ETHER will result in EINVAL.  Moreover,
> reading gif(4) yields
> {{{
>      Previously, gif supported RFC 3378 EtherIP tunnels over bridge(4)
>      interfaces.  This is now handled by etherip(4).
> }}}
> and this provides the explicit explanation that gif(4) used to work
> with bridge(4), but now etherip(4) took over for Ethernet tunnels
> and other gif(4) types seem to have no Ethernet-compatible MACs,
> so they can't be used with bridge(4).

Thanks for digging that informations!  In that case I'd like to commit
the tweaked version of your diff below.  Are you ok with it?

Index: net/if_bridge.c
===================================================================
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.333
diff -u -p -r1.333 if_bridge.c
--- net/if_bridge.c 13 May 2019 18:14:05 -0000 1.333
+++ net/if_bridge.c 9 Jun 2019 17:12:22 -0000
@@ -285,7 +285,10 @@ bridge_ioctl(struct ifnet *ifp, u_long c
  error = ENOENT;
  break;
  }
-
+ if (ifs->if_type != IFT_ETHER) {
+ error = EINVAL;
+ break;
+ }
  if (ifs->if_bridgeidx != 0) {
  if (ifs->if_bridgeidx == ifp->if_index)
  error = EEXIST;
@@ -304,23 +307,18 @@ bridge_ioctl(struct ifnet *ifp, u_long c
  break;
  }
 
- if (ifs->if_type == IFT_ETHER) {
- error = ifpromisc(ifs, 1);
- if (error != 0)
- break;
- } else {
- error = EINVAL;
- break;
- }
-
  bif = malloc(sizeof(*bif), M_DEVBUF, M_NOWAIT|M_ZERO);
  if (bif == NULL) {
- if (ifs->if_type == IFT_ETHER)
- ifpromisc(ifs, 0);
  error = ENOMEM;
  break;
  }
 
+ error = ifpromisc(ifs, 1);
+ if (error != 0) {
+ free(bif, M_DEVBUF, sizeof(*bif));
+ break;
+ }
+
  bif->bridge_sc = sc;
  bif->ifp = ifs;
  bif->bif_flags = IFBIF_LEARNING | IFBIF_DISCOVER;
@@ -363,7 +361,10 @@ bridge_ioctl(struct ifnet *ifp, u_long c
  break;
  }
  if (ifs->if_bridgeidx != 0) {
- error = EBUSY;
+ if (ifs->if_bridgeidx == ifp->if_index)
+ error = EEXIST;
+ else
+ error = EBUSY;
  break;
  }
  SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
@@ -1201,8 +1202,6 @@ bridge_process(struct ifnet *ifp, struct
  */
  bif0 = bif;
  SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
- if (bif->ifp->if_type != IFT_ETHER)
- continue;
  if (bridge_ourether(bif->ifp, eh->ether_dhost)) {
  if (bif0->bif_flags & IFBIF_LEARNING)
  bridge_rtupdate(sc,

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: bridge(4), simplify IFT_ETHER checks

Eygene Ryabinkin
Sun, Jun 09, 2019 at 02:14:57PM -0300, Martin Pieuchot wrote:
> In that case I'd like to commit the tweaked version of your diff
> below.  Are you ok with it?

Yes.  Though, I'd split out the following chunk

> Index: net/if_bridge.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_bridge.c,v
> retrieving revision 1.333
> diff -u -p -r1.333 if_bridge.c
> --- net/if_bridge.c 13 May 2019 18:14:05 -0000 1.333
> +++ net/if_bridge.c 9 Jun 2019 17:12:22 -0000
> @@ -363,7 +361,10 @@ bridge_ioctl(struct ifnet *ifp, u_long c
>   break;
>   }
>   if (ifs->if_bridgeidx != 0) {
> - error = EBUSY;
> + if (ifs->if_bridgeidx == ifp->if_index)
> + error = EEXIST;
> + else
> + error = EBUSY;
>   break;
>   }
>   SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
into its own commit: it unifies error codes, not IFT_ETHER checks.
Might ease the work of people who'll be studying commits and their reasons;
but I am not familiar enough with OpenBSD's general practices on such
questions and may be there are reasons not to do so.

Thanks a lot!
--
rea

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: bridge(4), simplify IFT_ETHER checks

David Gwynne-5
In reply to this post by Martin Pieuchot


> On 10 Jun 2019, at 03:14, Martin Pieuchot <[hidden email]> wrote:
>
> On 09/06/19(Sun) 18:41, Eygene Ryabinkin wrote:
>> Martin, good day.
>>
>> Sun, Jun 09, 2019 at 11:35:48AM -0300, Martin Pieuchot wrote:
>>> On 07/06/19(Fri) 20:55, Eygene Ryabinkin wrote:
>>>> With the hints from Martin Pieuchot had found out that the current
>>>> handling of IFT_ETHER for bridge(4) /and nowadays, after de-introduction
>>>> of IFT_MPLSTUNNEL/mpw(4) type interfaces, only IFT_ETHER ones can
>>>> be attached to the bridge(4)/
>>>
>>> Can't gif(4) interfaces be attached as well?
>>
>> Reading bridge_ioctl() in /sys/net/if_bridge.c and examining
>> paths for SIOCBRDGADD/SIOCBRDGADDL and SIOCBRDGADDS I see the
>> following blocks:
>> {{{ ADD/ADDL
>>                if (ifs->if_type == IFT_ETHER) {
>>                        error = ifpromisc(ifs, 1);
>>                        if (error != 0)
>>                                break;
>>                } else {
>>                        error = EINVAL;
>>                        break;
>>                }
>> }}}
>> {{{ ADDS
>>                if (ifs->if_type != IFT_ETHER) {
>>                        error = EINVAL;
>>                        break;
>>                }
>> }}}
>> So, ifs->if_type != IFT_ETHER will result in EINVAL.  Moreover,
>> reading gif(4) yields
>> {{{
>>     Previously, gif supported RFC 3378 EtherIP tunnels over bridge(4)
>>     interfaces.  This is now handled by etherip(4).
>> }}}
>> and this provides the explicit explanation that gif(4) used to work
>> with bridge(4), but now etherip(4) took over for Ethernet tunnels
>> and other gif(4) types seem to have no Ethernet-compatible MACs,
>> so they can't be used with bridge(4).

There are a couple of interfaces and interface types that can change type at runtime. Firstly, any ethernet interface added to a trunk will change from IFT_ETHER to IFT_IEEE8023ADLAG. Secondly, tun and tap can change from IFT_TUNNEL and IFT_ETHER to anything userland sets.

I really dislike both of these behaviours.

FreeBSD at least blocked changing the tun/tap interface types from userland for safety reasons. I would like to do the same.

It sort of makes sense for trunk to change the type of ports, but it also doesn't. Can we just let it check another thing to prevent reuse of ports?

Otherwise everything is created with a type and sticks to it at runtime, which is a lot safer and predictable in my opinion.

dlg

>
> Thanks for digging that informations!  In that case I'd like to commit
> the tweaked version of your diff below.  Are you ok with it?
>
> Index: net/if_bridge.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_bridge.c,v
> retrieving revision 1.333
> diff -u -p -r1.333 if_bridge.c
> --- net/if_bridge.c 13 May 2019 18:14:05 -0000 1.333
> +++ net/if_bridge.c 9 Jun 2019 17:12:22 -0000
> @@ -285,7 +285,10 @@ bridge_ioctl(struct ifnet *ifp, u_long c
> error = ENOENT;
> break;
> }
> -
> + if (ifs->if_type != IFT_ETHER) {
> + error = EINVAL;
> + break;
> + }
> if (ifs->if_bridgeidx != 0) {
> if (ifs->if_bridgeidx == ifp->if_index)
> error = EEXIST;
> @@ -304,23 +307,18 @@ bridge_ioctl(struct ifnet *ifp, u_long c
> break;
> }
>
> - if (ifs->if_type == IFT_ETHER) {
> - error = ifpromisc(ifs, 1);
> - if (error != 0)
> - break;
> - } else {
> - error = EINVAL;
> - break;
> - }
> -
> bif = malloc(sizeof(*bif), M_DEVBUF, M_NOWAIT|M_ZERO);
> if (bif == NULL) {
> - if (ifs->if_type == IFT_ETHER)
> - ifpromisc(ifs, 0);
> error = ENOMEM;
> break;
> }
>
> + error = ifpromisc(ifs, 1);
> + if (error != 0) {
> + free(bif, M_DEVBUF, sizeof(*bif));
> + break;
> + }
> +
> bif->bridge_sc = sc;
> bif->ifp = ifs;
> bif->bif_flags = IFBIF_LEARNING | IFBIF_DISCOVER;
> @@ -363,7 +361,10 @@ bridge_ioctl(struct ifnet *ifp, u_long c
> break;
> }
> if (ifs->if_bridgeidx != 0) {
> - error = EBUSY;
> + if (ifs->if_bridgeidx == ifp->if_index)
> + error = EEXIST;
> + else
> + error = EBUSY;
> break;
> }
> SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_spanlist, bif_next) {
> @@ -1201,8 +1202,6 @@ bridge_process(struct ifnet *ifp, struct
> */
> bif0 = bif;
> SMR_SLIST_FOREACH_LOCKED(bif, &sc->sc_iflist, bif_next) {
> - if (bif->ifp->if_type != IFT_ETHER)
> - continue;
> if (bridge_ourether(bif->ifp, eh->ether_dhost)) {
> if (bif0->bif_flags & IFBIF_LEARNING)
> bridge_rtupdate(sc,