From: Alexander Bluhm <bluhm@openbsd.org>
Subject: ip6 forwarding MP consistent read
To: tech@openbsd.org
Date: Mon, 17 Jun 2024 14:19:05 +0200

Hi,

In IPv4 we use IP_FORWARDING to pass down a consistent value of
net.inet.ip.forwarding down the stack.  This is needed for unlocking
sysctl.  I would like to do the same for IPv6.

Read ip6_forwarding once in ip6_input_if() and pass down IPV6_FORWARDING
as flags to ip6_ours(), ip6_hbhchcheck(), ip6_forward().  Replace
the srcrt value with IPV6_REDIRECT flag for consistency with IPv4.

To have consistent syntax with IPv4, use ip6_forwarding == 0 checks
instead of !ip6_forwarding.  This will also make it easier to
implement net.inet6.ip6.forwarding=2 for IPsec only forwarding
later.

There is a little change for IPv4 behavior.  Before we did accept
redirects if ip_forwarding == 2 in icmp_input_if(), now we reject
them for 1 and 2.  Claudio had some concerns here and I think he
is right.  I have to make the same decision for IPv6.

In nd6_ns_input() and nd6_na_input() I read ip6_forwarding once and
store it in i_am_router.  The variable name is chosen to avoid
confusion with is_router, which indicates router flag of the packet.
Reading of ip6_forwarding is done independently from ip6_input_if(),
consistency does not really matter.  One is for ND router befavior
the other for forwarding.  Again I use the ip6_forwarding != 0
check, so when ip6_forwarding IPsec only value 2 gets implemented,
it will behave like a router.

ok?

bluhm

Index: net/if.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
diff -u -p -r1.718 if.c
--- net/if.c	6 Feb 2024 00:18:53 -0000	1.718
+++ net/if.c	17 Jun 2024 11:22:18 -0000
@@ -3378,7 +3378,7 @@ ifnewlladdr(struct ifnet *ifp)
 	 * Update the link-local address.  Don't do it if we're
 	 * a router to avoid confusing hosts on the network.
 	 */
