On 16/03/16(Wed) 20:54, Momtchil Momtchev wrote:
> [..]
> Here is my patch that adds support for creating IPv6-only or IPv4-only
> bridges. This is different from simply blocking one of the protocols via PF
> - it allows you to create a setup where IPv4 is routed and IPv6 is bridged
> (or vice versa). Both of them being filtered by the same set of PF rules. It
> adds two new bridge port options to ifconfig - BLOCKIPV4 and BLOCKIPV6.
> BLOCKIPV4 also stops ARPs requests from "leaking" across the bridge -
> something I couldn't accomplish by PF alone.
Interesting. It seems that some devs would have a use case for that.
However I wonder if it wouldn't make more sense to extend the rule
filtering to be able to filter on something else than the Ethernet src/dst.
Reyk, Goda what to you think? Are you also interested in blocking IPv4
and/or IPv6?
Note that Ethernet Multicast/IP Multicast are also blocked in bridge(4)
via the link0 and link1 flags. Do you think it would make sense to use
rules for that too?
> The patch breaks the binary compatibility of ifconfig - it must be
> rebuilt with the new kernel.
This is not a big deal.
> I don't know if anyone will find any use for it. For sure it is very
> useful with the second-biggest FTTH/ADSL operator in France who offers
> consumer-grade IPv6 access with an indivisible /64 network that must be
> bridged for firewalling (and a single IPv4/32 address that must be NATted).
> Patch is against -current, any comments are welcome.
Comments below, but let's see what Reyk and Goda think the right
approach is before sending a new one.
> Index: sbin/ifconfig/brconfig.c
> ===================================================================
> RCS file: /cvs/src/sbin/ifconfig/brconfig.c,v
> retrieving revision 1.9
> diff -u -p -u -r1.9 brconfig.c
> --- sbin/ifconfig/brconfig.c 18 Jul 2015 06:50:24 -0000 1.9
> +++ sbin/ifconfig/brconfig.c 16 Mar 2016 19:37:44 -0000
> @@ -59,7 +59,7 @@ void bridge_showrule(struct ifbrlreq *);
>
> #define IFBAFBITS "\020\1STATIC"
> #define IFBIFBITS \
> -"\020\1LEARNING\2DISCOVER\3BLOCKNONIP\4STP\5EDGE\6AUTOEDGE\7PTP\10AUTOPTP\11SPAN"
> +"\020\1LEARNING\2DISCOVER\3BLOCKNONIP\4STP\5EDGE\6AUTOEDGE\7PTP\10AUTOPTP\11BLOCKIPV4\12BLOCKIPV6\15SPAN"
>
> #define PV2ID(pv, epri, eaddr) do { \
> epri = pv >> 48; \
> @@ -93,102 +93,28 @@ char *stproles[] = {
> };
>
>
> -void
> -setdiscover(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_DISCOVER);
> -}
> -
> -void
> -unsetdiscover(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_DISCOVER);
> -}
> -
> -void
> -setblocknonip(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_BLOCKNONIP);
> -}
> -
> -void
> -unsetblocknonip(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_BLOCKNONIP);
> -}
> -
> -void
> -setlearn(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_LEARNING);
> -}
> -
> -void
> -unsetlearn(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_LEARNING);
> -}
> -
> -void
> -setstp(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_STP);
> -}
> -
> -void
> -unsetstp(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_STP);
> -}
> -
> -void
> -setedge(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_BSTP_EDGE);
> -}
> -
> -void
> -unsetedge(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_BSTP_EDGE);
> -}
> -
> -void
> -setautoedge(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_BSTP_AUTOEDGE);
> -}
> -
> -void
> -unsetautoedge(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_BSTP_AUTOEDGE);
> -}
> -
> -void
> -setptp(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_BSTP_PTP);
> -}
> -
> -void
> -unsetptp(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_BSTP_PTP);
> -}
> -
> -void
> -setautoptp(const char *val, int d)
> -{
> - bridge_ifsetflag(val, IFBIF_BSTP_AUTOPTP);
> -}
> -
> -void
> -unsetautoptp(const char *val, int d)
> -{
> - bridge_ifclrflag(val, IFBIF_BSTP_AUTOPTP);
> -}
> -
> +#define IFBIF_SETUNSET(NAME,FLAG) \
> + void \
> + set ## NAME (const char *val, int d) \
> + { \
> + bridge_ifsetflag(val, FLAG); \
> + } \
> + void \
> + unset ## NAME (const char *val, int d) \
> + { \
> + bridge_ifclrflag(val, FLAG); \
> + }
Please avoid mixing a refactoring and a new feature in the same diff.
Plus this kind of abstraction are questionable and might block or delay
your diff to go in ;)
> +IFBIF_SETUNSET(discover,IFBIF_DISCOVER)
> +IFBIF_SETUNSET(blocknonip,IFBIF_BLOCKNONIP)
> +IFBIF_SETUNSET(blockipv4,IFBIF_BLOCKIPV4)
> +IFBIF_SETUNSET(blockipv6,IFBIF_BLOCKIPV6)
> +IFBIF_SETUNSET(learn,IFBIF_LEARNING)
> +IFBIF_SETUNSET(stp,IFBIF_STP)
> +IFBIF_SETUNSET(edge,IFBIF_BSTP_EDGE)
> +IFBIF_SETUNSET(autoedge,IFBIF_BSTP_AUTOEDGE)
> +IFBIF_SETUNSET(ptp,IFBIF_BSTP_PTP)
> +IFBIF_SETUNSET(autoptp,IFBIF_BSTP_AUTOPTP)
>
> void
> bridge_ifsetflag(const char *ifsname, u_int32_t flag)
> Index: sys/net/if_bridge.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_bridge.c,v
> retrieving revision 1.276
> diff -u -p -u -r1.276 if_bridge.c
> --- sys/net/if_bridge.c 8 Mar 2016 09:09:43 -0000 1.276
> +++ sys/net/if_bridge.c 16 Mar 2016 19:37:44 -0000
> @@ -122,7 +122,7 @@ void bridge_stop(struct bridge_softc *);
> void bridge_init(struct bridge_softc *);
> int bridge_bifconf(struct bridge_softc *, struct ifbifconf *);
>
> -int bridge_blocknonip(struct ether_header *, struct mbuf *);
> +int bridge_blockiptype(struct ether_header *, struct mbuf *, u_int32_t);
> struct mbuf *bridge_ip(struct bridge_softc *, int, struct ifnet *,
> struct ether_header *, struct mbuf *m);
> int bridge_ifenqueue(struct bridge_softc *, struct ifnet *, struct mbuf
> *);
> @@ -783,6 +783,14 @@ bridge_output(struct ifnet *ifp, struct
> (p->bif_flags & IFBIF_STP) &&
> (p->bif_state == BSTP_IFSTATE_DISCARDING))
> continue;
> +
> + /* If we are blocking protocol types
> + * send this packet only if this was the
> + * original output interface
> + */
Your comment isn't respecting syle(9) and more generally your diff is
badly formatted and wont apply (tabs vs spaces). Check your mail client,
they are generally responsible for that ;)
> + if (dst_if != ifp
> + && bridge_blockiptype(eh, m, p->bif_flags))
> + continue;
> #if NMPW > 0
> /*
> * Split horizon: avoid broadcasting messages from
> @@ -960,7 +968,7 @@ bridgeintr_frame(struct bridge_softc *sc
> }
> }
>
> - if (ifl->bif_flags & IFBIF_BLOCKNONIP && bridge_blocknonip(&eh, m)) {
> + if (bridge_blockiptype(&eh, m, ifl->bif_flags)) {
> m_freem(m);
> return;
> }
> @@ -1196,9 +1204,8 @@ bridge_broadcast(struct bridge_softc *sc
> (m->m_flags & (M_BCAST | M_MCAST)) == 0)
> continue;
>
> - /* Drop non-IP frames if the appropriate flag is set. */
> - if (p->bif_flags & IFBIF_BLOCKNONIP &&
> - bridge_blocknonip(eh, m))
> + /* Drop frames if the appropriate flag is set. */
> + if (bridge_blockiptype(eh, m, p->bif_flags))
> continue;
>
> if (bridge_filterrule(&p->bif_brlout, eh, m) == BRL_ACTION_BLOCK)
> @@ -1317,20 +1324,25 @@ bridge_span(struct bridge_softc *sc, str
> }
>
> /*
> - * Block non-ip frames:
> - * Returns 0 if frame is ip, and 1 if it should be dropped.
> + * Block frames by type : non-IP, IPv4 or IPv6 :
> + * Returns 0 if frame is the right type, and 1 if it should be dropped.
> */
> int
> -bridge_blocknonip(struct ether_header *eh, struct mbuf *m)
> +bridge_blockiptype(struct ether_header *eh, struct mbuf *m, u_int32_t
> flags)
> {
> struct llc llc;
> u_int16_t etype;
>
> - if (m->m_pkthdr.len < ETHER_HDR_LEN)
> - return (1);
> + if (m->m_pkthdr.len < ETHER_HDR_LEN) {
> + if ((flags & IFBIF_BLOCKNONIP) == IFBIF_BLOCKNONIP)
> + return (1);
> + return (0);
> + }
>
> #if NVLAN > 0
> - if (m->m_flags & M_VLANTAG)
> + /* TODO: Should VLAN-tagged packets be considered non-IP? */
> + if ((m->m_flags & M_VLANTAG) &&
> + (flags & IFBIF_BLOCKNONIP) == IFBIF_BLOCKNONIP)
> return (1);
This is an open question. Problem is that driver with hardware vlan
support feed decapsulated packets to the stack, which does not play
well with bridge, but you already know that ;)
> #ifdef IPSEC
> Index: sys/net/if_bridge.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if_bridge.h,v
> retrieving revision 1.48
> diff -u -p -u -r1.48 if_bridge.h
> --- sys/net/if_bridge.h 1 Dec 2015 18:28:29 -0000 1.48
> +++ sys/net/if_bridge.h 16 Mar 2016 19:37:44 -0000
> @@ -70,8 +70,10 @@ struct ifbreq {
> #define IFBIF_BSTP_AUTOEDGE 0x0020 /* member stp autoedge enabled */
> #define IFBIF_BSTP_PTP 0x0040 /* member stp ptp */
> #define IFBIF_BSTP_AUTOPTP 0x0080 /* member stp autoptp enabled */
> -#define IFBIF_SPAN 0x0100 /* ifs is a span port (ro) */
> -#define IFBIF_RO_MASK 0xff00 /* read only bits */
> +#define IFBIF_BLOCKIPV4 0x0100 /* ifs blocks IPv4 in/out */
> +#define IFBIF_BLOCKIPV6 0x0200 /* ifs blocks IPv6 in/out */
> +#define IFBIF_SPAN 0x1000 /* ifs is a span port (ro) */
> +#define IFBIF_RO_MASK 0xf000 /* read only bits */
We try to stick to #define<space> or #define<tab> but not mix the too in
the same file.
