Re: 5.8 panic with vlan on bridge

[Armin Wolfermann]

* Martin Pieuchot  [05.11.2015 11:12]:
> I think I was confused, could you test the other way around, adding
> the bridge before the vlan?
> 
>   # ifconfig em0 up
>   # ifconfig bridge0 add em0 up rule block on em
>   # ifconfig vlan10  vlandev em0 up
 
Ok, doing it this way around it works as expected.

Re: 5.8 panic with vlan on bridge

[Martin Pieuchot]

On 04/11/15(Wed) 18:03, Armin Wolfermann wrote:
> * Martin Pieuchot  [04.11.2015 12:53]:
> > I'd appreciate if you could test my diff and report back. 
> 
> Seems the bridge is working but tagged packets can still be blocked 
> with a bridge rule. This is a current system with your diff and
> 
>   ifconfig em0 up
>   ifconfig vlan10  vlandev em0 up
> 
> If I add a bridge with
> 
>   ifconfig bridge0 add em0 up rule block on em0
> 
> the packets will not reach the vlan interface.

I think I was confused, could you test the other way around, adding
the bridge before the vlan?

  # ifconfig em0 up
  # ifconfig bridge0 add em0 up rule block on em
  # ifconfig vlan10  vlandev em0 up

Re: 5.8 panic with vlan on bridge

[Martin Pieuchot]

On 04/11/15(Wed) 12:25, Armin Wolfermann wrote:
> * Martin Pieuchot  [03.11.2015 17:55]:
> > I'm not sure it applies on 5.8 though...
> 
> Nope, but I found the following changes to be sufficient on 5.8. It just
> keeps the vlan tagged packets away from the bridge.

This only works because your underlying drivers supports hardware vlan.
This is not a proper fix.

I'd appreciate if you could test my diff and report back. 

> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.357
> diff -u -p -u -r1.357 if.c
> --- net/if.c13 Aug 2015 07:19:58 -  1.357
> +++ net/if.c4 Nov 2015 10:35:07 -
> @@ -432,7 +432,7 @@ if_enqueue(struct ifnet *ifp, struct mbu
> unsigned short mflags;
>  
>  #if NBRIDGE > 0
> -   if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0)
> +   if (ifp->if_bridgeport && (m->m_flags & (M_PROTO1 | M_VLANTAG)) == 0)
> return (bridge_output(ifp, m, NULL, NULL));
> m->m_flags &= ~M_PROTO1;/* Loop prevention */
>  #endif
> @@ -522,7 +522,8 @@ if_input_process(void *xmq)
> }
>  
>  #if NBRIDGE > 0
> -   if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
> +   if (ifp->if_bridgeport &&
> +   (m->m_flags & (M_PROTO1 | M_VLANTAG)) == 0) {
> m = bridge_input(ifp, m);
> if (m == NULL)
> continue;
> 

Re: 5.8 panic with vlan on bridge

[Armin Wolfermann]

* Martin Pieuchot  [03.11.2015 17:55]:
> I'm not sure it applies on 5.8 though...

Nope, but I found the following changes to be sufficient on 5.8. It just
keeps the vlan tagged packets away from the bridge.

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.357
diff -u -p -u -r1.357 if.c
--- net/if.c13 Aug 2015 07:19:58 -  1.357
+++ net/if.c4 Nov 2015 10:35:07 -
@@ -432,7 +432,7 @@ if_enqueue(struct ifnet *ifp, struct mbu
unsigned short mflags;
 
 #if NBRIDGE > 0
-   if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0)
+   if (ifp->if_bridgeport && (m->m_flags & (M_PROTO1 | M_VLANTAG)) == 0)
return (bridge_output(ifp, m, NULL, NULL));
m->m_flags &= ~M_PROTO1;/* Loop prevention */
 #endif
@@ -522,7 +522,8 @@ if_input_process(void *xmq)
}
 
 #if NBRIDGE > 0
