| Message ID | 300c36a0644b63228cee8d0a74be0e1e81d0fe98.1698394516.git.antony.antony@secunet.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | None | expand |
Antony Antony via Devel <devel@linux-ipsec.org> wrote: > When enabling support for xfrm lookup using reverse ICMP payload, > We have identified an issue where the source address of the IPv4 e.g > "Destination Host Unreachable" message is incorrect. The IPv6 appear > to do the right thing. One thing that operators of routers with a multitude of interfaces want to do is send all ICMP messages from a specific IP address. Often the public address, that has the sane reverse DNS name. AFAIK, this is not an option on Linux, but Cisco/Juniper/etc. devices usually can do this. I can't recall how today. (I was actually looking that up this week) This can conflict however, with the need to get the result back into the tunnel. I don't have a good answer, except that we probably need a fair bit of flexibility, with some good automatically discovered defaults.
On Fri, Oct 27, 2023 at 09:30:07AM -0400, Michael Richardson via Devel wrote: > > Antony Antony via Devel <devel@linux-ipsec.org> wrote: > > When enabling support for xfrm lookup using reverse ICMP payload, > > We have identified an issue where the source address of the IPv4 e.g > > "Destination Host Unreachable" message is incorrect. The IPv6 appear > > to do the right thing. > > One thing that operators of routers with a multitude of interfaces want to do > is send all ICMP messages from a specific IP address. Often the public > address, that has the sane reverse DNS name. While it makes sense for routers with multiple interfaces, receiving ICMP errors from private addresses can be confusing. However, wouldn't this also make it more challenging to adhere to BCP 32 and BCP 38? Routing with multiple interfaces is tricky on Linux, especially when it comes to compliance with these BCPs. > AFAIK, this is not an option on Linux, but Cisco/Juniper/etc. devices usually > can do this. I can't recall how today. (I was actually looking that up this week) I wonder if a netfilter rule would be a solution for you, something like: "ip protocol icmp type <error codes> snat to x.x.x.x" I would love see a simple option instead of a SNAT hack. May be a routing rule that will choose sourse address for ICMP error code. > This can conflict however, with the need to get the result back into the > tunnel. I don't have a good answer, except that we probably need a fair bit Forwarding that ICMP packet, which is not covered by your forward IPsec policy, would be fixed with the second patch in this series. In that case lookup would using the orginal packet as describe in RFC 4301, Section 6. -antony
Antony Antony <antony@phenome.org> wrote: > On Fri, Oct 27, 2023 at 09:30:07AM -0400, Michael Richardson via Devel > wrote: >> >> Antony Antony via Devel <devel@linux-ipsec.org> wrote: > When enabling >> support for xfrm lookup using reverse ICMP payload, > We have >> identified an issue where the source address of the IPv4 e.g > >> "Destination Host Unreachable" message is incorrect. The IPv6 appear > >> to do the right thing. >> >> One thing that operators of routers with a multitude of interfaces >> want to do is send all ICMP messages from a specific IP address. >> Often the public address, that has the sane reverse DNS name. > While it makes sense for routers with multiple interfaces, receiving > ICMP errors from private addresses can be confusing. However, wouldn't > this also make it more challenging to adhere to BCP 32 and BCP 38? > Routing with multiple interfaces is tricky on Linux, especially when it > comes to compliance with these BCPs. Yes, that's why sending from a public, topically significant source address is really important. Yet, many links are numbered in 1918 because.. > I wonder if a netfilter rule would be a solution for you, something > like: > I would love see a simple option instead of a SNAT hack. May be a > routing rule that will choose sourse address for ICMP error code. yeah, I really don't want to do SNAT stuff. I'd like to have a flag on each interface that says to use the "global" ICMP source or use the heuristic we have now. And then we need a way to set that source address. Most routing platforms put a /32 address (and /128) on lo (or a dummy) as the device's reachable address, and then spread that through OSPF.
On Fri, Oct 27, 2023 at 10:16:52AM +0200, Antony Antony wrote: > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index e63a3bf99617..bec234637122 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -555,7 +555,6 @@ static struct rtable *icmp_route_lookup(struct net *net, > XFRM_LOOKUP_ICMP); > if (!IS_ERR(rt2)) { > dst_release(&rt->dst); > - memcpy(fl4, &fl4_dec, sizeof(*fl4)); This is not really IPsec code. The change needs either an Ack of one of the netdev Maintainers, or it has to go through the nedev tree. Also, please consider this as a fix.
On Fri, Nov 17, 2023 at 10:21:37AM +0100, Steffen Klassert via Devel wrote: > On Fri, Oct 27, 2023 at 10:16:52AM +0200, Antony Antony wrote: > > > > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > > index e63a3bf99617..bec234637122 100644 > > --- a/net/ipv4/icmp.c > > +++ b/net/ipv4/icmp.c > > @@ -555,7 +555,6 @@ static struct rtable *icmp_route_lookup(struct net *net, > > XFRM_LOOKUP_ICMP); > > if (!IS_ERR(rt2)) { > > dst_release(&rt->dst); > > - memcpy(fl4, &fl4_dec, sizeof(*fl4)); > > This is not really IPsec code. The change needs either an > Ack of one of the netdev Maintainers, or it has to go I understand your concern. I chose to submit the change to ipsec-next as it is directly related to the outcome of a successful xfrm_lookup(). > through the nedev tree. Also, please consider this as > a fix. It is a fix:) I considered including a 'Fixes:' tag initially but ultimately decided against it. My hesitation stemmed from the concern that if this fix were backported, it could inadvertently trigger regressions in someone’s test suite. This might lead to requests for a revert through the ipsec tree, which I am keen to avoid. However, I do concur that this submission qualifies as a fix. Is there a way to include the 'Fixes:' tag while also advising against backporting it to reduce the risk of potential regressions? I will add the 'Fixes:' tag to the new version. When it comes to backport I will recomend not to backport this fix. Please keep an eye out for those messages. This could get backported to all curently maintained releases! The key reason for pairing this update with my other patch ("xfrm: introduce forwarding of ICMP Error messages") is to proactively address any potential claims of a regression. Without this new patch, it's conceivable that the changes could be misinterpreted as causing a regression, especially considering that the commit this patch addresses is 12 years old! By submitting them together, it should help clarify that these changes are, in fact, rectifying long-standing issues rather than introducing new ones. I believe applying two patches together will provide a clearer context for both the changes and help streamline their acceptance and integration. thanks, -antony
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index e63a3bf99617..bec234637122 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -555,7 +555,6 @@ static struct rtable *icmp_route_lookup(struct net *net, XFRM_LOOKUP_ICMP); if (!IS_ERR(rt2)) { dst_release(&rt->dst); - memcpy(fl4, &fl4_dec, sizeof(*fl4)); rt = rt2; } else if (PTR_ERR(rt2) == -EPERM) { if (rt)