NetBSD Problem Report #51280

From kre@munnari.OZ.AU  Mon Jun 27 01:18:43 2016
Return-Path: <kre@munnari.OZ.AU>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id AE78F7A474
	for <gnats-bugs@www.NetBSD.org>; Mon, 27 Jun 2016 01:18:43 +0000 (UTC)
Message-Id: <201606270118.u5R1ISVG000088@andromeda.noi.kre.to>
Date: Mon, 27 Jun 2016 08:18:28 +0700 (ICT)
From: kre@munnari.OZ.AU
To: gnats-bugs@www.NetBSD.org
Subject: if_srt fails to correctly route IPv6 packets (+ potential fix)
X-Send-Pr-Version: 3.95

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jun 27 01:20:01 +0000 2016
>Closed-Date:    Thu Feb 09 11:44:58 +0000 2017
>Last-Modified:  Thu Feb 09 11:45:00 +0000 2017
>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:/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...

>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);
 	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);
 		update_mtu(sc);
 		return 0;
 	case SRT_DELRT:

>Release-Note:

>Audit-Trail:
From: Ryota Ozaki <ozaki-r@netbsd.org>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
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> 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:/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:
 >

From: Robert Elz <kre@munnari.OZ.AU>
To: Ryota Ozaki <ozaki-r@netbsd.org>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>,
        kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/51280: if_srt fails to correctly route IPv6 packets (+ potential fix)
