Re: [PATCH nf v2 2/2] netfilter: nf_conntrack_tcp: defer tcp_in_window invalid logging until after unlock

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

On Thu, Jul 30, 2026 at 10:28:23PM +0200, Florian Westphal wrote:
> Zihan Xi <zihanx@nebusec.ai> wrote:
> > tcp_in_window() can emit several invalid-packet logs while ct->lock is
> > still held.  If invalid logging is routed to nfnetlink_log with
> > conntrack export enabled, this can re-enter conntrack netlink glue and
> > recurse into tcp_to_nlattr() on the same conntrack.
> > 
> > Fix this by storing only the minimal invalid-log context for the
> > remaining tcp_in_window() cases and emitting the actual log after
> > releasing ct->lock.  Rename the helper to reflect that it now records
> > context instead of logging immediately, and add an explicit lockdep
> > assertion plus comment to document that invalid TCP logs must not be
> > emitted while ct->lock is held.
> > 
> > Fixes: d48668052b26 ("netfilter: fix nf_l4proto_log_invalid to log invalid packets")
> > Cc: stable@vger.kernel.org
> > Reported-by: Vega <vega@nebusec.ai>
> > Assisted-by: Codex:gpt-5.4
> > Signed-off-by: Zihan Xi <zihanx@nebusec.ai>
> > ---
> > changes in v2:
> >   - Keep only the tcp_in_window() invalid logging cases in this patch.
> >   - Rename the helper to reflect that it stores invalid-log context.
> >   - Add a lockdep assertion and comment documenting that invalid logs must not be emitted under ct->lock.
> >   - v1 Link: https://lore.kernel.org/all/cover.1785307980.git.zihanx@nebusec.ai/
> > ---
> >  net/netfilter/nf_conntrack_proto_tcp.c | 113 ++++++++++++++++++-------
> >  1 file changed, 82 insertions(+), 31 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > index ef31dcaffd19..e5875b7528fe 100644
> > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > @@ -480,37 +480,87 @@ static void tcp_init_sender(struct ip_ct_tcp_state *sender,
> >  	}
> >  }
> >  
> > -__printf(6, 7)
> > -static enum nf_ct_tcp_action nf_tcp_log_invalid(const struct sk_buff *skb,
> > -						const struct nf_conn *ct,
> > -						const struct nf_hook_state *state,
> > -						const struct ip_ct_tcp_state *sender,
> > -						enum nf_ct_tcp_action ret,
> > -						const char *fmt, ...)
> > +enum nf_tcp_invalid_log_type {
> > +	NF_TCP_LOG_NONE,
> > +	NF_TCP_LOG_OVERSHOT,
> > +	NF_TCP_LOG_SEQ_OVER,
> > +	NF_TCP_LOG_ACK_OVER,
> > +	NF_TCP_LOG_SEQ_UNDER,
> > +	NF_TCP_LOG_ACK_UNDER,
> > +};
> > +
> > +struct nf_tcp_invalid_log {
> > +	enum nf_tcp_invalid_log_type type;
> > +	u32 value;
> > +};
> > +
> > +static enum nf_ct_tcp_action
> > +nf_tcp_store_invalid(const struct nf_conn *ct,
> > +		     const struct ip_ct_tcp_state *sender,
> > +		     struct nf_tcp_invalid_log *log,
> > +		     enum nf_ct_tcp_action ret,
> > +		     enum nf_tcp_invalid_log_type type,
> > +		     u32 value)
> >  {
> >  	const struct nf_tcp_net *tn = nf_tcp_pernet(nf_ct_net(ct));
> > -	struct va_format vaf;
> > -	va_list args;
> >  	bool be_liberal;
> >  
> >  	be_liberal = sender->flags & IP_CT_TCP_FLAG_BE_LIBERAL || tn->tcp_be_liberal;
> >  	if (be_liberal)
> >  		return NFCT_TCP_ACCEPT;
> >  
> > -	va_start(args, fmt);
> > -	vaf.fmt = fmt;
> > -	vaf.va = &args;
> > -	nf_ct_l4proto_log_invalid(skb, ct, state, "%pV", &vaf);
> > -	va_end(args);
> > -
> > +	log->type = type;
> > +	log->value = value;
> >  	return ret;
> >  }
> >  
> > +static void nf_tcp_log_invalid(const struct sk_buff *skb,
> > +			       const struct nf_conn *ct,
> > +			       const struct nf_hook_state *state,
> > +			       const struct nf_tcp_invalid_log *log)
> > +{
> > +	/* nfnetlink_log may re-enter conntrack attribute dumping and try to
> > +	 * take ct->lock again via tcp_to_nlattr(), so invalid TCP logs must
> > +	 * only be emitted after dropping ct->lock.
> > +	 */
> > +	lockdep_assert_not_held(&ct->lock);
> > +
> > +	switch (log->type) {
> > +	case NF_TCP_LOG_OVERSHOT:
> > +		nf_ct_l4proto_log_invalid(skb, ct, state,
> > +					  "%u bytes more than expected",
> > +					  log->value);
> 
> Can you move the comment and the lockdep assert into
> nf_ct_l4proto_log_invalid() so we can catch offenders outside
> tcp as well?
> 
> SCTP seems to be buggy as well.
> 
> Other than that:
> 
> Reviewed-by: Florian Westphal <fw@strlen.de>
> 
> You can keep this tag in next iteration.

Please, also squash these two patches in one, they are basically the
same logical fix plus include the SCTP fix.

I would suggest you call this patch:

  netfilter: nf_conntrack: defer invalid log until after unlock 

Thanks.