Re: [PATCH v6 ipsec-next] xfrm: introduce forwarding of ICMP Error messages

Steffen Klassert
2024-01-26T09:11:13Z

On Fri, Jan 19, 2024 at 12:27:42PM +0100, Antony Antony wrote:
> This commit aligns with RFC 4301, Section 6, and addresses the
> requirement to forward unauthenticated ICMP error messages that do not
> match any xfrm policies. It utilizes the ICMP payload as an skb and
> performs a reverse lookup. If a policy match is found, forward
> the packet.
>
> The ICMP payload typically contains a partial IP packet that is likely
> responsible for the error message.
>
> The following error types will be forwarded:
> - IPv4 ICMP error types: ICMP_DEST_UNREACH & ICMP_TIME_EXCEEDED
> - IPv6 ICMPv6 error types: ICMPV6_DEST_UNREACH, ICMPV6_PKT_TOOBIG,
> ICMPV6_TIME_EXCEED
>
> To implement this feature, a reverse lookup has been added to the xfrm
> forward path, making use of the ICMP payload as the skb.
>
> To enable this functionality from user space, the XFRM_POLICY_ICMP flag
> should be added to the outgoing and forward policies, and the
> XFRM_STATE_ICMP flag should be set on incoming states.
>
> e.g.
> ip xfrm policy add flag icmp tmpl
>
> ip xfrm policy
> src 192.0.2.0/24 dst 192.0.1.0/25
> dir out priority 2084302 ptype main flag icmp
>
> ip xfrm state add ...flag icmp
>
> ip xfrm state
> root@west:~#ip x s
> src 192.1.2.23 dst 192.1.2.45
> proto esp spi 0xa7b76872 reqid 16389 mode tunnel
> replay-window 32 flag icmp af-unspec
>
> Changes since v5:
> - fix return values bool->int, feedback from Steffen
>
> Changes since v4:
> - split the series to only ICMP erorr forwarding
>
> Changes since v3: no code chage
> - add missing white spaces detected by checkpatch.pl
>
> Changes since v2: reviewed by Steffen Klassert
> - user consume_skb instead of kfree_skb for the inner skb
> - fixed newskb leaks in error paths
> - free the newskb once inner flow is decoded with change due to
> commit 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector")
> - if xfrm_decode_session_reverse() on inner payload fails ignore.
> do not increment error counter
>
> Changes since v1:
> - Move IPv6 variable declaration inside IS_ENABLED(CONFIG_IPV6)
>
> Changes since RFC:
> - Fix calculation of ICMPv6 header length
>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>

Patch applied, thanks a lot Antony!