From: Denis Fondras <denis@openbsd.org>
Subject: Re: route reject/blackhole semantics
To: David Gwynne <david@gwynne.id.au>
Cc: tech@openbsd.org
Date: Mon, 29 Jul 2024 12:00:49 +0200

Le Mon, Jul 29, 2024 at 09:59:22AM +1000, David Gwynne a écrit :
> every few years i am surprised to learn that the RTF_REJECT and
> RTF_BLACKHOLE flags on routes are only really checked when going
> "out" on loopback interfaces. my expectation is that these route
> checks happen when the route lookup happens. it's especially
> surprising that routes with reject/blackhole flags can make it into
> pf.
> 
> this diff moves the reject/blackhole route checks into ip_output before
> pf_test is called.
> 
> i am not alone in my confusion. claudio@ seems keen for this change, but
> also because he thinks it can help simplify handling of blackhole/reject
> routing in things like bgpd, but mostly that it is confusing.
> 
> because the stack checks those flags, lo(4) output doesnt have to
> anymore. however, if you do want to "null route" a prefix so it usually
> wont end up on the wire, but give pf a chance to fiddle with it, you can
> still add a route via lo(4) for it. which leads to a bug fix.
> 
> you can add routes like this (even without the reject/blackhole diffs):
> 
> # route add 100.64.0.0/24 127.0.0.1
> 
> if you then ping 100.64.0.1, you'll end up producing a traffic loop
> inside the kernel. each ping packet will loop through lo0 until the ttl
> expires. i think lo ouput should check that the address it's delivering
> to has RTF_LOCAL set, which is what the if_loop.c chunk of the diff
> does.
> 
> thoughts? tests? ok?
> 

Looks OK denis@

> Index: net/if_loop.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_loop.c,v
> diff -u -p -r1.98 if_loop.c
> --- net/if_loop.c	29 Dec 2023 11:43:04 -0000	1.98
> +++ net/if_loop.c	28 Jul 2024 11:58:10 -0000
> @@ -261,10 +261,10 @@ looutput(struct ifnet *ifp, struct mbuf 
>  	if ((m->m_flags & M_PKTHDR) == 0)
>  		panic("%s: no header mbuf", __func__);
>  
> -	if (rt && rt->rt_flags & (RTF_REJECT|RTF_BLACKHOLE)) {
> +	if (rt != NULL && !ISSET(rt->rt_flags, RTF_LOCAL)) {
>  		m_freem(m);
> -		return (rt->rt_flags & RTF_BLACKHOLE ? 0 :
> -			rt->rt_flags & RTF_HOST ? EHOSTUNREACH : ENETUNREACH);
> +		return (ISSET(rt->rt_flags, RTF_HOST) ?
> +		    EHOSTUNREACH : ENETUNREACH);
>  	}
>  
>  	/*
> Index: netinet/ip_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> diff -u -p -r1.401 ip_output.c
> --- netinet/ip_output.c	2 Jul 2024 18:33:47 -0000	1.401
> +++ netinet/ip_output.c	28 Jul 2024 11:58:10 -0000
> @@ -398,6 +398,20 @@ sendit:
>  	}
>  #endif /* IPSEC */
>  
> +	if (ro && ro->ro_rt) {
> +		struct rtentry *rt = ro->ro_rt;
> +
> +		if (ISSET(rt->rt_flags, RTF_REJECT)) {
> +			error = ISSET(rt->rt_flags, RTF_HOST) ?
> +			    EHOSTUNREACH : ENETUNREACH;
> +			goto bad;
> +		}
> +		if (ISSET(rt->rt_flags, RTF_BLACKHOLE)) {
> +			error = 0;
> +			goto bad;
> +		}
> +	}
> +
>  	/*
>  	 * Packet filter
>  	 */
> Index: netinet6/ip6_output.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_output.c,v
> diff -u -p -r1.292 ip6_output.c
> --- netinet6/ip6_output.c	4 Jul 2024 12:50:08 -0000	1.292
> +++ netinet6/ip6_output.c	28 Jul 2024 11:58:10 -0000
> @@ -483,9 +483,20 @@ reroute:
>  		route6_cache(ro, &ip6->ip6_dst, NULL, m->m_pkthdr.ph_rtableid);
>  	}
>  
> -	if (rt && (rt->rt_flags & RTF_GATEWAY) &&
> -	    !IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst))
> -		dst = satosin6(rt->rt_gateway);
> +	if (rt != NULL) {
> +		if (ISSET(rt->rt_flags, RTF_REJECT)) {
> +			error = ISSET(rt->rt_flags, RTF_HOST) ?
> +			    EHOSTUNREACH : ENETUNREACH;
> +			goto bad;
> +		}
> +		if (ISSET(rt->rt_flags, RTF_BLACKHOLE)) {
> +			error = 0;
> +			goto bad;
> +		}
> +		if (ISSET(rt->rt_flags, RTF_GATEWAY) &&
> +		    !IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst))
> +			dst = satosin6(rt->rt_gateway);
> +	}
>  
>  	if (!IN6_IS_ADDR_MULTICAST(&ip6->ip6_dst)) {
>  		/* Unicast */
>