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

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Aug 04, 2026 at 05:19:07PM -0700, Jakub Kicinski wrote:
> 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?
 
No, and it is intentional.

We do not want updates on the conntrack state machine to automatically
end up in -stable.

Do we have to write this in the cover letter from this time on?

> [...] 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?

No, it is just a timeout update in conntrack, there is nothing the
packetdrill test can do about this.

> 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.

Yes, that is expected.

> > 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?

Yes, it is indeed extending the timeout rather intentionally.

> 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.

Yes, and it is intentional, this is lowering the timeout indeed.

Actually, it is lowering it in a less "aggressive" way, instead of
using CLOSE timeout it uses the UNACK timeout (5 MINS) to avoid

> 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?

No, and it is intentional. We are changing to the UNACK timeout which
is less aggresive.

> 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.

Not sure what wording adjustment need to be done here.