OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

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

OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

Eygene Ryabinkin
Good day.

The attached patch fixes incoming interface for pf(4) processing
in the case of bridging of multiple VLAN interfaces which have
the same parent iface and unicast packets destined to the bridge
member: we can't rely solely on the destination MAC in determining
proper incoming interface.

Can it be reviewed and possibly applied?  It was verified in my
production environment.

Thanks.
--
rea

0001-bridge-4-filter-locally-destined-packets-on-the-prop.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

Martin Pieuchot
On 07/06/19(Fri) 20:50, Eygene Ryabinkin wrote:

> Good day.
>
> The attached patch fixes incoming interface for pf(4) processing
> in the case of bridging of multiple VLAN interfaces which have
> the same parent iface and unicast packets destined to the bridge
> member: we can't rely solely on the destination MAC in determining
> proper incoming interface.
>
> Can it be reviewed and possibly applied?  It was verified in my
> production environment.

Are you suggesting that the order in which bridge members are checked
matters?  In that case would it makes sense to apply the so-called
unicast check to `ifp', the incoming interface, first?


> From b7c4009fc4e85acc7a99a6a5b8a5fbf283137612 Mon Sep 17 00:00:00 2001
> From: Eygene Ryabinkin <[hidden email]>
> Date: Thu, 6 Jun 2019 09:40:06 +0300
> Subject: [PATCH 1/2] bridge(4): filter locally-destined packets on the proper
>  interface
>
> When vlan(4) interfaces are used as the bridge members, they share
> the MAC of their parent ifs.  Thus, when multiple VLANs with the
> same parent interface are bridged (for filtering), they will all have
> the same MAC and MAC-based strategy for indentification
> of locally-destined traffic may pick the wrong interface for pf(4)
> processing.  Actually, it will use the most recently added interface
> with the packet's destination MAC.
>
> As we know the incoming interface for bridge_process(), check
> if it has the packet's destination MAC and use it for bridge_input()
> when the check succeeds.
>
> Signed-off-by: Eygene Ryabinkin <[hidden email]>
> ---
>  sys/net/if_bridge.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/sys/net/if_bridge.c b/sys/net/if_bridge.c
> index 1d178dc4d22..d64bdc05abb 100644
> --- a/sys/net/if_bridge.c
> +++ b/sys/net/if_bridge.c
> @@ -1217,7 +1217,13 @@ bridge_process(struct ifnet *ifp, struct mbuf *m)
>   brifp->if_ipackets++;
>   brifp->if_ibytes += m->m_pkthdr.len;
>  
> - ifp = bif->ifp;
> + /*
> + * Give the original input interface
> + * a chance to be the input one.
> + */
> + if (!bridge_ourether(ifp, eh->ether_dhost)) {
> + ifp = bif->ifp;
> + }
>   goto reenqueue;
>   }
>   if (bridge_ourether(bif->ifp, eh->ether_shost))
> --
> 2.21.0
>

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

Eygene Ryabinkin
Martin, good day.

Sun, Jun 09, 2019 at 11:44:10AM -0300, Martin Pieuchot wrote:

> On 07/06/19(Fri) 20:50, Eygene Ryabinkin wrote:
> > The attached patch fixes incoming interface for pf(4) processing
> > in the case of bridging of multiple VLAN interfaces which have
> > the same parent iface and unicast packets destined to the bridge
> > member: we can't rely solely on the destination MAC in determining
> > proper incoming interface.
> >
> > Can it be reviewed and possibly applied?  It was verified in my
> > production environment.
>
> Are you suggesting that the order in which bridge members are checked
> matters?
Yes.

> In that case would it makes sense to apply the so-called unicast
> check to `ifp', the incoming interface, first?

Right.  Had rewritten the patch to do this explicitely: +1 goto,
-1 (I hope) confusion point about ifp setting.

The bridge with updated patch works as expected already for some hours
in production: will continue to monitor it.
--
rea

0001-bridge-4-filter-locally-destined-packets-on-the-prop.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

