Re: [PATCH nf-next,v2] netfilter: add DEBUG_NET_WARN_ON_ONCE to skb_set_nfct()

On 8/5/26 11:30 PM, Pablo Neira Ayuso wrote:
> Trigger a warning if nf_ct_set() overlaps an existing ct object leading
> to refcount leak. Add this warning to skb_set_nfct() whose only user is
> nf_ct_set() instead.
> 
> Update existing nf_ct_set() callers to use nf_reset_ct() first to clean
> up stale pointer to conntrack object which migh trigger false positive
> warnings.
> 
> Signed-off-by: Pablo Neira Ayuso <[email protected]>

Reviewed-by: Fernando Fernandez Mancera <[email protected]>

Thanks!

> ---
> sashiko: ignore current overwrites of skb->_nfct in
> net/bridge/netfilter/nf_conntrack_bridge.c:nf_ct_bridge_pre(),
> that will be fixed in a separated patch.
> 
>   include/linux/skbuff.h            |  1 +
>   include/net/ip_vs.h               |  2 +-
>   net/netfilter/nf_conntrack_core.c |  2 +-
>   net/openvswitch/conntrack.c       | 12 +++---------
>   net/sched/act_ct.c                |  6 +++---
>   5 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 22eda1d54a0e..95184183180f 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -5004,6 +5004,7 @@ static inline unsigned long skb_get_nfct(const struct sk_buff *skb)
>   static inline void skb_set_nfct(struct sk_buff *skb, unsigned long nfct)
>   {
>   #if IS_ENABLED(CONFIG_NF_CONNTRACK)
> +	DEBUG_NET_WARN_ON_ONCE(skb->_nfct & NFCT_PTRMASK);
>   	skb->slow_gro |= !!nfct;
>   	skb->_nfct = nfct;
>   #endif
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 8feb69158816..9c6c5336de1c 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -2130,7 +2130,7 @@ static inline void ip_vs_notrack(struct sk_buff *skb)
>   	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
>   
>   	if (ct) {
> -		nf_conntrack_put(&ct->ct_general);
> +		nf_reset_ct(skb);
>   		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>   	}
>   #endif
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 784bd1d7a9bf..d0d9e5ea84a0 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1031,7 +1031,7 @@ static int __nf_ct_resolve_clash(struct sk_buff *skb,
>   		nf_conntrack_get(&ct->ct_general);
>   
>   		nf_ct_acct_merge(ct, ctinfo, loser_ct);
> -		nf_ct_put(loser_ct);
> +		nf_reset_ct(skb);
>   		nf_ct_set(skb, ct, ctinfo);
>   
>   		NF_CT_STAT_INC(net, clash_resolve);
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 95697d4e16e6..4dd82c4e87d3 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -603,7 +603,7 @@ static bool skb_nfct_cached(struct net *net,
>   		if (nf_ct_is_confirmed(ct))
>   			nf_ct_delete(ct, 0, 0);
>   
> -		nf_ct_put(ct);
> +		nf_reset_ct(skb);
>   		nf_ct_set(skb, NULL, 0);
>   		return false;
>   	}
> @@ -745,8 +745,7 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>   
>   		/* Associate skb with specified zone. */
>   		if (tmpl) {
> -			ct = nf_ct_get(skb, &ctinfo);
> -			nf_ct_put(ct);
> +			nf_reset_ct(skb);
>   			nf_conntrack_get(&tmpl->ct_general);
>   			nf_ct_set(skb, tmpl, IP_CT_NEW);
>   		}
> @@ -1075,12 +1074,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
>   
>   int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
>   {
> -	enum ip_conntrack_info ctinfo;
> -	struct nf_conn *ct;
> -
> -	ct = nf_ct_get(skb, &ctinfo);
> -
> -	nf_ct_put(ct);
> +	nf_reset_ct(skb);
>   	nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>   
>   	if (key)
> diff --git a/net/sched/act_ct.c b/net/sched/act_ct.c
> index 4ca7964e83c8..7f54fb4e4ec9 100644
> --- a/net/sched/act_ct.c
> +++ b/net/sched/act_ct.c
> @@ -782,7 +782,7 @@ static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
>   	return true;
>   
>   drop_ct:
> -	nf_ct_put(ct);
> +	nf_reset_ct(skb);
>   	nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>   
>   	return false;
> @@ -996,7 +996,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>   		qdisc_skb_cb(skb)->post_ct = false;
>   		ct = nf_ct_get(skb, &ctinfo);
>   		if (ct) {
> -			nf_ct_put(ct);
> +			nf_reset_ct(skb);
>   			nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>   		}
>   
> @@ -1034,7 +1034,7 @@ TC_INDIRECT_SCOPE int tcf_ct_act(struct sk_buff *skb, const struct tc_action *a,
>   
>   		/* Associate skb with specified zone. */
>   		if (tmpl) {
> -			nf_conntrack_put(skb_nfct(skb));
> +			nf_reset_ct(skb);
>   			nf_conntrack_get(&tmpl->ct_general);
>   			nf_ct_set(skb, tmpl, IP_CT_NEW);
>   		}