netinet: handle blackhole routes

[kp]

Summary

If during ip_output() we find a blackhole (or reject) route we should stop
processing and count this in the 'cantforward' counter, just like we already do
for IPv6.
Blackhole routes are set to use the loopback interface, so we don't actually
incorrectly forward traffic, but we do fail to count it as unroutable.

Test this, both for IPv4 and IPv6.

Sponsored by: Rubicon Communications, LLC ("Netgate")

[melifaro]

sys/netinet/ip_output.c

525

I'm not sure if we should return ENETUNREACH (that's likely be returned to the user) for the blackhole routes.
Also I'm trying to find the code you refer to in IPv6 and I can't seem to find any place in ip6_output() that increments ip6s_cantforward counter.

[zlei]

And, with this change, it seems we no longer require nexthop to be loopback interface (V_lo) for blackhole / rejected routes.

sys/netinet/ip_output.c

525

For NHF_REJECT I think it is OK to have EHOSTUNREACH or ENETUNREACH since this is explicit by admin. Well I'd prefer the former ( EHOSTUNREACH ) for unicast destination. For a multicast / broadcast destination I think ENETUNREACH is proper.

For NHF_BLACKHOLE, the system should behave as if the packet was transmitted / forwarded, so should return 0 IIUC.

[kp]

In D47529#1084216, @zlei wrote:

And, with this change, it seems we no longer require nexthop to be loopback interface (V_lo) for blackhole / rejected routes.

Indeed, although the new netlink code does just automatically set the nexthop to loopback for blackhole routes (see https://cgit.freebsd.org/src/tree/sys/net/route/nhop_ctl.c#n830) . I believe that's why blackhole routes actually worked, absent an explicit check.
My main motivation here is to get the counter, but we should be explicit about not routing such packets rather than relying on some other code setting things up so that we do the right thing here.

sys/netinet/ip_output.c

525

This is the IPv6 equivalent: https://cgit.freebsd.org/src/tree/sys/netinet6/ip6_forward.c#n197

It's in ip_output() here, because ip_forward() doesn't do a routing lookup, but calls into ip_forward(). ip6_forward() does do its own work.

I have no particularly strong feelings on what error code we return in this case, although I think I would prefer not making a distinction between NHF_REJECT and NHF_BLACKHOLE, if only to keep the code simpler (also, IPv6 doesn't).

[melifaro]

sys/netinet/ip_output.c

525

I'm afraid I don't understand :-(

ip_output() main purpose is to output local packets, not forward.
The default forwarding path, fast forwarding, does not touch ip_output() at all. It's only ipsec and multicast that go through "slow" forwarding process and end up in ip_output().

I don't disagree with proper accounting for errors, but let's actually account *only* for the packets that are being forwarded.

[kp]

sys/netinet/ip_output.c

525

Hmm, now I'm confused too. I missed the ip_fastfwd() call, where it looks like we ought to increment ips_cantforward (via ip_findroute()), but the fwd_ip_blackhole test failed without this patch, and succeeds with. Presumably that means we don't take ip_fastfwd(), but I don't immediately see why.

I'm also tempted to make this code use ip_findroute(), because that handles the counter bits for ip_fastfwd(), but that would still fail to make the output/forward distinction. I don't really want to add even more flags to make ip_findroute() cope with that.

[kp]

Update the counter in ip_forward(), so we only set 'cantroute' in the forwarding case.
Extend the tests to test both slow and fast path forwarding.

[melifaro]

Generally LGTM, with the nit around NHF_BROADCAST

sys/netinet/ip_input.c

945 ↗

(On Diff #146522)

Q - why NHF_BROADCAST?
Also - both cases seem to have pretty much the same action except sending icmp_error() - maybe it's worth handling both in a single condition?

[kp]

sys/netinet/ip_input.c

945 ↗

(On Diff #146522)

This mirrors the check done in the fast forward case: https://cgit.freebsd.org/src/tree/sys/netinet/ip_fastfwd.c#n207

And given that the commonality between REJECT and BLACKHOLE|BROADCAST is two lines (IPSTAT_INC(ips_cantforward) and NH_FREE()) I'm not sure that's worth making things more complicated. (Much like we do in the fastforward case)

We'd end up with this:

if (ro.ro_nh->nh_flags & (NHF_BLACKHOLE | NHF_BROADCAST
    | NHF_REJECT)) {
   	IPSTAT_INC(ips_cantforward);
	NH_FREE(ro.ro_nh);
   	if (ro.ro_nh->nh_flags & NHF_REJECT) {
   	    	 icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_HOST, 0, 0);
   	} else {
   	    	m_freem(m);
   	}
   	return;
}

(i.e. more complex, and we're now repeating the NHF_REJECT check)