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

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

The following reply was made to PR kern/51280; it has been noted by GNATS.

From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
To: "gnats-bugs%NetBSD.org@localhost" <gnats-bugs%netbsd.org@localhost>
Cc: kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/51280: if_srt fails to correctly route IPv6 packets (+
 potential fix)
Date: Wed, 6 Jul 2016 19:13:22 +0900

 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:
 >