Date: Thu, 10 Nov 2022 14:21:29 +0000 From: Glen Barber <gjb@freebsd.org> To: rscheff@freebsd.org Cc: re@freebsd.org, Michael Tuexen <tuexen@freebsd.org>, freebsd-net@freebsd.org Subject: Re: ip_fw_nat stealing bits Message-ID: <20221110142129.GV76435@FreeBSD.org> In-Reply-To: <bce6167d-13d3-6812-ba5a-edad869df5dd@gmx.at> References: <bce6167d-13d3-6812-ba5a-edad869df5dd@gmx.at>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On Thu, Nov 10, 2022 at 12:51:48PM +0100, Scheffenegger, Richard wrote:
> As mentioned previously, the nat module of ip_fw steals one bit in the
> TCP header, which used to be unallocated, for internal purposes.
>
> That prevents the use of TCP AccECN across fbsd operated NAT devices.
>
> This simple fix shifts the used bit (unnamed previously and therefore
> missed for a long time) from TH_AE (0x100 of the TCP header flags) to
> TH_RES1 (0x800). Because of endianess, and how the TCP header flags are
> split into th_flags and th_x2, some bitshifting is required.
>
> The patch was run earlier this week at the IETF115 L4S Interop during
> the Hackathon, and reviewed/tested by tuexen@FreeBSD.org (present at the
> time, and finding this issue earlier during Interop testing).
>
> The risk involved here is also small, as the previously used bottommost
> bit ("1") is only shifted left to the topmost bit, and checked properly.
>
> As 12.4 may be around for a long time, getting this patch picked up will
> allow the more widespread use of L4S and AccECN faster in the coming years.
>
Approved.
Glen
> From 5009a530f6964c6a1193f9d97873cea7f96cf217 Mon Sep 17 00:00:00 2001
> From: Richard Scheffenegger <rscheff@FreeBSD.org>
> Date: Wed, 9 Nov 2022 10:54:34 +0100
> Subject: [PATCH] ipfw: Have NAT steal the TH_RES1 bit, instead of the TH_AE
> bit
>
> The NAT module use of the tcphdr.th_x2 field now collides with the
> use of this TCP header flag as AccECN (AE) bit. Use the topmost
> bit instead to allow negotiation of AccECN across a NAT device.
>
> Event: IETF 115 Hackathon
> Reviewed By: #transport, tuexen
> MFC after: 3 days
> Approved by: re (gjb, early-MFC)
> Sponsored by: NetApp, Inc.
> Differential Revision: https://reviews.freebsd.org/D37300
>
> (cherry picked from commit 0b00b801493aa1d4996b0891ea58fbef343f85df)
> (cherry picked from commit 9839a5ad3a683c3841ec00c9e1a4d551dcf9c1de)
> ---
> sys/netinet/libalias/alias_ftp.c | 2 +-
> sys/netinet/libalias/alias_irc.c | 2 +-
> sys/netinet/libalias/alias_proxy.c | 2 +-
> sys/netinet/libalias/alias_skinny.c | 6 +++---
> sys/netinet/libalias/alias_smedia.c | 4 ++--
> sys/netinet/tcp.h | 3 +++
> sys/netpfil/ipfw/ip_fw_nat.c | 4 ++--
> 7 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/sys/netinet/libalias/alias_ftp.c b/sys/netinet/libalias/alias_ftp.c
> index 962194ec0a68..b2fcfbf2396b 100644
> --- a/sys/netinet/libalias/alias_ftp.c
> +++ b/sys/netinet/libalias/alias_ftp.c
> @@ -754,7 +754,7 @@ NewFtpMessage(struct libalias *la, struct ip *pip,
> /* Compute TCP checksum for revised packet */
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> diff --git a/sys/netinet/libalias/alias_irc.c b/sys/netinet/libalias/alias_irc.c
> index 32e831742048..524b70b0632c 100644
> --- a/sys/netinet/libalias/alias_irc.c
> +++ b/sys/netinet/libalias/alias_irc.c
> @@ -458,7 +458,7 @@ AliasHandleIrcOut(struct libalias *la,
> /* Compute TCP checksum for revised packet */
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> diff --git a/sys/netinet/libalias/alias_proxy.c b/sys/netinet/libalias/alias_proxy.c
> index 9b75b22a74b3..7efab1fdc8db 100644
> --- a/sys/netinet/libalias/alias_proxy.c
> +++ b/sys/netinet/libalias/alias_proxy.c
> @@ -368,7 +368,7 @@ ProxyEncodeTcpStream(struct alias_link *lnk,
>
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> diff --git a/sys/netinet/libalias/alias_skinny.c b/sys/netinet/libalias/alias_skinny.c
> index 31b33696fc20..2c664c2c58d9 100644
> --- a/sys/netinet/libalias/alias_skinny.c
> +++ b/sys/netinet/libalias/alias_skinny.c
> @@ -216,7 +216,7 @@ alias_skinny_reg_msg(struct RegisterMessage *reg_msg, struct ip *pip,
>
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> @@ -259,7 +259,7 @@ alias_skinny_port_msg(struct IpPortMessage *port_msg, struct ip *pip,
>
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> @@ -291,7 +291,7 @@ alias_skinny_opnrcvch_ack(struct libalias *la, struct OpenReceiveChannelAck *opn
>
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> diff --git a/sys/netinet/libalias/alias_smedia.c b/sys/netinet/libalias/alias_smedia.c
> index 9b5a9d673ecf..c09c8e0c6d77 100644
> --- a/sys/netinet/libalias/alias_smedia.c
> +++ b/sys/netinet/libalias/alias_smedia.c
> @@ -404,7 +404,7 @@ alias_rtsp_out(struct libalias *la, struct ip *pip,
>
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> @@ -451,7 +451,7 @@ alias_pna_out(struct libalias *la, struct ip *pip,
> /* Compute TCP checksum for revised packet */
> tc->th_sum = 0;
> #ifdef _KERNEL
> - tc->th_x2 = 1;
> + tc->th_x2 = (TH_RES1 >> 8);
> #else
> tc->th_sum = TcpChecksum(pip);
> #endif
> diff --git a/sys/netinet/tcp.h b/sys/netinet/tcp.h
> index 21922eb4df2e..beb6ece82f35 100644
> --- a/sys/netinet/tcp.h
> +++ b/sys/netinet/tcp.h
> @@ -72,6 +72,9 @@ struct tcphdr {
> #define TH_ECE 0x40
> #define TH_CWR 0x80
> #define TH_AE 0x100 /* maps into th_x2 */
> +#define TH_RES3 0x200
> +#define TH_RES2 0x400
> +#define TH_RES1 0x800
> #define TH_FLAGS (TH_FIN|TH_SYN|TH_RST|TH_PUSH|TH_ACK|TH_URG|TH_ECE|TH_CWR)
> #define PRINT_TH_FLAGS "\20\1FIN\2SYN\3RST\4PUSH\5ACK\6URG\7ECE\10CWR\11AE"
>
> diff --git a/sys/netpfil/ipfw/ip_fw_nat.c b/sys/netpfil/ipfw/ip_fw_nat.c
> index 9e15e9addbe5..b75210246a00 100644
> --- a/sys/netpfil/ipfw/ip_fw_nat.c
> +++ b/sys/netpfil/ipfw/ip_fw_nat.c
> @@ -416,7 +416,7 @@ ipfw_nat(struct ip_fw_args *args, struct cfg_nat *t, struct mbuf *m)
> struct tcphdr *th;
>
> th = (struct tcphdr *)(ip + 1);
> - if (th->th_x2)
> + if (th->th_x2 & (TH_RES1 >> 8))
> ldt = 1;
> }
>
> @@ -436,7 +436,7 @@ ipfw_nat(struct ip_fw_args *args, struct cfg_nat *t, struct mbuf *m)
> * Maybe it was set in
> * libalias...
> */
> - th->th_x2 = 0;
> + th->th_x2 &= ~(TH_RES1 >> 8);
> th->th_sum = cksum;
> mcl->m_pkthdr.csum_data =
> offsetof(struct tcphdr, th_sum);
> --
> 2.37.3
>
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEEjRJAPC5sqwhs9k2jAxRYpUeP4pMFAmNtCOMACgkQAxRYpUeP
4pMmqQ/+Mqoo/bwufQjwmO/FHxf/1//I3g7qDscf6VkzxTq4pT64UtBVFo6TwWJp
Oo8XN3kNwc3ZSmIr0BqSHLs+HhDXdXg+QNO6VvFXDjH05KYkDiEGU/kJve6rfWan
hYQsKng7Axwron+h+Znx3q776FvT/8IV3nh/xqW17nL5AAmVTQ4kORzo/UKlbMvf
zMpVGNwp0YbuAiBAyMNyg7jcwTPs+VXiRgK0yysuqDpN44MujDCJhMvXMK7pIhCu
6XCIK0jAQP7p4PIYbaJG9FO+vAbehPoVF+/EmXjrP0dRvdl7YHNNfmGlj4/s+e9C
f54ONg1fcfLPVVmli+odMGPl1MjAo/u7i8CI4jNGR4f0MtU7U9ztd2NrntWPsFd1
WqNTCZOn7PODS6/Z452G2xQCuWpZLKMpJaN9y9M0hZnmiszbv2/ZEPNnFKrvWKk6
b4ZPncWb6GtUFIC3REWJjBWolOLw3VhMxOPG9Ypd98Fz+Y4GHj71NipGNSZvz2xh
IAvzNw2+HOjBVsPo4xVlirS/IqqI4j2GXRj09c+l4I1eDyKgQG2anjUGOKTFAmAJ
/DqY+DtD1qatLlCDDmd7sjJweATl+hKUSbxRkR8GZ3DZGT1NEZ7LP82F+dYaNs/e
Bb7S90tZwTeMH3siH23QRQShvUkgbJa33lrvovjlzKqONpczWAs=
=Yk49
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20221110142129.GV76435>