-   if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
+   if (ifp->if_bridgeport &&
+   (m->m_flags & (M_PROTO1 | M_VLANTAG)) == 0) {
m = bridge_input(ifp, m);
if (m == NULL)
continue;

Re: 5.8 panic with vlan on bridge

[Armin Wolfermann]

* Martin Pieuchot  [04.11.2015 12:53]:
> I'd appreciate if you could test my diff and report back. 

Seems the bridge is working but tagged packets can still be blocked 
with a bridge rule. This is a current system with your diff and

  ifconfig em0 up
  ifconfig vlan10  vlandev em0 up

If I add a bridge with

  ifconfig bridge0 add em0 up rule block on em0

the packets will not reach the vlan interface.

Re: 5.8 panic with vlan on bridge

[Martin Pieuchot]

On 03/11/15(Tue) 11:03, Armin Wolfermann wrote:
> * Armin Wolfermann  [02.11.2015 19:06]:
> > No more panics but it seems the bridge filter rules work different
> > than before. Will test further and report back.

Thanks I committed the fix to -current.

> It seems the order of interface evaluation changed in 5.8. If a vlan
> parent interface is on a bridge, vlan tagged packets get blocked by
> bridge filter rules on the parent interface.

Yes, as soon as a interface is part of a bridge(4), the packets it
receives will be feed to this bridge before doing any processing.

Why do you need the parent interface and the vlan interface on your
bridge?

Re: 5.8 panic with vlan on bridge

[Martin Pieuchot]

On 03/11/15(Tue) 17:03, Armin Wolfermann wrote:
> * Martin Pieuchot  [03.11.2015 13:51]:
> > > It seems the order of interface evaluation changed in 5.8. If a vlan
> > > parent interface is on a bridge, vlan tagged packets get blocked by
> > > bridge filter rules on the parent interface.
> > 
> > Yes, as soon as a interface is part of a bridge(4), the packets it
> > receives will be feed to this bridge before doing any processing.
> 
> But now the bridge filter rules affect all packets - even vlan tagged
> packets destined for another interface. The vlan interface doesn't even
> need to be part of the same bridge.

So you want to keep your vlan packets outside of the bridge?

> > Why do you need the parent interface and the vlan interface on your
> > bridge?
> 
> It is a boot server for dump devices on different vlans.

Diff below should restore the previous behavior as long as bridge is
configured last on your interface (which is the case in netstart(8)).

I'm not sure it applies on 5.8 though...

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.402
diff -u -p -r1.402 if.c
--- net/if.c3 Nov 2015 12:25:37 -   1.402
+++ net/if.c3 Nov 2015 16:41:31 -
@@ -791,17 +791,6 @@ if_input_process(void *xmq)
continue;
}
 
-#if NBRIDGE > 0
-   if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
-   m = bridge_input(ifp, m);
-   if (m == NULL) {
-   if_put(ifp);
-   continue;
-   }
-   }
-   m->m_flags &= ~M_PROTO1;/* Loop prevention */
-#endif
-
/*
 * Pass this mbuf to all input handlers of its
 * interface until it is consumed.
Index: net/if_bridge.c
===
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.268
diff -u -p -r1.268 if_bridge.c
--- net/if_bridge.c 12 Oct 2015 10:03:25 -  1.268
+++ net/if_bridge.c 3 Nov 2015 16:45:51 -
@@ -114,6 +114,7 @@
 
 void   bridgeattach(int);
 intbridge_ioctl(struct ifnet *, u_long, caddr_t);
+intbridge_input(struct ifnet *, struct mbuf *, void *);
 void   bridge_start(struct ifnet *);
 void   bridge_process(struct ifnet *, struct mbuf *);
 void   bridgeintr_frame(struct bridge_softc *, struct ifnet *, struct mbuf *);
@@ -270,6 +271,7 @@ bridge_delete(struct bridge_softc *sc, s
p->ifp->if_bridgeport = NULL;
error = ifpromisc(p->ifp, 0);
 
+   if_ih_remove(p->ifp, bridge_input, NULL);
TAILQ_REMOVE(>sc_iflist, p, next);
bridge_rtdelete(sc, p->ifp, 0);
bridge_flushrule(p);
@@ -388,6 +390,7 @@ bridge_ioctl(struct ifnet *ifp, u_long c
SIMPLEQ_INIT(>bif_brlin);
SIMPLEQ_INIT(>bif_brlout);
ifs->if_bridgeport = (caddr_t)p;
+   if_ih_insert(p->ifp, bridge_input, NULL);
TAILQ_INSERT_TAIL(>sc_iflist, p, next);
break;
case SIOCBRDGDEL:
@@ -1266,14 +1269,19 @@ bridgeintr_frame(struct bridge_softc *sc
  * Receive input from an interface.  Queue the packet for bridging if its
  * not for us, and schedule an interrupt.
  */
