Re: [Openvpn-devel] [PATCH v2] Drop recursively routed packets

[Lev S. <lst...@gm...>]

Hi,

I could probably add that patch has been in our production for a while
(several months), OS X / Win, works as expected.

-Lev

2016-08-13 12:58 GMT+03:00 Gert Doering <ge...@gr...>:

> Hi,
>
> On Fri, Aug 12, 2016 at 08:24:35PM +0200, David Sommerseth wrote:
> > On 04/01/16 13:43, Lev Stipakov wrote:
> > > v2: better method naming
> > >
> > > On certain OSes (Windows, OS X) when network adapter is disabled
> > > (ethernet cable pulled off, Wi-Fi hardware switch disabled),
> > > operating system starts to use tun as an external interface.
> > > Outgoing packets are routed to tun, UDP encapsulated, given to
> > > routing table and sent to.. tun.
> >
> > Can this happen on Linux or *BSD?  Could this recursive routing
> > actually happen at all?
>
> Of course :-) - the moment you lose your external interface, for
> example, because you unplug the network cable and network manager
> deconfigures ip address plus all routes via that interface...
>
> What is left is "routes into the tunnel", so you send the tunneled
> packets into the tunnel again.  Without that patch, this leads to
> "openvpn eats 100% cpu, your VPN is down, and until ping-timeout
> fires, it won't recover and the user has no idea why it is broken".
>
> Very typical problem with routing and tunnels :-)
>
> >
> > > + +      /* skip ipv4 packets for ipv6 tun */ +      if
> > > (tun_sa.addr.sa.sa_family != AF_INET) +        return;
> >
> > [...snip...]
> >
> > > + +      /* skip ipv6 packets for ipv4 tun */ +      if
> > > (tun_sa.addr.sa.sa_family != AF_INET6) +      return;
> >
> > How likely is it that these two checks will hit?  Do we truly need
> > them?
>
> It's quite likely for IPv6 packets to be transported over an IPv4
> tunnel, and vice versa, and will be even *more* likely for the next
> 10 years to come.
>
> So comparing "inside IPv6 address" to "outside IPv4 address" needs
> to be avoided (at best, the code might misfire, at worst, you might
> overrun comparing an IPv6 address to something that has not allocated
> enough bytes) - maybe it can be written more efficient, but in the end
> you always have the "is it the same protocol inside and outside?"
> check, before you go on comparing addresses.
>
> Right now, the code is nicely readable by using the is_ipv4() macro
> etc., but sacrificing this by having a check very early on that
> is the unrolled form of the macro, and basically does
>
>   if ( outside_address_family != inside_address_family ) { return; }
>
> would indeed be more efficient for the mixed-family case.
>
> OTOH, you need another comparison then for "is it IPv4?  is it IPv6?",
> so the overall amount of code for the same-family case might be even
> higher.
>
> OTOH again, is_ipv4() boils down to an amazing amount of code...  so
> moving *that* from proto.c to proto.h and make it an inline function
> (and hope that the compiler will be smart enough to keep track of all
> the then-duplicate checks for is_ipv4() vs. is_ipv6() and just optimize
> it properly) might produce more efficient *and* more readable code :-)
> (and since it's only called in two places ever, the extra code size
> due to inlining isn't big)
>
>
> > I'm basically concerned of the potential cost an extra check
> > adds.  As the link speed increases, such checks will have an impact
> > later on if now.  If truly needed, is it worth looking into providing
> > some branch prediction hints to the compiler?  (unlikely/likely hints)
>
> For the "protocol variants", you can't, because it depends on what
> users do with their tunnels.  Today, they might be predominantly
> IPv4-over-IPv4, while my personal sessions are mostly IPv6-over-IPv4
> already...
>
> > > if (c->c2.buf.len > 0) { +      drop_if_recursive_routing (c,
> > > &c->c2.buf);
> >
> > If this is truly only an issue on Windows and OSX, can we consider to
> > #ifdef the drop_if_recursive_routing() call?
>
> It can happen on all platforms where "inside" and "outside" routing
> (wrt openvpn) happens in the same routing context / namespace / ... and
> "some external entity" can modify the "outside" routing table without
> OpenVPN knowing.  In other words: on all platforms we support today.
>
> gert
> --
> USENET is *not* the non-clickable part of WWW!
>                                                            //
> www.muc.de/~gert/
> Gert Doering - Munich, Germany
> ge...@gr...
> fax: +49-89-35655025                        ge...@ne...rmatik.tu-
> muenchen.de
>



-- 
-Lev