From: Vitaliy Makkoveev Subject: Re: ip forwarding ipsec read once To: Alexander Bluhm Cc: tech@openbsd.org Date: Tue, 2 Jul 2024 17:50:59 +0300 On Tue, Jul 02, 2024 at 04:42:43PM +0200, Alexander Bluhm wrote: > Anyone? > ok mvs. > I would like to implement the IPsec forwarding also for IPv6. Then > the feature set is equivalent. But first IPv4 should be MP safe. > > bluhm > > On Wed, Jun 26, 2024 at 02:10:17PM +0200, Alexander Bluhm wrote: > > Hi, > > > > Claudio mentioned that there is a MP race between reading ip_forwarding > > in ip_input() and checking ip_forwarding == 2 in ip_output(). > > > > In theory ip_forwarding could be 2 during ip_input() and later 0 > > in ip_output(). Then a packet would be forwarded that was never > > allowed. > > > > Currently exclusive netlock in sysctl(2) prevents all the races. > > > > We could introduce IP_FORWARDING_IPSEC and pass it with the flags > > parameter we have introduced for IP_FORWARDING. > > > > Instead of calling m_tag_find(), traversing the list, and comparing > > with NULL, we could just check PACKET_TAG_IPSEC_IN_DONE bit. Reading > > ipsec_in_use in ip_output() is a performance hack that is not > > necessary. New code only checks tree bits. > > > > ok? > > > > bluhm > > > > Index: net/pf.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/net/pf.c,v > > diff -u -p -r1.1199 pf.c > > --- net/pf.c 21 Jun 2024 12:51:29 -0000 1.1199 > > +++ net/pf.c 25 Jun 2024 10:58:17 -0000 > > @@ -7958,20 +7958,26 @@ done: > > case PF_AFRT: > > if (pf_translate_af(&pd)) { > > action = PF_DROP; > > - break; > > + goto out; > > } > > pd.m->m_pkthdr.pf.flags |= PF_TAG_GENERATED; > > switch (pd.naf) { > > case AF_INET: > > if (pd.dir == PF_IN) { > > - int flags; > > + int flags = IP_REDIRECT; > > > > - if (ip_forwarding == 0) { > > + switch (ip_forwarding) { > > + case 2: > > + SET(flags, IP_FORWARDING_IPSEC); > > + /* FALLTHROUGH */ > > + case 1: > > + SET(flags, IP_FORWARDING); > > + break; > > + default: > > ipstat_inc(ips_cantforward); > > action = PF_DROP; > > - break; > > + goto out; > > } > > - flags = IP_FORWARDING | IP_REDIRECT; > > if (ip_directedbcast) > > SET(flags, IP_ALLOWBROADCAST); > > ip_forward(pd.m, ifp, NULL, flags); > > @@ -7985,7 +7991,7 @@ done: > > if (ip6_forwarding == 0) { > > ip6stat_inc(ip6s_cantforward); > > action = PF_DROP; > > - break; > > + goto out; > > } > > flags = IPV6_FORWARDING | IPV6_REDIRECT; > > ip6_forward(pd.m, NULL, flags); > > @@ -7993,10 +7999,8 @@ done: > > ip6_output(pd.m, NULL, NULL, 0, NULL, NULL); > > break; > > } > > - if (action != PF_DROP) { > > - pd.m = NULL; > > - action = PF_PASS; > > - } > > + pd.m = NULL; > > + action = PF_PASS; > > break; > > #endif /* INET6 */ > > case PF_DROP: > > @@ -8036,6 +8040,7 @@ done: > > st->if_index_out = ifp->if_index; > > } > > > > + out: > > *m0 = pd.m; > > > > pf_state_unref(st); > > Index: netinet/ip_input.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_input.c,v > > diff -u -p -r1.396 ip_input.c > > --- netinet/ip_input.c 24 Jun 2024 12:19:19 -0000 1.396 > > +++ netinet/ip_input.c 25 Jun 2024 11:00:54 -0000 > > @@ -465,8 +465,14 @@ ip_input_if(struct mbuf **mp, int *offp, > > SET(flags, IP_REDIRECT); > > #endif > > > > - if (ip_forwarding != 0) > > + switch (ip_forwarding) { > > + case 2: > > + SET(flags, IP_FORWARDING_IPSEC); > > + /* FALLTHROUGH */ > > + case 1: > > SET(flags, IP_FORWARDING); > > + break; > > + } > > if (ip_directedbcast) > > SET(flags, IP_ALLOWBROADCAST); > > > > @@ -529,7 +535,7 @@ ip_input_if(struct mbuf **mp, int *offp, > > * ip_output().) > > */ > > KERNEL_LOCK(); > > - error = ip_mforward(m, ifp); > > + error = ip_mforward(m, ifp, flags); > > KERNEL_UNLOCK(); > > if (error) { > > ipstat_inc(ips_cantforward); > > Index: netinet/ip_mroute.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_mroute.c,v > > diff -u -p -r1.142 ip_mroute.c > > --- netinet/ip_mroute.c 6 Apr 2024 14:23:27 -0000 1.142 > > +++ netinet/ip_mroute.c 25 Jun 2024 10:58:17 -0000 > > @@ -122,7 +122,7 @@ int get_api_support(struct mbuf *); > > int get_api_config(struct mbuf *); > > int socket_send(struct socket *, struct mbuf *, > > struct sockaddr_in *); > > -int ip_mdq(struct mbuf *, struct ifnet *, struct rtentry *); > > +int ip_mdq(struct mbuf *, struct ifnet *, struct rtentry *, int); > > struct ifnet *if_lookupbyvif(vifi_t, unsigned int); > > struct rtentry *rt_mcast_add(struct ifnet *, struct sockaddr *, > > struct sockaddr *); > > @@ -1080,7 +1080,7 @@ socket_send(struct socket *so, struct mb > > #define TUNNEL_LEN 12 /* # bytes of IP option for tunnel encapsulation */ > > > > int > > -ip_mforward(struct mbuf *m, struct ifnet *ifp) > > +ip_mforward(struct mbuf *m, struct ifnet *ifp, int flags) > > { > > struct ip *ip = mtod(m, struct ip *); > > struct vif *v; > > @@ -1121,7 +1121,7 @@ ip_mforward(struct mbuf *m, struct ifnet > > > > /* Entry exists, so forward if necessary */ > > if (rt != NULL) { > > - return (ip_mdq(m, ifp, rt)); > > + return (ip_mdq(m, ifp, rt, flags)); > > } else { > > /* > > * If we don't have a route for packet's origin, > > @@ -1183,7 +1183,7 @@ ip_mforward(struct mbuf *m, struct ifnet > > * Packet forwarding routine once entry in the cache is made > > */ > > int > > -ip_mdq(struct mbuf *m, struct ifnet *ifp0, struct rtentry *rt) > > +ip_mdq(struct mbuf *m, struct ifnet *ifp0, struct rtentry *rt, int flags) > > { > > struct ip *ip = mtod(m, struct ip *); > > struct mfc *mfc = (struct mfc *)rt->rt_llinfo; > > @@ -1281,7 +1281,7 @@ ip_mdq(struct mbuf *m, struct ifnet *ifp > > imo.imo_ttl = ip->ip_ttl - IPTTLDEC; > > imo.imo_loop = 1; > > > > - ip_output(mc, NULL, NULL, IP_FORWARDING, &imo, NULL, 0); > > + ip_output(mc, NULL, NULL, flags | IP_FORWARDING, &imo, NULL, 0); > > if_put(ifp); > > } while ((rt = rtable_iterate(rt)) != NULL); > > > > Index: netinet/ip_output.c > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v > > diff -u -p -r1.400 ip_output.c > > --- netinet/ip_output.c 7 Jun 2024 18:24:16 -0000 1.400 > > +++ netinet/ip_output.c 25 Jun 2024 10:58:17 -0000 > > @@ -331,7 +331,7 @@ reroute: > > int rv; > > > > KERNEL_LOCK(); > > - rv = ip_mforward(m, ifp); > > + rv = ip_mforward(m, ifp, flags); > > KERNEL_UNLOCK(); > > if (rv != 0) > > goto bad; > > @@ -428,9 +428,8 @@ sendit: > > #endif > > > > #ifdef IPSEC > > - if ((flags & IP_FORWARDING) && ip_forwarding == 2 && > > - (!ipsec_in_use || > > - m_tag_find(m, PACKET_TAG_IPSEC_IN_DONE, NULL) == NULL)) { > > + if (ISSET(flags, IP_FORWARDING) && ISSET(flags, IP_FORWARDING_IPSEC) && > > + !ISSET(m->m_pkthdr.ph_tagsset, PACKET_TAG_IPSEC_IN_DONE)) { > > error = EHOSTUNREACH; > > goto bad; > > } > > Index: netinet/ip_var.h > > =================================================================== > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_var.h,v > > diff -u -p -r1.118 ip_var.h > > --- netinet/ip_var.h 7 Jun 2024 18:24:16 -0000 1.118 > > +++ netinet/ip_var.h 25 Jun 2024 10:58:17 -0000 > > @@ -207,6 +207,7 @@ struct ipoffnxt { > > #define IP_FORWARDING 0x0001 /* most of ip header exists */ > > #define IP_RAWOUTPUT 0x0002 /* raw ip header exists */ > > #define IP_REDIRECT 0x0004 /* redirected by pf or source route */ > > +#define IP_FORWARDING_IPSEC 0x0008 /* only packets processed by IPsec */ > > #define IP_ALLOWBROADCAST SO_BROADCAST /* can send broadcast packets */ > > #define IP_MTUDISC 0x0800 /* pmtu discovery, set DF */ > > > > @@ -246,7 +247,7 @@ int ip_getmoptions(int, struct ip_mopti > > void ip_init(void); > > struct mbuf* > > ip_insertoptions(struct mbuf *, struct mbuf *, int *); > > -int ip_mforward(struct mbuf *, struct ifnet *); > > +int ip_mforward(struct mbuf *, struct ifnet *, int); > > int ip_optcopy(struct ip *, struct ip *); > > int ip_output(struct mbuf *, struct mbuf *, struct route *, int, > > struct ip_moptions *, const struct ipsec_level *, u_int32_t); >