From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: ip forwarding ipsec read once
To: tech@openbsd.org
Date: Tue, 2 Jul 2024 16:42:43 +0200

Anyone?

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