Eygene Ryabinkin
Mon, Jun 17, 2019 at 04:15:10PM +0300, Eygene Ryabinkin wrote:
> Sun, Jun 09, 2019 at 11:44:10AM -0300, Martin Pieuchot wrote:
> > In that case would it makes sense to apply the so-called unicast
> > check to `ifp', the incoming interface, first?
>
> Right.  Had rewritten the patch to do this explicitely: +1 goto,
> -1 (I hope) confusion point about ifp setting.
>
> The bridge with updated patch works as expected already for some hours
> in production: will continue to monitor it.

No issues up to date.

Ping!  Pong?
--
rea

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

Eygene Ryabinkin
Mon, Jun 24, 2019 at 11:43:15AM +0300, Eygene Ryabinkin wrote:

> Mon, Jun 17, 2019 at 04:15:10PM +0300, Eygene Ryabinkin wrote:
> > Sun, Jun 09, 2019 at 11:44:10AM -0300, Martin Pieuchot wrote:
> > > In that case would it makes sense to apply the so-called unicast
> > > check to `ifp', the incoming interface, first?
> >
> > Right.  Had rewritten the patch to do this explicitely: +1 goto,
> > -1 (I hope) confusion point about ifp setting.
> >
> > The bridge with updated patch works as expected already for some hours
> > in production: will continue to monitor it.
>
> No issues up to date.
>
> Ping!  Pong?

Every Monday he pinged tech@OpenBSD...
--
rea

Reply | Threaded
Open this post in threaded view
|

Re: OpenBSD: patch for bridge(4) to fix incoming interface for pf(4)

Martin Pieuchot
In reply to this post by Eygene Ryabinkin
On 17/06/19(Mon) 16:15, Eygene Ryabinkin wrote:

> Sun, Jun 09, 2019 at 11:44:10AM -0300, Martin Pieuchot wrote:
> > On 07/06/19(Fri) 20:50, Eygene Ryabinkin wrote:
> > > The attached patch fixes incoming interface for pf(4) processing
> > > in the case of bridging of multiple VLAN interfaces which have
> > > the same parent iface and unicast packets destined to the bridge
> > > member: we can't rely solely on the destination MAC in determining
> > > proper incoming interface.
> > >
> > > Can it be reviewed and possibly applied?  It was verified in my
> > > production environment.
> >
> > Are you suggesting that the order in which bridge members are checked
> > matters?
>
> Yes.
>
> > In that case would it makes sense to apply the so-called unicast
> > check to `ifp', the incoming interface, first?
>
> Right.  Had rewritten the patch to do this explicitely: +1 goto,
> -1 (I hope) confusion point about ifp setting.
>
> The bridge with updated patch works as expected already for some hours
> in production: will continue to monitor it.

Diff is correct.  The problem with using a goto here is that `bif->ifp'
is only valid inside the loop.  At least that how it will become once
the SMR_LIST is converted to a no-locked variant.  That's what I have
in my tree.

So I'd suggest we move the bridge_ifinput() call inside the loop and
duplicate the logic for `ifp0'.

In order to reduce the amount of duplicated logic we could start by
moving the filter above.  This means that bridge_rtupdate() won't be
executed on blocked packet, which should be fine.

Are you ok with the diff below?

Index: net/if_bridge.c
===================================================================
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.335
diff -u -p -r1.335 if_bridge.c
--- net/if_bridge.c 9 Jun 2019 17:42:16 -0000 1.335
+++ net/if_bridge.c 10 Jul 2019 17:19:24 -0000
@@ -1014,10 +1014,6 @@ bridgeintr_frame(struct ifnet *brifp, st
  return;
  }
 
- if (bridge_filterrule(&bif->bif_brlin, &eh, m) == BRL_ACTION_BLOCK) {
- m_freem(m);
- return;
- }
  m = bridge_ip(&sc->sc_if, BRIDGE_IN, src_if, &eh, m);
  if (m == NULL)
  return;
@@ -1197,6 +1193,9 @@ bridge_process(struct ifnet *ifp, struct
  return;
  }
 
+ if (bridge_filterrule(&bif->bif_brlin, eh, m) == BRL_ACTION_BLOCK)
+ goto bad;
+
  /*
  * Unicast, make sure it's not for us.
  */
@@ -1207,11 +1206,6 @@ bridge_process(struct ifnet *ifp, struct
  bridge_rtupdate(sc,
     (struct ether_addr *)&eh->ether_shost,
     ifp, 0, IFBAF_DYNAMIC, m);
- if (bridge_filterrule(&bif0->bif_brlin, eh, m) ==
-    BRL_ACTION_BLOCK) {
-     goto bad;
- }
-
  /* Count for the bridge */
  brifp->if_ipackets++;
  brifp->if_ibytes += m->m_pkthdr.len;