Re: [PATCH 4/9 nf-next] netfilter: conntrack: use DEBUG_NET_WARN_ON_ONCE on packet paths

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

Hi Fernando,

I'm a making a bit more in-depth review for this specific patch.

I think, in general about this series, it would be good to avoid,
things like:

        DEBUG_NET_WARN_ON_ONCE(blah);

        func(blah->info, ...);

but it might not be trivial in all cases, sometimes it is simply
better to remove in that case.

The patch in this series for nf_tables (already upstream) always
follow the idiom:

        if (cond) {
                DEBUG_NET_WARN_ON_ONCE(1);
                terminal statament (ie. return, break...)
        }

I will try to provide you with hints on what to do in other patches in
this series to speed up inclusion.

Now comments on this specific patch, see below.

On Mon, Jun 01, 2026 at 09:30:44PM +0200, Fernando Fernandez Mancera wrote:
> Replace WARN_ON and WARN_ON_ONCE with DEBUG_NET_WARN_ON_ONCE inside
> conntrack confirmation, extension management, helper assignment, and
> protocol parsing loops. This prevents unnecessary system panics when
> panic_on_warn=1 is enabled in production systems.
> 
> Signed-off-by: Fernando Fernandez Mancera <fmancera@suse.de>
> ---
>  net/netfilter/nf_conntrack_core.c       | 2 +-
>  net/netfilter/nf_conntrack_extend.c     | 3 ++-
>  net/netfilter/nf_conntrack_helper.c     | 4 +++-
>  net/netfilter/nf_conntrack_ovs.c        | 2 +-
>  net/netfilter/nf_conntrack_proto_icmp.c | 3 ++-
>  net/netfilter/nf_conntrack_seqadj.c     | 2 +-
>  net/netfilter/nf_conntrack_sip.c        | 5 ++++-
>  7 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 8ba5b22a1eef..51e2d8ebe756 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1244,7 +1244,7 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>  	 * unconfirmed conntrack.
>  	 */
>  	if (unlikely(nf_ct_is_confirmed(ct))) {
> -		WARN_ON_ONCE(1);
> +		DEBUG_NET_WARN_ON_ONCE(1);
>  		nf_conntrack_double_unlock(hash, reply_hash);
>  		local_bh_enable();
>  		return NF_DROP;

OK, explicit drop, fine. Keep it.

> diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
> index dd62cc12e775..68169007aea2 100644
> --- a/net/netfilter/nf_conntrack_extend.c
> +++ b/net/netfilter/nf_conntrack_extend.c
> @@ -95,7 +95,8 @@ void *nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
>  	struct nf_ct_ext *new;
>  
>  	/* Conntrack must not be confirmed to avoid races on reallocation. */
> -	WARN_ON(nf_ct_is_confirmed(ct));
> +	if (unlikely(nf_ct_is_confirmed(ct)))
> +		DEBUG_NET_WARN_ON_ONCE(1);

Keep it, but return NULL here. It provide good context, extensions can
only be added with a unconfirmed conntrack.

>  	/* struct nf_ct_ext uses u8 to store offsets/size */
>  	BUILD_BUG_ON(total_extension_size() > 255u);
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 17e971bd4c74..0a0e41dd4c95 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -198,8 +198,10 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
>  	if (test_bit(IPS_HELPER_BIT, &ct->status))
>  		return 0;
>  
> -	if (WARN_ON_ONCE(!tmpl))
> +	if (unlikely(!tmpl)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
>  		return 0;
> +	}

Useless for netfilter:

        if (!exp && tmpl)
                __nf_ct_try_assign_helper(ct, tmpl, GFP_ATOMIC);

_BUT_ it can catch bugs in other existing users, eg. net/sched/act_ct.c

        if (!nf_ct_is_confirmed(ct) && commit && p->helper && !nfct_help(ct)) {
                err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);

keep it.

it is also fine that there is a branch and return (to skip it).

>  	help = nfct_help(tmpl);
>  	if (help != NULL) {
> diff --git a/net/netfilter/nf_conntrack_ovs.c b/net/netfilter/nf_conntrack_ovs.c
> index a6988eeb1579..26f12dd0c1a4 100644
> --- a/net/netfilter/nf_conntrack_ovs.c
> +++ b/net/netfilter/nf_conntrack_ovs.c
> @@ -53,7 +53,7 @@ int nf_ct_helper(struct sk_buff *skb, struct nf_conn *ct,
>  		break;
>  	}
>  	default:
> -		WARN_ONCE(1, "helper invoked on non-IP family!");
> +		DEBUG_NET_WARN_ONCE(1, "helper invoked on non-IP family!");
>  		return NF_DROP;

OK, this is in a branch with an explicit action (drop packet) LGTm.

>  	}
>  
> diff --git a/net/netfilter/nf_conntrack_proto_icmp.c b/net/netfilter/nf_conntrack_proto_icmp.c
> index 32148a3a8509..0f39cb147c4f 100644
> --- a/net/netfilter/nf_conntrack_proto_icmp.c
> +++ b/net/netfilter/nf_conntrack_proto_icmp.c
> @@ -117,7 +117,8 @@ int nf_conntrack_inet_error(struct nf_conn *tmpl, struct sk_buff *skb,
>  	enum ip_conntrack_dir dir;
>  	struct nf_conn *ct;
>  
> -	WARN_ON(skb_nfct(skb));
> +	if (unlikely(skb_nfct(skb)))
> +		DEBUG_NET_WARN_ON_ONCE(1);

nf_conntrack_in
 [ reset skb->nfct ]
 nf_conntrack_handle_icmp
  nf_conntrack_icmpv4_error
   nf_conntrack_inet_error

There is nf_conntrack_inet_error() which performs the ct lookup.
There is resolve_normal_ct() too, but these two are coming later.

[ ... snippet that resets skb->nfct ... ]
unsigned int
nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
{
        enum ip_conntrack_info ctinfo;
        struct nf_conn *ct, *tmpl;
        u_int8_t protonum;
        int dataoff, ret;
 
        tmpl = nf_ct_get(skb, &ctinfo);
        if (tmpl || ctinfo == IP_CT_UNTRACKED) {
                /* Previously seen (loopback or untracked)?  Ignore. */
                if ((tmpl && !nf_ct_is_template(tmpl)) ||
                     ctinfo == IP_CT_UNTRACKED)
                        return NF_ACCEPT;
                skb->_nfct = 0; <---------  this is reset here.
        }
[ end of snippet ]

I don't remember to have seen this WARN_ON, so remove it.

>  	zone = nf_ct_zone_tmpl(tmpl, skb, &tmp);
>  
>  	/* Are they talking about one of our connections? */
> diff --git a/net/netfilter/nf_conntrack_seqadj.c b/net/netfilter/nf_conntrack_seqadj.c
> index 7ab2b25b57bc..2bf49f0b9406 100644
> --- a/net/netfilter/nf_conntrack_seqadj.c
> +++ b/net/netfilter/nf_conntrack_seqadj.c
> @@ -38,7 +38,7 @@ int nf_ct_seqadj_set(struct nf_conn *ct, enum ip_conntrack_info ctinfo,
>  		return 0;
>  
>  	if (unlikely(!seqadj)) {
> -		WARN_ONCE(1, "Missing nfct_seqadj_ext_add() setup call\n");
> +		DEBUG_NET_WARN_ONCE(1, "Missing nfct_seqadj_ext_add() setup call\n");

This WARN_ONCE is now gone in the nf.git/nf-next.git.

>  		return 0;
>  	}
>  
> diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
> index e69941f1a101..7e9237c810a0 100644
> --- a/net/netfilter/nf_conntrack_sip.c
> +++ b/net/netfilter/nf_conntrack_sip.c
> @@ -599,7 +599,10 @@ int ct_sip_parse_header_uri(const struct nf_conn *ct, const char *dptr,
>  
>  	ret = ct_sip_walk_headers(ct, dptr, dataoff ? *dataoff : 0, datalen,
>  				  type, in_header, matchoff, matchlen);
> -	WARN_ON(ret < 0);
> +	if (unlikely(ret < 0)) {
> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return -1;
> +	}

ct_sip_walk_headers() can never return < 0. This WARN_ON can be
removed.

>  	if (ret == 0)
>  		return ret;

Thanks.