From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: route cache mpath
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Tue, 27 Feb 2024 14:19:29 +0100

On Tue, Feb 27, 2024 at 01:52:51PM +0100, Alexander Bluhm wrote:
> On Mon, Feb 26, 2024 at 11:44:57AM +0100, Alexander Bluhm wrote:
> > Also IP input should pass a struct route to IP forward.  This is
> > the same logic that is done when passing a route from IP forward
> > to IP output.  As a result the numbers of route cache lookups in
> > netstat -s should be correct now.
> > 
> > Finally I removed some inconsistencies between IPv4 and IPv4 and
> > IP forward and IP output.
> 
> > Or should I split the diff in smaller pieces?
> 
> This is the other half of the diff.
> 
> ok?

Some bikesheds inline.
In general the diff should go, OK claudio@

-- 
:wq Claudio
 
> @@ -1482,26 +1480,23 @@ ip_forward(struct mbuf *m, struct ifnet 
>  	if (m->m_flags & (M_BCAST|M_MCAST) || in_canforward(ip->ip_dst) == 0) {
>  		ipstat_inc(ips_cantforward);
>  		m_freem(m);
> -		goto freecopy;
> +		goto done;

Would have been good to do that as a seperate step since it adds a lot of
noise to the diff.

> Index: netinet6/ip6_forward.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/ip6_forward.c,v
> diff -u -p -r1.115 ip6_forward.c
> --- netinet6/ip6_forward.c	22 Feb 2024 14:25:58 -0000	1.115
> +++ netinet6/ip6_forward.c	27 Feb 2024 12:46:55 -0000
> @@ -165,25 +166,22 @@ reroute:
>  	}
>  #endif /* IPSEC */
>  
> -	ro.ro_rt = NULL;
> -	route6_cache(&ro, &ip6->ip6_dst, &ip6->ip6_src,
> +	if (ro == NULL) {
> +		ro = &iproute;
> +		ro->ro_rt = NULL;
> +	}
> +	rt = route6_mpath(ro, &ip6->ip6_dst, &ip6->ip6_src,
>  	    m->m_pkthdr.ph_rtableid);
> -	dst = &ro.ro_dstsa;
> -	if (!rtisvalid(rt)) {
> -		rtfree(rt);
> -		rt = rtalloc_mpath(dst, &ip6->ip6_src.s6_addr32[0],
> -		    m->m_pkthdr.ph_rtableid);
> -		if (rt == NULL) {
> -			ip6stat_inc(ip6s_noroute);
> -			if (mcopy) {
> -				icmp6_error(mcopy, ICMP6_DST_UNREACH,
> -					    ICMP6_DST_UNREACH_NOROUTE, 0);
> -			}
> -			m_freem(m);
> -			goto out;
> +	if (rt == NULL) {
> +		ip6stat_inc(ip6s_noroute);
> +		if (mcopy) {

I would prefer to use if (mcopy != NULL) here. See more below.

> +			icmp6_error(mcopy, ICMP6_DST_UNREACH,
> +				    ICMP6_DST_UNREACH_NOROUTE, 0);
>  		}
> +		m_freem(m);
> +		goto done;
>  	}
> -	ro.ro_rt = rt;
> +	dst = &ro->ro_dstsa;
>  
>  	/*
>  	 * Scope check: if a packet can't be delivered to its destination

> @@ -324,21 +321,21 @@ reroute:
>  	if (error || m == NULL)
>  		goto senderr;
>  
> -	if (mcopy != NULL)
> +	if (mcopy)

Same here. I prefer if (mcopy != NULL) over if (mcopy) for pointers.
Especially since a few lines below we have: if (mcopy == NULL)

>  		icmp6_error(mcopy, ICMP6_PACKET_TOO_BIG, 0, ifp->if_mtu);
>  	m_freem(m);
> -	goto out;
> +	goto done;
>  
>  senderr:
>  	if (mcopy == NULL)
> -		goto out;
> +		goto done;
>  
>  	switch (error) {
>  	case 0:
>  		if (type == ND_REDIRECT) {
>  			icmp6_redirect_output(mcopy, rt);
>  			ip6stat_inc(ip6s_redirectsent);
> -			goto out;
> +			goto done;
>  		}
>  		goto freecopy;
>