Re: kern/51280: if_srt fails to correctly route IPv6 packets (+ potential fix)

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Mon, Jun 27, 2016 at 10:20 AM,  <kre%munnari.oz.au@localhost> wrote:
>>Number:         51280
>>Category:       kern
>>Synopsis:       if_srt fails to correctly route IPv6 packets (+ potential fix)
>>Confidential:   no
>>Severity:       serious
>>Priority:       medium
>>Responsible:    kern-bug-people
>>State:          open
>>Class:          sw-bug
>>Submitter-Id:   net
>>Arrival-Date:   Mon Jun 27 01:20:01 +0000 2016
>>Originator:     Robert Elz
>>Release:        NetBSD 7.99.32 (and everything since srt(4) was added)
>>Organization:
>>Environment:
>         This is the system sending the PR ...  Not useful info...
> System: NetBSD andromeda.noi.kre.to 7.99.26 NetBSD 7.99.26 (VBOX64-1.1-20160128) #43: Thu Jan 28 16:09:08 ICT 2016 kre%onyx.coe.psu.ac.th@localhost:/usr/obj/current/kernels/amd64/VBOX64 amd64
> Architecture: x86_64
> Machine: amd64
>>Description:
>         if_srt (srt(4)) enables routing based upon the source address
>         of the packets - when packets are routed to it (via normal routing
>         mechanisms, usually a default route aimed at the srt interface)
>         it matches the source addr against a configured list of (addr,prefixlen)
>         pairs and if a match is found, sends the packet a configured dest
>         addr via another interface.
>
>         For ipv4 sending a packet is done by calling the interface
>         ip_output routine, and that is what if_srt does.   But for
>         ipv6 packet output is done by sending the packet via nd6_output()
>         which then calls the ip_output routine after naighbour discovery
>         is performed.  if_srt doesn't do that.
>
>         Once that is fixed, there is another issue ... ipv6 routes are
>         generally via link local addresses (for many reasons not relevant
>         here).  In my case, on a system with 2 interfaces (not a router,
>         just a multi-homed host) the router on one of the links has no
>         global addr available to use, only link local.   The issue
>         here is that in the KAME IPv6 stack, inside the kernel, link
>         local addresses have the interface-id embedded in them (a truly
>         ugly hack) and Neighbour Discovery expects link local addresses
>         it sees to contain that embedded scope-id (interface id), and
>         fails if it is not present.   Since the interface id is not
>         really exposed to userland, it would be difficult to have the
>         srt config tool (srtconfig(8)) embed that ID in link local
>         addresses it configures - and in any case, this is an in-kernel
>         hack, and should be confined there.
>
>         Hence we also need a fix to detect when a srt destination address
>         is link local, and embed the scope-id in the address when it is.
>
>>How-To-Repeat:
>         Attempt to use srt to source route IPv6 packets (required in the
>         setup I have, as without it, packets addressed to the node via
>         one of its addresses, coming from random internet sources, all
>         get replies sent via the default interface (the interface to
>         which the default route is attached).   In general this works,
>         it produces (slightly) asymmetric routing, but that's not all
>         that unusual.   Unfortunately, there are firewalls (outside my
>         contro) on the links, and they do not at all like seeing just
>         half of a TCP connection - and cause such things to fail.
>
>         Watch it fail...

If there is a list of commands to reproduce, that's helpful.

>
>>Fix:
>         The patch below is intended for review by those more knowledgeable
>         of the IPv6 stack (and NetBSD networking in general) than I am.
>
>         There are 3 segments to the patch.
>
>         The first just adds the necessary include files to make the
>         rest of it compile...
>
>         The second runs nd6_output() instead if ifp->if_output (via the
>         new locking interface) for IPv6 packets (the locking stuff then
>         gets done in nd6_output).   This piece I am reasonably confident
>         is correct (the XXX comment that just precedes the change applies
>         to both output calls now.)
>
>         The third does the link local address embedding stuff.  I know
>         it is only half done (in the sense, that the embedded scope remains
>         visible when the config tool requests the current config and displays
>         it - that doesn't stop anything working, and it makes it easier to
>         debug the rest of this, though it is not really correct, and is ugly)
>         and also is probably not really the right way to do this - rather than
>         just using the if_index as the scope id (or zone id, this thing seems
>         unsure what it really should be called) it should probably really
>         use the data explicitly added to each interface to contain such things,
>         but for link locals, the zone id and interface index are always the
>         same, and link locals (or multicast link locals, but a multicast
>         dest addr makes no sense) are the only address forms that
>         in6_setzoneid() modifies, so the shortcut seems reasonable to me.
>
>
> Index: if_srt.c
> ===================================================================
> RCS file: /cvsroot/src/sys/net/if_srt.c,v
> retrieving revision 1.22
> diff -u -r1.22 if_srt.c
> --- if_srt.c    20 Jun 2016 06:46:37 -0000      1.22
> +++ if_srt.c    27 Jun 2016 00:29:36 -0000
> @@ -33,6 +33,9 @@
>  #include <sys/ioctl.h>
>  #include <netinet/ip.h>
>  #include <netinet/ip6.h>
> +#include <netinet6/in6_var.h>
> +#include <netinet6/nd6.h>
> +#include <netinet6/scope6_var.h>
>  #include <net/if_types.h>
>
>  #include "if_srt.h"
> @@ -232,6 +235,8 @@
>                 return 0; /* XXX ENETDOWN? */
>         }
>         /* XXX is 0 the right last arg here? */
> +       if (to->sa_family == AF_INET6)
> +               return nd6_output(r->u.dstifp, r->u.dstifp, m, &r->dst.sin6, 0);

I think it's okay. (Though it might be better we make if_output_lock
work for IPv6 as well somehow. Both FreeBSD and OpenBSD do so now.)

>         return if_output_lock(r->u.dstifp, r->u.dstifp, m, &r->dst.sa, 0);
>  }
>
> @@ -432,6 +437,8 @@
>                 scr->srcmask = dr->srcmask;
>                 scr->u.dstifp = ifp;
>                 memcpy(&scr->dst,&dr->dst,dr->dst.sa.sa_len);
> +               if (dr->af == AF_INET6)
> +                       in6_setzoneid(&scr->dst.sin6.sin6_addr, ifp->if_index);

in6_setscope may suit more?

And I think both pieces need #ifdef INET6.

  ozaki-r

>                 update_mtu(sc);
>                 return 0;
>         case SRT_DELRT:
>