From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: ipv4 icmp_reflect() source address selection optimisation
To: David Gwynne <david@gwynne.id.au>
Cc: tech@openbsd.org
Date: Thu, 19 Jun 2025 19:53:34 +0200

On Sat, Jun 07, 2025 at 01:57:45PM +1000, David Gwynne wrote:
> how about i make this a bit easier for you and split the changes
> in this diff up?

This diff passed all my tests.  OK bluhm@

> this one has icmp_reflect respect the route sourceaddr like
> in_pcbselsrc, but doesn't use the rcvif to pick an address.
> 
> Index: ip_icmp.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> diff -u -p -r1.199 ip_icmp.c
> --- ip_icmp.c	2 Mar 2025 21:28:32 -0000	1.199
> +++ ip_icmp.c	7 Jun 2025 03:51:30 -0000
> @@ -691,7 +691,8 @@ icmp_reflect(struct mbuf *m, struct mbuf
>  	struct ip *ip = mtod(m, struct ip *);
>  	struct mbuf *opts = NULL;
>  	struct sockaddr_in sin;
> -	struct rtentry *rt = NULL;
> +	struct rtentry *rt;
> +	struct in_addr ip_src = { INADDR_ANY };
>  	int optlen = (ip->ip_hl << 2) - sizeof(struct ip);
>  	u_int rtableid;
>  	u_int8_t pfflags;
> @@ -708,10 +709,6 @@ icmp_reflect(struct mbuf *m, struct mbuf
>  		return (ELOOP);
>  	}
>  	rtableid = m->m_pkthdr.ph_rtableid;
> -	pfflags = m->m_pkthdr.pf.flags;
> -	m_resethdr(m);
> -	m->m_pkthdr.ph_rtableid = rtableid;
> -	m->m_pkthdr.pf.flags = pfflags & PF_TAG_GENERATED;
>  
>  	/*
>  	 * If the incoming packet was addressed directly to us,
> @@ -725,19 +722,24 @@ icmp_reflect(struct mbuf *m, struct mbuf
>  		sin.sin_addr = ip->ip_dst;
>  
>  		rt = rtalloc(sintosa(&sin), 0, rtableid);
> -		if (rtisvalid(rt) &&
> -		    ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST))
> -			ia = ifatoia(rt->rt_ifa);
> -	}
> +		if (rtisvalid(rt)) {
> +			if (ISSET(rt->rt_flags, RTF_LOCAL))
> +				ip_src = ip->ip_dst;
> +			else if (ISSET(rt->rt_flags, RTF_BROADCAST)) {
> +				ia = ifatoia(rt->rt_ifa);
> +				ip_src = ia->ia_addr.sin_addr;
> +			}
> +		}
> +		rtfree(rt);
> +	} else
> +		ip_src = ia->ia_addr.sin_addr;
>  
>  	/*
>  	 * The following happens if the packet was not addressed to us.
> -	 * Use the new source address and do a route lookup. If it fails
> -	 * drop the packet as there is no path to the host.
> +	 * If we're directly connected use the closest address, otherwise
> +	 * try to use the sourceaddr from the routing table.
>  	 */
> -	if (ia == NULL) {
> -		rtfree(rt);
> -
> +	if (ip_src.s_addr == INADDR_ANY) {
>  		memset(&sin, 0, sizeof(sin));
>  		sin.sin_len = sizeof(sin);
>  		sin.sin_family = AF_INET;
> @@ -745,21 +747,38 @@ icmp_reflect(struct mbuf *m, struct mbuf
>  
>  		/* keep packet in the original virtual instance */
>  		rt = rtalloc(sintosa(&sin), RT_RESOLVE, rtableid);
> -		if (rt == NULL) {
> -			ipstat_inc(ips_noroute);
> -			m_freem(m);
> -			return (EHOSTUNREACH);
> +		if (rtisvalid(rt) &&
> +		    ISSET(rt->rt_flags, RTF_LLINFO|RTF_HOST)) {
> +			ia = ifatoia(rt->rt_ifa);
> +			ip_src = ia->ia_addr.sin_addr;
> +		} else {
> +			struct sockaddr *sourceaddr;
> +			struct ifaddr *ifa;
> +
> +			sourceaddr = rtable_getsource(rtableid, AF_INET);
> +			if (sourceaddr != NULL) {
> +				ifa = ifa_ifwithaddr(sourceaddr, rtableid);
> +				if (ifa != NULL &&
> +				    ISSET(ifa->ifa_ifp->if_flags, IFF_UP))
> +					ip_src = satosin(sourceaddr)->sin_addr;
> +			}
>  		}
> -
> -		ia = ifatoia(rt->rt_ifa);
> +		rtfree(rt);
>  	}
>  
> +	/*
> +	 * If the above didn't find an ip_src, ip_output() will try
> +	 * and fill it in for us.
> +	 */
> +
> +	pfflags = m->m_pkthdr.pf.flags;
> +
> +	m_resethdr(m);
> +	m->m_pkthdr.ph_rtableid = rtableid;
> +	m->m_pkthdr.pf.flags = pfflags & PF_TAG_GENERATED;
>  	ip->ip_dst = ip->ip_src;
> +	ip->ip_src = ip_src;
>  	ip->ip_ttl = MAXTTL;
> -
> -	/* It is safe to dereference ``ia'' iff ``rt'' is valid. */
> -	ip->ip_src = ia->ia_addr.sin_addr;
> -	rtfree(rt);
>  
>  	if (optlen > 0) {
>  		u_char *cp;