On Mon, Aug 05, 2024 at 07:27:42PM +0200, Alexander Bluhm wrote:
> 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.

it's less pointer chasing, and it is more obvious to the reader what the
effect will be. but yes, i agree that the effect is the same.