Re: [PATCH net-next 9/9] netfilter: conntrack: tcp: use UNACK timeout for non-closing RST packets

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: conntrack: tcp: use UNACK timeout for non-closing RST packets

This changes the timeout selection in nf_conntrack_tcp_packet() so that an
RST which leaves the entry in TCP_CONNTRACK_ESTABLISHED (the non-exact
sequence case from be0502a3f2e9) gets timeouts[TCP_CONNTRACK_UNACK] instead
of timeouts[TCP_CONNTRACK_CLOSE].  RSTs that conntrack accepts as closing
the flow are meant to keep using timeouts[new_state].

A few questions below.

>     Use the UNACK timeout for RST packets that leave the conntrack entry in
>     TCP_CONNTRACK_ESTABLISHED.  Exact-match RSTs and accepted RST packet
>     trains still fall through to timeouts[new_state], which preserves the
>     CLOSE timeout when conntrack accepts the RST as closing the flow.

[Severity: Low]
Should this carry a Fixes: tag?  The changelog describes a user-visible
failure (an ESTABLISHED entry expiring after 10 seconds while both TCP
endpoints stay established), and it names be0502a3f2e9 in prose only, so
stable tooling has nothing machine-readable to key on.

There is also a wording tension: be0502a3f2e9's own changelog presented the
CLOSE timeout for non-exact RSTs as intentional ("With patch, timeout is
lowered like CLOSE, but connection stays in ESTABLISHED state" / "If no
challenge ACK is received, the connection will time out after CLOSE
seconds").  Either adding

    Fixes: be0502a3f2e94211 ("netfilter: conntrack: tcp: only close if RST matches exact sequence")

or rewording this as a deliberate policy refinement rather than a bug fix
would make the two consistent.

[Severity: Low]
Is there a test that observes this change?  The only in-tree artifact
covering the inexact in-window RST path,
tools/testing/selftests/net/netfilter/packetdrill/conntrack_rst_invalid.pkt,
checks only the state string:

    // in-window but not exact match
    +0.0 < R	42:42(0)	win 0
    +0 `conntrack -f $NFCT_IP_VERSION -L -p tcp --dport 8080 2>/dev/null |grep -q ESTABLISHED`

The timeout is now the sole observable difference for that scenario, and it
is never inspected.

On top of that, the segment labelled "in-window but not exact match" has seq
before seen[!dir].td_maxack (the peer had acked 3001), so
nf_conntrack_tcp_packet() rejects it at the "invalid rst" check and never
reaches the patched timeout selection.  The test's own counter assertion
agrees:

    // Expect four invalid RSTs
    +0 `$xtables -v -S INPUT  | grep INVALID | grep -q -- "-c 4 "`

So the suite passes identically with and without this change.

> diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> index ceeed3d7fe520..723e946a78f40 100644
> --- a/net/netfilter/nf_conntrack_proto_tcp.c
> +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> @@ -1281,8 +1281,9 @@ int nf_conntrack_tcp_packet(struct nf_conn *ct,
>  	if (ct->proto.tcp.retrans >= tn->tcp_max_retrans &&
>  	    timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
>  		timeout = timeouts[TCP_CONNTRACK_RETRANS];
> -	else if (unlikely(index == TCP_RST_SET))
> -		timeout = timeouts[TCP_CONNTRACK_CLOSE];
> +	else if (unlikely(index == TCP_RST_SET &&
> +			  new_state == TCP_CONNTRACK_ESTABLISHED))
> +		timeout = timeouts[TCP_CONNTRACK_UNACK];

[Severity: Medium]
Can this new branch lengthen an entry's lifetime rather than shorten it?
Every sibling branch in this chain only assigns when the replacement is
smaller, for example the RETRANS branch just above and the UNACK branch just
below:

    if (ct->proto.tcp.retrans >= tn->tcp_max_retrans &&
        timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
        timeout = timeouts[TCP_CONNTRACK_RETRANS];
    ...
    else if ((ct->proto.tcp.seen[0].flags | ct->proto.tcp.seen[1].flags) &
         IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED &&
         timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])
        timeout = timeouts[TCP_CONNTRACK_UNACK];

The new branch has no such guard.  Since this branch is only reachable via
"new_state = old_state;" in case TCP_CONNTRACK_CLOSE, new_state is
ESTABLISHED here, so the effective comparison is timeouts[UNACK] against
timeouts[ESTABLISHED].

With the defaults in tcp_timeouts (ESTABLISHED 5 DAYS, UNACK 5 MINS) this is
always a lowering.  But both values are independently tunable with no
cross-validation:

    net/netfilter/nf_conntrack_standalone.c:
    [NF_SYSCTL_CT_PROTO_TIMEOUT_TCP_ESTABLISHED] = {
        .procname	= "nf_conntrack_tcp_timeout_established",
        .maxlen		= sizeof(unsigned int),
        .mode		= 0644,
        .proc_handler	= proc_dointvec_jiffies,
    },

and tcp_timeout_nlattr_to_obj() copies CTA_TIMEOUT_TCP_* verbatim.  So an
administrator lowering nf_conntrack_tcp_timeout_established (120 s is a
common hardening/NAT-appliance setting) inverts the relation.

__nf_ct_refresh_acct() then writes the expiry absolutely rather than
clamping it:

    net/netfilter/nf_conntrack_core.c:__nf_ct_refresh_acct() {
        ...
        if (nf_ct_is_confirmed(ct))
            extra_jiffies += nfct_time_stamp;

        if (READ_ONCE(ct->timeout) != extra_jiffies)
            WRITE_ONCE(ct->timeout, extra_jiffies);
        ...
    }

In that configuration, an in-window RST whose sequence does not match
td_maxack pushes the entry past the configured ESTABLISHED policy, and a
periodic stream of such RSTs can renew it indefinitely, holding entries
against nf_conntrack_max.  Would the usual form keep the intent while
avoiding this?

    timeout = min(timeouts[new_state], timeouts[TCP_CONNTRACK_UNACK]);

or the matching guard:

    else if (unlikely(index == TCP_RST_SET &&
                      new_state == TCP_CONNTRACK_ESTABLISHED) &&
             timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])

>  	else if ((ct->proto.tcp.seen[0].flags | ct->proto.tcp.seen[1].flags) &
>  		 IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED &&
>  		 timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])

[Severity: Low]
For an RST that conntrack does accept as closing the flow, is the CLOSE
timeout really preserved?  Previously such packets took the unconditional
timeouts[TCP_CONNTRACK_CLOSE] assignment; now they fall through into the
DATA_UNACKNOWLEDGED and last_win == 0 branches:

    else if ((ct->proto.tcp.seen[0].flags | ct->proto.tcp.seen[1].flags) &
         IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED &&
         timeouts[new_state] > timeouts[TCP_CONNTRACK_UNACK])
        timeout = timeouts[TCP_CONNTRACK_UNACK];
    else if (ct->proto.tcp.last_win == 0 &&
         timeouts[new_state] > timeouts[TCP_CONNTRACK_RETRANS])
        timeout = timeouts[TCP_CONNTRACK_RETRANS];

Those fire precisely when nf_conntrack_tcp_timeout_close is raised above
nf_conntrack_tcp_timeout_unacknowledged or the retrans timeout, and both
preconditions are reachable for a closing RST: IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED
is commonly set on a flow that carried data, and ct->proto.tcp.last_win is
sticky since it is only refreshed for ACK-only segments.

This also diverges from the other closing path in the same file, which does
not cap:

    nf_conntrack_tcp_set_closing() {
        ...
        timeout = timeouts[TCP_CONNTRACK_CLOSE];
        WRITE_ONCE(ct->timeout, timeout + nfct_time_stamp);
        ...
    }

Given that, is the changelog sentence "Exact-match RSTs and accepted RST
packet trains still fall through to timeouts[new_state], which preserves the
CLOSE timeout when conntrack accepts the RST as closing the flow" accurate
for non-default close timeouts?  Restoring an explicit CLOSE assignment for
new_state == TCP_CONNTRACK_CLOSE, or adjusting the wording, would resolve
it.