Date: Thu, 07 Jul 2016 01:18:13 +0700

     Date:        Wed, 6 Jul 2016 19:13:22 +0900
     From:        Ryota Ozaki <ozaki-r@netbsd.org>
     Message-ID:  <CAKrYomh7CUGzKp=vL+-y2fkxzm-HOF0Tp+cghyKgG2zK2KPsdA@mail.gmail.com>

   | >         Watch it fail...
   | 
   | If there is a list of commands to reproduce, that's helpful.

 Anything at all that uses it, but the config I used for testing is:

 (first, build a kernel with if_srt included, it is not in amd64 GENERIC)
 Also, make /dev/srt0 (which needs a manual mknod as srt is not in MAKEDEV
 either...)   "mknod /dev/srt0 c 179 0" works on amd64 (and probably everywhere.)

 Then /etc/ifconfig.srt0 contains ...

   create
   inet6 fe80::1%srt0
   up

 (without the leading spaces, they're just for this e-mail)

 ifconfig.xennet0 (this is being tested in a XEN DomU) contains ...

   inet6 fe80::/64 eui64
   up
   inet6 2001:3c8:9009:181::/64 eui64
   ! /sbin/ifconfig srt0 create 2>/dev/null || true
   ! /usr/sbin/srtconfig srt0 add 2001:3c8:9009:: /48 xennet0 2001:3c8:9009:181::1

 (the test system has one big root filesystem containing everything, so
 /usr/sbin is available when the network is being configured ... srtconfig
 probably really needs to be in /bin though, without it, there might be no
 workable way to get to a networked /usr in some environments.)(

 2001:3c8:9009:181::1 is the address of the router on xennet0 (this router
 has a global address, so I used it ... even though routing via LL addresses
 is generally preferred.)

 ifconfig.xennet0 contains ...

   inet6 fe80::/64 eui64
   up
   inet6 2001:3c8:9007:1::/64 eui64
   ! /sbin/ifconfig srt0 create 2>/dev/null || true
   ! /usr/sbin/srtconfig srt0 add 2001:3c8:9007:: /48 xennet1 fe80::9007:1:1

 fe80::9007:1:1 is the address (LL address, obviously) of the router on
 zennet1 (this one has (or appears to have) no global address on this
 interface, the LL address was all I could find - I do not control the
 routers.)

 When not using srt0, either of the two router addresses given works as
 a default route, and if the firewalls get turned off, works fine for all
 communications.   When the firewalls (also outside my control) are enabled
 communications using the (local) address on the same interface as the
 default route work fine, communications using the other fail (the assumption
 is that incoming packets come via the direct path to the interface concerned,
 whereas outgoing packets follow the default route - if those are using the
 same interface, all is fine, if not, each firewall sees half the connection,
 and never sees any replies, so kills that connection.)

 To use srt0, rc.conf contains ...

   defaultroute6=fe80::1%srt0

 The extra "ifconfig srt0 create" stuff spread all over the place is
 just because I did not want to depend upon the order in which the
 interfaces are configured.   In practice, the "create" in ifconfig.srt0
 gives an "already happened" error on the console during boot.  That is
 harmless...

 Then to test, first lookup the address of a friendly remote IPv6 capable
 host to which a connection can be made - ftp, smtp, ssh, any IPv6
 connection ... lookup as I didn't bother to config DNS on this test
 host (it has no IPv4 addresses at all, and IPv6 was not working as configured,
 so configuring DNS would have been a waste of time) nor was I intelligent
 enough to put the relevant address in /etc/hosts !)

 So, elsewhere
 	host ftp.netbsd.org
 and observe the line ...
 	ftp.netbsd.org has IPv6 address 2001:470:a085:999::21
 in the output.

 Then on the system using srt0, do...

   ifconfig xennet0

 and observe its assigned global address ...  2001:3c8:9009:181:something

 then:

   ssh -6 -b 2001:3c8:9009:181:something 2001:470:a085:999::21

 Running tcpdump on xennet0 produces no output at all.

 Repeat the same, but binding to xennet1's address, and observe no
 output on xennet1

 Add diagnostic printf's in srt0 and observe that it appears to be doing
 what is intended (it is attempting to send packets via xennet0 or xennet1
 as appropriate, with the correct destination address.)

 With the patch in the PR, both connections work fine, as does "ftp -s ..."
 connections, ping -S ... (and networking in general, I don't think telnet
 has an option to bind the source addr [it probably should] to enable easy
 testing of SMTP, HTTP, etc.)   And that is with the firewalls enabled
 (though without the patch, the firewalls are irrelevant, nothing reached
 the network for the firewall to block when srt0 was being used.)

 Without the scope setting part of the patch, the xennet0 path worked (with
 just the nd6_outpout() patch) using the global addr for the router there,
 but the xennet1 path (with the LL router address) still failed.

 Incoming connections to both global addresses also work after the patch
 is applied.

 I suspect you can see now why I left all of this out of the original PR,
 there's nothing tricky or unusual here, it all just follows the manuals,
 and the obvious thing to do.   It doesn't matter whether the outgoing ssh
 connections actually succeed (whether the local key files are present, etc)
 for this test to demonstrate success/failure (and anon ftp, and ping, both
 work just fine after the patch.)


   | 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.)

 nd6_output() does that I believe.  If nd6_output() itself needs some
 additional locking, I believe it would be better done inside it, rather
 than on all callers (for interface output routines, the current method is
 better, as there are so many of them, and more eppear over time - but there
 is just one nd6_output() that is called several places...)

   | in6_setscope may suit more?

 Yes, perhaps - that is the kind of info I was looking for, though it
 seems perhaps a little over fussy - it calls in6_setzoneid() to actually
 do the work, the rest of it is just working out the zoneid to use.
 It calculates a zoneid for all kinds of addresses, and then calls
 in6_setzoneid() which does precisely nothing for anything except LL
 addresses.   For LL addresses the zoneid it calculates is always the
 interface index (there's no way to cause it to get set to something else)
 so the only real benefit of in6_setscope() is that it returns the zoneid
 (for all address forms) if requested (by giving a non-null pointer for an
 int to put it in.)   if_srt has no use for that info...

 However, it could be done either way, and I will certainly accept advice
 that it really should be in6_setscope() if that is considered better.

   | And I think both pieces need #ifdef INET6.

 Yes, of course, but that ought be a separate commit - there's (lots) more in
 if_srt.c that won't compile if INET6 is not defined (it has some #ifdef INET6
 in it, but not enough...)

 That ought to be done before considering adding if_srt to GENERIC.

 kre

State-Changed-From-To: open->closed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Thu, 09 Feb 2017 11:44:58 +0000
State-Changed-Why:
Patch applied.    This one was sitting idle for too long...


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51280 CVS commit: src/sys/net
Date: Thu, 9 Feb 2017 11:43:32 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Thu Feb  9 11:43:32 UTC 2017

 Modified Files:
 	src/sys/net: if_srt.c

 Log Message:
 PR kern/51280

 This allows srt devices to work for IPv6.   srt still needs work
 (particularly #ifdef INET6 but also general effeciency and similar.)


 To generate a diff of this commit:
 cvs rdiff -u -r1.24 -r1.25 src/sys/net/if_srt.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

>Unformatted:

Home	PR Database Search