On Mon, Aug 05, 2024 at 10:39:24AM +0200, Alexandr Nedvedicky wrote:
> Hello,
> 
> On Mon, Jul 29, 2024 at 09:21:25AM +1000, David Gwynne wrote:
> </snip>
> > 
> > after talking to claudio, i think the actual source address selection in
> > icmp_reflect can be improved.
> > 
> > this diff still delegates setting the source address based on a
> > destination route lookup to ip_output(), but it adds some extra attempts
> > in icmp_reflect() itself to pick a good source address.
> > 
> > if original packet is for a local address on the host, then we use the
> > local address as the source for the icmp reflected packet, as we do now.
> > 
> > however, if we're routing the packet, the way we pick a source address
> > now has some extra steps. firstly, we try and use a "route sourceaddr"
> > as we do for socket addresses. this means if we have a link local or
> > host route for the icmp packet destination we prefer to use that, and
> > then fall through to using the address configured with "route
> > sourceaddr" next. if that doesn't exist then we try and use an ip
> > address from the interface the icmp packet was received on. if all that
> > fails, we let ip_output set it.
> > 
> > why do we care about this when the status quo has been fine for so long
> > you ask? 
> > 
> > our stack is a lot more complicated now, and things like pf and ipsec
> > mean that it is possible to reroute packets that would otherwise be
> > dropped. the pf_forward regress test demonstrates some of them, but the
> > most interesting one is when we generate an icmp reply that's supposed
> > to head toward a blackholed prefix, but pf reply-to tries to fix it up.
> > blackhole routes are only really supported on loopback interfaces, which
> > means pretty much every icmp reflected packet ends up using the local ip
> > on lo0. without pf, these replies would be dropped, but with reply-to we
> > suddnely want to send an icmp packet with the local ip from lo0, which
> > is 127.0.0.1, which also should never appear on the wire.
> > 
> > these changes mean we try a lot harder to use a usable ip as the source
> > address on icmp reflected packets, and will hopefully let us clean up a
> > fixup in pf_route for handling packets that were generated with loopback
> > ips on them.
> > 
> 
>     it all make sense. It's definitely an improvement to what we have.
>     I have one comment/question to your diff. see in-line.
> 
> > 
> > Index: ip_icmp.c
> > ===================================================================
> > RCS file: /cvs/src/sys/netinet/ip_icmp.c,v
> > diff -u -p -r1.196 ip_icmp.c
> > --- ip_icmp.c	14 Jul 2024 18:53:39 -0000	1.196
> > +++ ip_icmp.c	28 Jul 2024 22:59:21 -0000
> > @@ -684,7 +684,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;
> > @@ -701,10 +702,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,
> > @@ -718,19 +715,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;
> >  
>     I would expect to find ip_src address in ->ia_list. Are we sure
>     ia_addr holds IP address attached to interface?
>     From seeing the places where we call icmp_reflect() this else
>     branch seems to handle some MPLS case. So there is a chance
>     you code is still right and I miss something here.

I there a funtional difference in this chunk?  When a route lookup
to ip->ip_dst returns a RTF_LOCAL route, then
ifatoia(rt->rt_ifa)->ia_addr.sin_addr it must be the same as
ip->ip_dst.

bluhm