-struct mbuf *
-bridge_input(struct ifnet *ifp, struct mbuf *m)
+int
+bridge_input(struct ifnet *ifp, struct mbuf *m, void *cookie)
 {
-   if ((m->m_flags & M_PKTHDR) == 0)
-   panic("bridge_input(): no HDR");
+   KASSERT(m->m_flags & M_PKTHDR);
+
+   if (m->m_flags & M_PROTO1) {
+   m->m_flags &= ~M_PROTO1;
+   return (0);
+   }
 
niq_enqueue(, m);
-   return (NULL);
+
+   return (1);
 }
 
 void
Index: net/if_bridge.h
===
RCS file: /cvs/src/sys/net/if_bridge.h,v
retrieving revision 1.45
diff -u -p -r1.45 if_bridge.h
--- net/if_bridge.h 24 Aug 2015 21:28:47 -  1.45
+++ net/if_bridge.h 3 Nov 2015 16:38:49 -
@@ -436,7 +436,6 @@ struct bridge_softc {
 extern const u_int8_t bstp_etheraddr[];
 
 void   bridge_ifdetach(struct ifnet *);
-struct mbuf *bridge_input(struct ifnet *, struct mbuf *);
 intbridge_output(struct ifnet *, struct mbuf *, struct sockaddr *,
 struct rtentry *);
 void   bridge_update(struct ifnet *, struct ether_addr *, int);
Index: netinet/ip_ether.c
===
RCS file: /cvs/src/sys/netinet/ip_ether.c,v
retrieving revision 1.78
diff -u -p -r1.78 ip_ether.c
--- netinet/ip_ether.c  31 Jul 2015 15:38:10 -  1.78
+++ netinet/ip_ether.c  3 Nov 2015 16:45:59 -
@@ -164,7 +164,7 @@ etherip_decap(struct mbuf *m, int iphlen
 {
struct etherip_header eip;
struct gif_softc *sc;
-   int s;
+   struct mbuf_list ml 

Re: 5.8 panic with vlan on bridge

[Armin Wolfermann]

* Martin Pieuchot  [03.11.2015 13:51]:
> > It seems the order of interface evaluation changed in 5.8. If a vlan
> > parent interface is on a bridge, vlan tagged packets get blocked by
> > bridge filter rules on the parent interface.
> 
> Yes, as soon as a interface is part of a bridge(4), the packets it
> receives will be feed to this bridge before doing any processing.

But now the bridge filter rules affect all packets - even vlan tagged
packets destined for another interface. The vlan interface doesn't even
need to be part of the same bridge.

> Why do you need the parent interface and the vlan interface on your
> bridge?

It is a boot server for dump devices on different vlans.

Re: 5.8 panic with vlan on bridge

[Armin Wolfermann]

* Martin Pieuchot  [02.11.2015 16:28]:
> On 30/10/15(Fri) 23:30, Armin Wolfermann wrote:
> > The simplest way to reproduce:
> > 
> >   ifconfig em0 192.168.1.1 up
> >   ifconfig vlan10 vlan 10 vlandev em0 up
> >   ifconfig bridge0 add em0 add vlan10 up
> 
> Thanks for your report, could you tell me if the diff below helps?
> 
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.401
> diff -u -p -r1.401 if.c
> --- net/if.c  2 Nov 2015 14:40:09 -   1.401
> +++ net/if.c  2 Nov 2015 15:26:40 -
> @@ -564,7 +564,6 @@ if_enqueue(struct ifnet *ifp, struct mbu
>  #if NBRIDGE > 0
>   if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0)
>   return (bridge_output(ifp, m, NULL, NULL));
> - m->m_flags &= ~M_PROTO1;/* Loop prevention */
>  #endif
>  
>   length = m->m_pkthdr.len;
> 

No more panics but it seems the bridge filter rules work different
than before. Will test further and report back.

Re: 5.8 panic with vlan on bridge

[Martin Pieuchot]

On 30/10/15(Fri) 23:30, Armin Wolfermann wrote:
> The simplest way to reproduce:
> 
>   ifconfig em0 192.168.1.1 up
>   ifconfig vlan10 vlan 10 vlandev em0 up
>   ifconfig bridge0 add em0 add vlan10 up

Thanks for your report, could you tell me if the diff below helps?

Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.401
diff -u -p -r1.401 if.c
--- net/if.c2 Nov 2015 14:40:09 -   1.401
+++ net/if.c2 Nov 2015 15:26:40 -
@@ -564,7 +564,6 @@ if_enqueue(struct ifnet *ifp, struct mbu
 #if NBRIDGE > 0
if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0)
return (bridge_output(ifp, m, NULL, NULL));
-   m->m_flags &= ~M_PROTO1;/* Loop prevention */
 #endif
 
length = m->m_pkthdr.len;

5.8 panic with vlan on bridge

[Armin Wolfermann]

The simplest way to reproduce:

  ifconfig em0 192.168.1.1 up
  ifconfig vlan10 vlan 10 vlandev em0 up
  ifconfig bridge0 add em0 add vlan10 up

booting hd0a:/bsd: 6683804+2138976+254984+0+598016 [72+563184+374279]=0xa210c0
entry point at 0x1000160 [7205c766, 3404, 24448b12, 4060a304]
[ using 938176 bytes of bsd ELF symbol table ]
Copyright (c) 1982, 1986, 1989, 1991, 1993
The Regents of the University of California.  All rights reserved.
Copyright (c) 1995-2015 OpenBSD. All rights reserved.  http://www.OpenBSD.org

OpenBSD 5.8 (GENERIC.MP) #1236: Sun Aug 16 02:31:04 MDT 2015
dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
real mem = 2119380992 (2021MB)
avail mem = 2051313664 (1956MB)
mpath0 at root
scsibus0 at mpath0: 256 targets
mainbus0 at root
bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xe3590 (23 entries)
bios0: vendor Intel Corp. version "LF94510J.86A.0103.2008.0814.1910" date 
08/14/2008
bios0: Intel Corporation D945GCLF
acpi0 at bios0: rev 0
acpi0: sleep states S0 S1 S3 S4 S5
acpi0: tables DSDT FACP APIC WDDT MCFG ASF!
acpi0: wakeup devices SLPB(S4) P32_(S4) UAR1(S4) UAR2(S4) PEX0(S4) PEX1(S4) 
PEX2(S4) PEX3(S4) PEX4(S4) PEX5(S4) UHC1(S3) UHC2(S3) UHC3(S3) UHC4(S3) 
EHCI(S3) AC9M(S4) [...]
acpitimer0 at acpi0: 3579545 Hz, 24 bits
acpimadt0 at acpi0 addr 0xfee0: PC-AT compat
cpu0 at mainbus0: apid 0 (boot processor)
cpu0: Intel(R) Atom(TM) CPU 230 @ 1.60GHz, 1596.33 MHz
cpu0: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,TM2,SSSE3,CX16,xTPR,PDCM,MOVBE,NXE,LONG,LAHF,PERF,SENSOR
cpu0: 512KB 64b/line 16-way L2 cache
cpu0: smt 0, core 0, package 0
mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges
cpu0: apic clock running at 133MHz
cpu0: mwait min=64, max=64, C-substates=0.1, IBE
cpu1 at mainbus0: apid 1 (application processor)
cpu1: Intel(R) Atom(TM) CPU 230 @ 1.60GHz, 1596.12 MHz
cpu1: 
FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,DTES64,MWAIT,DS-CPL,TM2,SSSE3,CX16,xTPR,PDCM,MOVBE,NXE,LONG,LAHF,PERF,SENSOR
cpu1: 512KB 64b/line 16-way L2 cache
cpu1: smt 1, core 0, package 0
ioapic0 at mainbus0: apid 2 pa 0xfec0, version 20, 24 pins
ioapic0: misconfigured as apic 0, remapped to apid 2
acpimcfg0 at acpi0 addr 0xf000, bus 0-127
acpiprt0 at acpi0: bus 0 (PCI0)
acpiprt1 at acpi0: bus 4 (P32_)
acpiprt2 at acpi0: bus 1 (PEX0)
acpiprt3 at acpi0: bus -1 (PEX1)
acpiprt4 at acpi0: bus 2 (PEX2)
acpiprt5 at acpi0: bus 3 (PEX3)
acpiprt6 at acpi0: bus -1 (PEX4)
acpiprt7 at acpi0: bus -1 (PEX5)
acpicpu0 at acpi0: C1(@1 halt!)
acpicpu1 at acpi0: C1(@1 halt!)
acpibtn0 at acpi0: SLPB
pci0 at mainbus0 bus 0
pchb0 at pci0 dev 0 function 0 "Intel 82945G Host" rev 0x02
vga1 at pci0 dev 2 function 0 "Intel 82945G Video" rev 0x02
intagp0 at vga1
agp0 at intagp0: aperture at 0x8000, size 0x800
inteldrm0 at vga1
drm0 at inteldrm0
inteldrm0: 1280x1024
wsdisplay0 at vga1 mux 1: console (std, vt100 emulation)
wsdisplay0: screen 1-5 added (std, vt100 emulation)
azalia0 at pci0 dev 27 function 0 "Intel 82801GB HD Audio" rev 0x01: msi
azalia0: codecs: Realtek ALC662
audio0 at azalia0
ppb0 at pci0 dev 28 function 0 "Intel 82801GB PCIE" rev 0x01: msi
pci1 at ppb0 bus 1
1:0:0: mem address conflict 0xfffe/0x2
re0 at pci1 dev 0 function 0 "Realtek 8101E" rev 0x02: RTL8102EL (0x2480), msi, 
address 00:1c:c0:6e:a5:7f
rlphy0 at re0 phy 7: RTL8201L 10/100 PHY, rev. 1
ppb1 at pci0 dev 28 function 2 "Intel 82801GB PCIE" rev 0x01: msi
pci2 at ppb1 bus 2
ppb2 at pci0 dev 28 function 3 "Intel 82801GB PCIE" rev 0x01: msi
pci3 at ppb2 bus 3
uhci0 at pci0 dev 29 function 0 "Intel 82801GB USB" rev 0x01: apic 2 int 23
uhci1 at pci0 dev 29 function 1 "Intel 82801GB USB" rev 0x01: apic 2 int 19
uhci2 at pci0 dev 29 function 3 "Intel 82801GB USB" rev 0x01: apic 2 int 16
ehci0 at pci0 dev 29 function 7 "Intel 82801GB USB" rev 0x01: apic 2 int 23
usb0 at ehci0: USB revision 2.0
uhub0 at usb0 "Intel EHCI root hub" rev 2.00/1.00 addr 1
ppb3 at pci0 dev 30 function 0 "Intel 82801BA Hub-to-PCI" rev 0xe1
pci4 at ppb3 bus 4
em0 at pci4 dev 0 function 0 "Intel 82541GI" rev 0x05: apic 2 int 21, address 
00:0e:0c:75:ee:fe
pcib0 at pci0 dev 31 function 0 "Intel 82801GB LPC" rev 0x01
pciide0 at pci0 dev 31 function 1 "Intel 82801GB IDE" rev 0x01: DMA, channel 0 
configured to compatibility, channel 1 configured to compatibility
atapiscsi0 at pciide0 channel 0 drive 0
scsibus1 at atapiscsi0: 2 targets
cd0 at scsibus1 targ 0 lun 0:  ATAPI 5/cdrom 
removable
cd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 2
pciide0: channel 1 ignored (disabled)
pciide1 at pci0 dev 31 function 2 "Intel 82801GB SATA" rev 0x01: DMA, channel 0 
configured to native-PCI, channel 1 configured to native-PCI
pciide1: using apic 2 int 19 for native-PCI interrupt
wd0 at pciide1 channel 0 drive 0: 
wd0: 16-sector PIO, LBA48,