-	if (!ip6_forwarding) {
+	if (ip6_forwarding == 0) {
 		ifa = &in6ifa_ifpforlinklocal(ifp, 0)->ia_ifa;
 		if (ifa) {
 			in6_purgeaddr(ifa);
Index: net/pf.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v
diff -u -p -r1.1197 pf.c
--- net/pf.c	7 Jun 2024 18:24:16 -0000	1.1197
+++ net/pf.c	17 Jun 2024 11:22:18 -0000
@@ -7974,12 +7974,15 @@ done:
 			break;
 		case AF_INET6:
 			if (pd.dir == PF_IN) {
+				int flags;
+
 				if (ip6_forwarding == 0) {
 					ip6stat_inc(ip6s_cantforward);
 					action = PF_DROP;
 					break;
 				}
-				ip6_forward(pd.m, NULL, 1);
+				flags = IPV6_FORWARDING | IPV6_REDIRECT;
+				ip6_forward(pd.m, NULL, flags);
 			} else
 				ip6_output(pd.m, NULL, NULL, 0, NULL, NULL);
 			break;
Index: net/pf_norm.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf_norm.c,v
diff -u -p -r1.230 pf_norm.c
--- net/pf_norm.c	22 Apr 2024 13:30:22 -0000	1.230
+++ net/pf_norm.c	17 Jun 2024 11:57:31 -0000
@@ -1011,7 +1011,7 @@ pf_refragment6(struct mbuf **m0, struct 
 	while ((m = ml_dequeue(&ml)) != NULL) {
 		m->m_pkthdr.pf.flags |= PF_TAG_REFRAGMENTED;
 		if (ifp == NULL) {
-			ip6_forward(m, NULL, 0);
+			ip6_forward(m, NULL, IPV6_FORWARDING);
 		} else if ((u_long)m->m_pkthdr.len <= ifp->if_mtu) {
 			ifp->if_output(ifp, m, sin6tosa(dst), rt);
 		} else {
Index: netinet/ip_carp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v
diff -u -p -r1.361 ip_carp.c
--- netinet/ip_carp.c	13 Feb 2024 12:22:09 -0000	1.361
+++ netinet/ip_carp.c	17 Jun 2024 11:22:18 -0000
@@ -1287,6 +1287,10 @@ carp_send_na(struct carp_softc *sc)
 	struct ifaddr *ifa;
 	struct in6_addr *in6;
 	static struct in6_addr mcast = IN6ADDR_LINKLOCAL_ALLNODES_INIT;
+	int flags = ND_NA_FLAG_OVERRIDE;
+
+	if (ip6_forwarding != 0)
+		flags |= ND_NA_FLAG_ROUTER;
 
 	TAILQ_FOREACH(ifa, &sc->sc_if.if_addrlist, ifa_list) {
 
@@ -1294,9 +1298,7 @@ carp_send_na(struct carp_softc *sc)
 			continue;
 
 		in6 = &ifatoia6(ifa)->ia_addr.sin6_addr;
-		nd6_na_output(&sc->sc_if, &mcast, in6,
-		    ND_NA_FLAG_OVERRIDE |
-		    (ip6_forwarding ? ND_NA_FLAG_ROUTER : 0), 1, NULL);
+		nd6_na_output(&sc->sc_if, &mcast, in6, flags, 1, NULL);
 	}
 }
 #endif /* INET6 */
Index: netinet/ip_icmp.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_icmp.c,v
diff -u -p -r1.193 ip_icmp.c
--- netinet/ip_icmp.c	7 Jun 2024 18:24:16 -0000	1.193
+++ netinet/ip_icmp.c	17 Jun 2024 11:22:18 -0000
@@ -589,7 +589,7 @@ reflect:
 		struct sockaddr_in ssrc;
 		struct rtentry *newrt = NULL;
 
-		if (icmp_rediraccept == 0 || ip_forwarding == 1)
+		if (icmp_rediraccept == 0 || ip_forwarding != 0)
 			goto freeit;
 		if (code > 3)
 			goto badcode;
Index: netinet6/icmp6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/icmp6.c,v
diff -u -p -r1.252 icmp6.c
--- netinet6/icmp6.c	21 Apr 2024 17:32:10 -0000	1.252
+++ netinet6/icmp6.c	17 Jun 2024 11:22:18 -0000
@@ -1240,8 +1240,8 @@ icmp6_redirect_input(struct mbuf *m, int
 	if (ifp == NULL)
 		return;
 
-	/* XXX if we are router, we don't update route by icmp6 redirect */
-	if (ip6_forwarding)
+	/* if we are router, we don't update route by icmp6 redirect */
+	if (ip6_forwarding != 0)
 		goto freeit;
 	if (!(ifp->if_xflags & IFXF_AUTOCONF6))
 		goto freeit;
@@ -1442,7 +1442,7 @@ icmp6_redirect_output(struct mbuf *m0, s
 	icmp6_errcount(ND_REDIRECT, 0);
 
 	/* if we are not router, we don't send icmp6 redirect */
-	if (!ip6_forwarding)
+	if (ip6_forwarding == 0)
 		goto fail;
 
 	/* sanity check */
Index: netinet6/ip6_forward.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_forward.c,v
diff -u -p -r1.118 ip6_forward.c
--- netinet6/ip6_forward.c	7 Jun 2024 18:24:16 -0000	1.118
+++ netinet6/ip6_forward.c	17 Jun 2024 11:22:18 -0000
@@ -82,7 +82,7 @@
  */
 
 void
-ip6_forward(struct mbuf *m, struct route *ro, int srcrt)
+ip6_forward(struct mbuf *m, struct route *ro, int flags)
 {
 	struct ip6_hdr *ip6 = mtod(m, struct ip6_hdr *);
 	struct route iproute;
@@ -248,8 +248,8 @@ reroute:
 		m_freem(m);
 		goto freecopy;
 	}
-	if (rt->rt_ifidx == m->m_pkthdr.ph_ifidx && !srcrt &&
-	    ip6_sendredirects &&
+	if (rt->rt_ifidx == m->m_pkthdr.ph_ifidx &&
+	    ip6_sendredirects && !ISSET(flags, IPV6_REDIRECT) &&
 	    (rt->rt_flags & (RTF_DYNAMIC|RTF_MODIFIED)) == 0) {
 		if ((ifp->if_flags & IFF_POINTOPOINT) &&
 		    nd6_is_addr_neighbor(&ro->ro_dstsin6, ifp)) {
@@ -305,7 +305,7 @@ reroute:
 	} else if (m->m_pkthdr.pf.flags & PF_TAG_REROUTE) {
 		/* tag as generated to skip over pf_test on rerun */
 		m->m_pkthdr.pf.flags |= PF_TAG_GENERATED;
-		srcrt = 1;
+		SET(flags, IPV6_REDIRECT);
 		if (ro == &iproute)
 			rtfree(ro->ro_rt);
 		ro = NULL;
Index: netinet6/ip6_input.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_input.c,v
diff -u -p -r1.262 ip6_input.c
--- netinet6/ip6_input.c	8 May 2024 13:01:30 -0000	1.262
+++ netinet6/ip6_input.c	17 Jun 2024 11:22:18 -0000
@@ -119,9 +119,9 @@ struct cpumem *ip6counters;
 
 uint8_t ip6_soiikey[IP6_SOIIKEY_LEN];
 
-int ip6_ours(struct mbuf **, int *, int, int);
+int ip6_ours(struct mbuf **, int *, int, int, int);
 int ip6_check_rh0hdr(struct mbuf *, int *);
-int ip6_hbhchcheck(struct mbuf **, int *, int *);
+int ip6_hbhchcheck(struct mbuf **, int *, int *, int);
 int ip6_hopopts_input(struct mbuf **, int *, u_int32_t *, u_int32_t *);
 struct mbuf *ip6_pullexthdr(struct mbuf *, size_t, int);
 int ip6_sysctl_soiikey(void *, size_t *, void *, size_t);
@@ -172,11 +172,11 @@ ip6_init(void)
  * NET_LOCK_SHARED() and the transport layer needing it exclusively.
  */
 int
-ip6_ours(struct mbuf **mp, int *offp, int nxt, int af)
+ip6_ours(struct mbuf **mp, int *offp, int nxt, int af, int flags)
 {
 	/* ip6_hbhchcheck() may be run before, then off and nxt are set */
 	if (*offp == 0) {
-		nxt = ip6_hbhchcheck(mp, offp, NULL);
+		nxt = ip6_hbhchcheck(mp, offp, NULL, flags);
 		if (nxt == IPPROTO_DONE)
 			return IPPROTO_DONE;
 	}
@@ -365,7 +365,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 #if NPF > 0
 	struct in6_addr odst;
 #endif
-	int pfrdr = 0;
+	int flags = 0;
 
 	KASSERT(*offp == 0);
 
@@ -412,9 +412,13 @@ ip6_input_if(struct mbuf **mp, int *offp
 		goto bad;
 
 	ip6 = mtod(m, struct ip6_hdr *);
-	pfrdr = !IN6_ARE_ADDR_EQUAL(&odst, &ip6->ip6_dst);
+	if (!IN6_ARE_ADDR_EQUAL(&odst, &ip6->ip6_dst))
+		SET(flags, IPV6_REDIRECT);
 #endif
 
+	if (ip6_forwarding != 0)
+		SET(flags, IPV6_FORWARDING);
+
 	/*
 	 * Without embedded scope ID we cannot find link-local
 	 * addresses in the routing table.
@@ -445,7 +449,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 
 #if NPF > 0
 	if (pf_ouraddr(m) == 1) {
-		nxt = ip6_ours(mp, offp, nxt, af);
+		nxt = ip6_ours(mp, offp, nxt, af, flags);
 		goto out;
 	}
 #endif
@@ -472,7 +476,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 		if (ip6_mforwarding && ip6_mrouter[ifp->if_rdomain]) {
 			int error;
 
-			nxt = ip6_hbhchcheck(&m, offp, &ours);
+			nxt = ip6_hbhchcheck(&m, offp, &ours, flags);
 			if (nxt == IPPROTO_DONE)
 				goto out;
 
@@ -496,7 +500,8 @@ ip6_input_if(struct mbuf **mp, int *offp
 
 			if (ours) {
 				if (af == AF_UNSPEC)
-					nxt = ip6_ours(mp, offp, nxt, af);
+					nxt = ip6_ours(mp, offp, nxt, af,
+					    flags);
 				goto out;
 			}
 			goto bad;
@@ -508,7 +513,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 				ip6stat_inc(ip6s_cantforward);
 			goto bad;
 		}
-		nxt = ip6_ours(mp, offp, nxt, af);
+		nxt = ip6_ours(mp, offp, nxt, af, flags);
 		goto out;
 	}
 
@@ -526,7 +531,8 @@ ip6_input_if(struct mbuf **mp, int *offp
 	if (rt != NULL && ISSET(rt->rt_flags, RTF_LOCAL)) {
 		struct in6_ifaddr *ia6 = ifatoia6(rt->rt_ifa);
 
-		if (ip6_forwarding == 0 && rt->rt_ifidx != ifp->if_index &&
+		if (!ISSET(flags, IPV6_FORWARDING) &&
+		    rt->rt_ifidx != ifp->if_index &&
 		    !((ifp->if_flags & IFF_LOOPBACK) ||
 		    (ifp->if_type == IFT_ENC) ||
 		    (m->m_pkthdr.pf.flags & PF_TAG_TRANSLATE_LOCALHOST))) {
@@ -567,7 +573,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 
 			goto bad;
 		} else {
-			nxt = ip6_ours(mp, offp, nxt, af);
+			nxt = ip6_ours(mp, offp, nxt, af, flags);
 			goto out;
 		}
 	}
@@ -582,18 +588,18 @@ ip6_input_if(struct mbuf **mp, int *offp
 	 * Now there is no reason to process the packet if it's not our own
 	 * and we're not a router.
 	 */
-	if (!ip6_forwarding) {
+	if (!ISSET(flags, IPV6_FORWARDING)) {
 		ip6stat_inc(ip6s_cantforward);
 		goto bad;
 	}
 
-	nxt = ip6_hbhchcheck(&m, offp, &ours);
+	nxt = ip6_hbhchcheck(&m, offp, &ours, flags);
 	if (nxt == IPPROTO_DONE)
 		goto out;
 
 	if (ours) {
 		if (af == AF_UNSPEC)
-			nxt = ip6_ours(mp, offp, nxt, af);
+			nxt = ip6_ours(mp, offp, nxt, af, flags);
 		goto out;
 	}
 
@@ -613,7 +619,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 	}
 #endif /* IPSEC */
 
-	ip6_forward(m, &ro, pfrdr);
+	ip6_forward(m, &ro, flags);
 	*mp = NULL;
 	rtfree(ro.ro_rt);
 	return IPPROTO_DONE;
@@ -627,7 +633,7 @@ ip6_input_if(struct mbuf **mp, int *offp
 
 /* On error free mbuf and return IPPROTO_DONE. */
 int
-ip6_hbhchcheck(struct mbuf **mp, int *offp, int *oursp)
+ip6_hbhchcheck(struct mbuf **mp, int *offp, int *oursp, int flags)
 {
 	struct ip6_hdr *ip6;
 	u_int32_t plen, rtalert = ~0;
@@ -680,7 +686,8 @@ ip6_hbhchcheck(struct mbuf **mp, int *of
 		 * accept the packet if a router alert option is included
 		 * and we act as an IPv6 router.
 		 */
-		if (rtalert != ~0 && ip6_forwarding && oursp != NULL)
+		if (rtalert != ~0 && ISSET(flags, IPV6_FORWARDING) &&
+		    oursp != NULL)
 			*oursp = 1;
 	} else
 		nxt = ip6->ip6_nxt;
Index: netinet6/ip6_var.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/ip6_var.h,v
diff -u -p -r1.117 ip6_var.h
--- netinet6/ip6_var.h	13 May 2024 01:15:53 -0000	1.117
+++ netinet6/ip6_var.h	17 Jun 2024 11:22:18 -0000
@@ -265,10 +265,11 @@ ip6stat_add(enum ip6stat_counters c, uin
 	counters_add(ip6counters, c, v);
 }
 
-/* flags passed to ip6_output as last parameter */
-#define	IPV6_UNSPECSRC		0x01	/* allow :: as the source address */
-#define	IPV6_FORWARDING		0x02	/* most of IPv6 header exists */
-#define	IPV6_MINMTU		0x04	/* use minimum MTU (IPV6_USE_MIN_MTU) */
+/* flags passed to ip6_output or ip6_forward as last parameter */
+#define IPV6_UNSPECSRC		0x01	/* allow :: as the source address */
+#define IPV6_FORWARDING		0x02	/* most of IPv6 header exists */
+#define IPV6_MINMTU		0x04	/* use minimum MTU (IPV6_USE_MIN_MTU) */
+#define IPV6_REDIRECT		0x08	/* redirected by pf */
 
 extern int ip6_mtudisc_timeout;		/* mtu discovery */
 extern struct rttimer_queue icmp6_mtudisc_timeout_q;
Index: netinet6/nd6.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6.c,v
diff -u -p -r1.280 nd6.c
--- netinet6/nd6.c	13 May 2023 16:27:59 -0000	1.280
+++ netinet6/nd6.c	17 Jun 2024 11:22:18 -0000
@@ -671,7 +671,7 @@ nd6_free(struct rtentry *rt)
 
 	ifp = if_get(rt->rt_ifidx);
 
-	if (!ip6_forwarding) {
+	if (ip6_forwarding == 0) {
 		if (ln->ln_router) {
 			/*
 			 * rt6_flush must be called whether or not the neighbor
Index: netinet6/nd6_nbr.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet6/nd6_nbr.c,v
diff -u -p -r1.151 nd6_nbr.c
--- netinet6/nd6_nbr.c	30 Jul 2023 12:52:03 -0000	1.151
+++ netinet6/nd6_nbr.c	17 Jun 2024 11:22:18 -0000
@@ -108,7 +108,7 @@ nd6_ns_input(struct mbuf *m, int off, in
 	struct ifaddr *ifa = NULL;
 	int lladdrlen = 0;
 	int anycast = 0, proxy = 0, tentative = 0;
-	int router = ip6_forwarding;
+	int i_am_router = (ip6_forwarding != 0);
 	int tlladdr;
 	struct nd_opts ndopts;
 	struct sockaddr_dl *proxydl = NULL;
@@ -244,7 +244,7 @@ nd6_ns_input(struct mbuf *m, int off, in
 			if (ifa) {
 				proxy = 1;
 				proxydl = satosdl(rt->rt_gateway);
-				router = 0;	/* XXX */
+				i_am_router = 0;	/* XXX */
 			}
 		}
 		if (rt)
@@ -317,7 +317,7 @@ nd6_ns_input(struct mbuf *m, int off, in
 		saddr6.s6_addr16[1] = htons(ifp->if_index);
 		nd6_na_output(ifp, &saddr6, &taddr6,
 		    ((anycast || proxy || !tlladdr) ? 0 : ND_NA_FLAG_OVERRIDE) |
-		    (router ? ND_NA_FLAG_ROUTER : 0),
+		    (i_am_router ? ND_NA_FLAG_ROUTER : 0),
 		    tlladdr, sdltosa(proxydl));
 		goto freeit;
 	}
@@ -327,7 +327,7 @@ nd6_ns_input(struct mbuf *m, int off, in
 
 	nd6_na_output(ifp, &saddr6, &taddr6,
 	    ((anycast || proxy || !tlladdr) ? 0 : ND_NA_FLAG_OVERRIDE) |
-	    (router ? ND_NA_FLAG_ROUTER : 0) | ND_NA_FLAG_SOLICITED,
+	    (i_am_router ? ND_NA_FLAG_ROUTER : 0) | ND_NA_FLAG_SOLICITED,
 	    tlladdr, sdltosa(proxydl));
  freeit:
 	m_freem(m);
@@ -559,6 +559,7 @@ nd6_na_input(struct mbuf *m, int off, in
 	int is_override;
 	char *lladdr = NULL;
 	int lladdrlen = 0;
+	int i_am_router = (ip6_forwarding != 0);
 	struct ifaddr *ifa;
 	struct in6_ifaddr *ifa6;
 	struct llinfo_nd6 *ln;
@@ -684,7 +685,7 @@ nd6_na_input(struct mbuf *m, int off, in
 	 * If we are a router, we may create new stale cache entries upon
 	 * receiving Unsolicited Neighbor Advertisements.
 	 */
-	if (rt == NULL && ip6_forwarding == 1) {
+	if (rt == NULL && i_am_router) {
 		rt = nd6_lookup(&taddr6, 1, ifp, ifp->if_rdomain);
 		if (rt == NULL || lladdr == NULL ||
 		    ((sdl = satosdl(rt->rt_gateway)) == NULL))
@@ -837,7 +838,7 @@ nd6_na_input(struct mbuf *m, int off, in
 		}
 
 		if (ln->ln_router && !is_router) {
-			if (!ip6_forwarding) {
+			if (!i_am_router) {
 				/*
 				 * The neighbor may be used
 				 * as a next hop for some destinations