On Wed, Aug 21, 2024 at 03:40:23PM +1000, David Gwynne wrote:
> On Fri, Aug 09, 2024 at 11:47:14AM +0200, Claudio Jeker wrote:
> > On Fri, Aug 09, 2024 at 11:15:34AM +0200, Alexander Bluhm wrote:
> > > On Fri, Aug 09, 2024 at 10:40:11AM +1000, David Gwynne wrote:
> > > > tl;dr: i believe this change would let us simplify pf_route (the
> > > > loopback ip handling specifically), so it's worth it.
> > > 
> > > In contrast I have worked on all the corner cases in 15 years to
> > > make it work.  Maybe not the best solution, but it works.  I fear
> > > that at our customers somethig will break if we change behavior
> > > just because we think it looks better.
> 
> all of them? that's a bold claim.
> 
> add this config the PF host in your test setup and then run the regress:
> 
> # cat /etc/hostname.vport0
> inet 169.254.0.1 255.255.255.0
> up
> # cat /etc/hostname.pfsync0
> syncif vport0
> maxupd 128
> defer
> up
> 
> 
> im hitting edge cases with pf_route and pfsync, that's why im working
> on this. this bit is a chunk out of a much larger set of changes to fix
> it, which ive included below. i'm starting to worry that i'll have
> to carry it as part of my menagerie of local diffs on the firewalls cos
> i'm the only person in the world doing multipath and pfsync?
> 
> my full diff for the pf_route and pfsync fixes is below. i'm not hugely
> happy with chewing up more mbuf tags ids, but we haven't come up with a
> less worse idea yet.
> 
> > This is not only about looks. ICMP source selection is a massive pain
> > point on DFZ routers and we need to fix this. Reducing the amount of
> > corners to cover would be very benefitial.
> > 
> > > In this particular case I think using the first addreess as source
> > > address is wrong.  IPv4 source address selection should be done
> > > with a route lookup.  Basically what in_pcbselsrc() does.  And using
> > > the route interface address seems reasonable to me.
> 
> for the large part my last diff does follow what in_pcbselsrc does
> more closely than the current code. it respects the "route sourceaddr"
> config while the current code doesnt.
> 
> however, the icmp reflect situation is a little different to what
> in_pcbselsrc usually handles though. in_pcbselsrc is used to pick
> an address for a locally terminated connection, while icmp_reflect
> can be used to reply to a packet going through the box. let's not
> pretend these are completely identical situations.

Yes. This is also the big issue of icmp reflect since it needs to select a
source IP for a situation where the system can not decide which IP is
better. An Ip from the received interface, that of the route the
destination points at, or an IP of the interface the ICMP will actually
go out with.

We have 3 options and all 3 have issues. It is a game we can not win in
code. With respecting the "route sourceaddr" we have at least a way to
decide this via configuration.
 
> > To be honest I think it should not matter which IP is selected in the IPv4
> > case. All adresses on the interface can be used to send out an ICMP error.
> > Now I do agree that using the same logic as in in_pcbselsrc() would benefit
> > consitancy.
> 
> i mostly agree, except selecting 127.0.0.1 to reply to a packet that
> arrived off the wire seems bloody minded when there's (what i consider)
> reasonable ways to avoid it.

Very much agree with that. I was mostly thinking of selecting an IP of an
external interface (like the one the packet was received on).

-- 
:wq Claudio