On Mon, Mar 18, 2024 at 10:39:15AM +0100, Pablo Neira Ayuso wrote:
> In case that either FIN or RST packet is seen, infer current TCP state
> based on the TCP packet flags before setting the _TEARDOWN flag:
>
> - FIN packets result in TCP_CONNTRACK_FIN_WAIT which uses a default
> timeout of 2 minutes.
> - RST packets lead to tcp_state TCP_CONNTRACK_CLOSE of 10 seconds.
>
> Therefore, TCP established state with a low timeout is not used anymore
> when handing over the flow to the classic conntrack path, otherwise a
> FIN packet coming in the reply direction could re-offload this flow
> again.
>
> If flow teardown is required for other reasons, eg. no traffic seen
> after NF_FLOW_TIMEOUT, then use TCP established state but set timeout to
> TCP_CONNTRACK_UNACK state which is used in conntrack to pick up
> connections from the middle (default is 5 minutes).
>
> Fixes: e5eaac2beb54 ("netfilter: flowtable: fix TCP flow teardown")
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> Compile-tested only.
>
> include/net/netfilter/nf_flow_table.h | 1 +
> net/netfilter/nf_flow_table_core.c | 45 +++++++++++++++++++++------
> net/netfilter/nf_flow_table_ip.c | 2 +-
> 3 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
> index a763dd327c6e..924f3720143f 100644
> --- a/include/net/netfilter/nf_flow_table.h
> +++ b/include/net/netfilter/nf_flow_table.h
> @@ -293,6 +293,7 @@ int nf_flow_table_init(struct nf_flowtable *flow_table);
> void nf_flow_table_free(struct nf_flowtable *flow_table);
>
> void flow_offload_teardown(struct flow_offload *flow);
> +void flow_offload_teardown_tcp(struct flow_offload *flow, bool fin);
>
> void nf_flow_snat_port(const struct flow_offload *flow,
> struct sk_buff *skb, unsigned int thoff,
> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index a0571339239c..481fe3d96bbc 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -165,10 +165,22 @@ void flow_offload_route_init(struct flow_offload *flow,
> }
> EXPORT_SYMBOL_GPL(flow_offload_route_init);
>
> -static void flow_offload_fixup_tcp(struct ip_ct_tcp *tcp)
> +static s32 flow_offload_fixup_tcp(struct net *net, struct nf_conn *ct,
> + enum tcp_conntrack tcp_state)
> {
> - tcp->seen[0].td_maxwin = 0;
> - tcp->seen[1].td_maxwin = 0;
> + struct nf_tcp_net *tn = nf_tcp_pernet(net);
> +
> + ct->proto.tcp.state = tcp_state;
> + ct->proto.tcp.seen[0].td_maxwin = 0;
> + ct->proto.tcp.seen[1].td_maxwin = 0;
> +
> + /* Similar to mid-connection pickup with loose=1.
> + * Avoid large ESTABLISHED timeout.
> + */
> + if (tcp_state == TCP_CONNTRACK_ESTABLISHED)
> + return tn->timeouts[TCP_CONNTRACK_UNACK];
Hi Pablo,
I tested the patch but the part that sets the timout to UNACK is not
very practical.
For example my long running SSH connections get killed off by the firewall
regularly now while beeing ESTABLISHED:
[NEW] tcp 6 120 SYN_SENT src=192.168.6.55 dst=192.168.10.22 sport=55582 dport=22 [UNREPLIED] src=192.168.10.22 dst=192.168.6.55 sport=22 dport=55582 mark=16777216
[UPDATE] tcp 6 60 SYN_RECV src=192.168.6.55 dst=192.168.10.22 sport=55582 dport=22 src=192.168.10.22 dst=192.168.6.55 sport=22 dport=55582 mark=16777216
[UPDATE] tcp 6 86400 ESTABLISHED src=192.168.6.55 dst=192.168.10.22 sport=55582 dport=22 src=192.168.10.22 dst=192.168.6.55 sport=22 dport=55582 [OFFLOAD] mark=16777216
[DESTROY] tcp 6 ESTABLISHED src=192.168.6.55 dst=192.168.10.22 sport=54941 dport=22 packets=133 bytes=13033 src=192.168.10.22 dst=192.168.6.55 sport=22 dport=54941 packets=95 bytes=15004 [ASSURED] mark=16777216 delta-time=1036
I would remove the if case here.
> +
> + return tn->timeouts[tcp_state];
> }
>
> static void flow_offload_fixup_ct(struct nf_conn *ct)
> @@ -178,12 +190,8 @@ static void flow_offload_fixup_ct(struct nf_conn *ct)
> s32 timeout;
>
> if (l4num == IPPROTO_TCP) {
> - struct nf_tcp_net *tn = nf_tcp_pernet(net);
> -
> - flow_offload_fixup_tcp(&ct->proto.tcp);
> -
> - timeout = tn->timeouts[ct->proto.tcp.state];
> - timeout -= tn->offload_timeout;
> + timeout = flow_offload_fixup_tcp(net, ct,
> + TCP_CONNTRACK_ESTABLISHED);
> } else if (l4num == IPPROTO_UDP) {
> struct nf_udp_net *tn = nf_udp_pernet(net);
> enum udp_conntrack state =
> @@ -346,12 +354,29 @@ static void flow_offload_del(struct nf_flowtable *flow_table,
>
> void flow_offload_teardown(struct flow_offload *flow)
> {
> + flow_offload_fixup_ct(flow->ct);
> + smp_mb__before_atomic();
> clear_bit(IPS_OFFLOAD_BIT, &flow->ct->status);
> set_bit(NF_FLOW_TEARDOWN, &flow->flags);
> - flow_offload_fixup_ct(flow->ct);
> }
> EXPORT_SYMBOL_GPL(flow_offload_teardown);
>
> +void flow_offload_teardown_tcp(struct flow_offload *flow, bool fin)
> +{
> + enum tcp_conntrack tcp_state;
> +
> + if (fin)
> + tcp_state = TCP_CONNTRACK_FIN_WAIT;
> + else /* rst */
> + tcp_state = TCP_CONNTRACK_CLOSE;
> +
> + flow_offload_fixup_tcp(nf_ct_net(flow->ct), flow->ct, tcp_state);
> + smp_mb__before_atomic();
> + clear_bit(IPS_OFFLOAD_BIT, &flow->ct->status);
> + set_bit(NF_FLOW_TEARDOWN, &flow->flags);
> +}
> +EXPORT_SYMBOL_GPL(flow_offload_teardown_tcp);
> +
> struct flow_offload_tuple_rhash *
> flow_offload_lookup(struct nf_flowtable *flow_table,
> struct flow_offload_tuple *tuple)
> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index e45fade76409..13b6c453d8bc 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -29,7 +29,7 @@ static int nf_flow_state_check(struct flow_offload *flow, int proto,
>
> tcph = (void *)(skb_network_header(skb) + thoff);
> if (unlikely(tcph->fin || tcph->rst)) {
> - flow_offload_teardown(flow);
> + flow_offload_teardown_tcp(flow, tcph->fin);
> return -1;
> }
>
> --
> 2.30.2
>
