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

Zihan Xi <[email protected]> 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: [email protected]
> Reported-by: Vega <[email protected]>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Zihan Xi <[email protected]>
> ---
> 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/[email protected]/
> ---
>  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 <[email protected]>

You can keep this tag in next iteration.