Re: [devel-ipsec] [PATCH v2 ipsec-next 2/2] xfrm: fix source address in icmp error generation from IPsec gateway

Antony Antony
2023-11-25T23:15:21Z

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