Re: [PATCH net 1/6] net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry

[Ilya Maximets]

On 9/22/26 4:58 PM, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] New behaviour change introduced by the guard in
>   __ovs_ct_lookup() (net/openvswitch/conntrack.c:741-747): when two skbs…
> - [Low] nf_ct_shared() is added as an undocumented generic predicate in
>   include/net/netfilter/nf_conntrack.h:188-191 returning…
> 
> Pre-existing issues:
> - [Low] Peer call site net/sched/act_ct.c (tcf_ct_act()) performs the
>   same 'cached, unconfirmed ct + commit modifies the extension set'…
> 
> --
> 
> net: openvswitch: conntrack: avoid modifying shared unconfirmed ct entry
> 
> This adds a new nf_ct_shared() helper to the conntrack header and uses it in
> __ovs_ct_lookup() to reset the cached conntrack entry when an skb arrives at
> a committing ct action with an unconfirmed entry that is shared with another
> skb.  The goal is to avoid two clones committing the same unconfirmed entry
> with different extension sets, which can lead to a use-after-free.
> 
> A few questions below.
> 
>> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
>> index bc42dd0e10e65..c39425e54d87d 100644
>> --- a/include/net/netfilter/nf_conntrack.h
>> +++ b/include/net/netfilter/nf_conntrack.h
>> @@ -185,6 +185,11 @@ static inline void nf_ct_put(struct nf_conn *ct)
>>  		nf_ct_destroy(&ct->ct_general);
>>  }
>>  
>> +static inline bool nf_ct_shared(const struct nf_conn *ct)
>> +{
>> +	return refcount_read(&ct->ct_general.use) > 1;
>> +}
>> +
> 
> [Severity: Low]
> Would it be worth documenting that this predicate is only meaningful for
> unconfirmed, non-template entries?
> 
> The reference accounting comment on struct nf_conn in this same header says
> the count is "1 for hash table, 1 per skb, plus 1 for any connection(s) we
> are `master' for".  With that, every confirmed entry owned by a single skb
> already has use == 2, since __nf_conntrack_confirm() does:
> 
> 	/* The caller holds a reference to this object */
> 	refcount_set(&ct->ct_general.use, 2);
> 
> The same is true for a template in use: nf_ct_tmpl_alloc() sets use = 1 and
> nf_ct_set(skb, tmpl, ...) adds another.  So nf_ct_shared() returns true for
> an ordinary single-owner confirmed entry and for an in-use template, while
> the name reads as "more than one skb owns this ct".

The comments in the callers state the cloned skb as an example, not as a
sole reason for the entry being shared.  "shared" just means there is more
than one reference here.  I can add a comment to the function itself if
someone thinks this is not clear enough, but otherwise I'd keep as is.

> Both consumers at the end of the series check !nf_ct_is_confirmed(ct) first,
> so there is no misbehaviour today, but a future caller without that check
> would get a surprising answer.
> 
> There is also already an open-coded twin of this test in
> nf_ct_drop_unconfirmed() in net/netfilter/nfnetlink_queue.c, which states the
> precondition ("conntrack assumes exclusive skb->_nfct ownership for
> unconfirmed entries") and applies it only after:
> 
> 	if (!ct || nf_ct_is_confirmed(ct))
> 		return false;
> 
> Should that one be converted to the new helper so there is a single
> definition of "shared"?

nfnetlink_queue needs some work on its own and can be cleaned up separately.

> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but the helper lands in the
> shared netfilter header while the equivalent call site in net/sched/act_ct.c
> is left alone here.
> 
> tcf_ct_act() runs the same sequence on a cached unconfirmed entry, adding
> extensions and running helpers:
> 
> 	err = __nf_ct_try_assign_helper(ct, p->tmpl, GFP_ATOMIC);
> 	...
> 	if (p->ct_action & TCA_CT_ACT_NAT && !nfct_seqadj(ct)) {
> 		if (!nfct_seqadj_ext_add(ct))
> 			goto drop;
> 
> tcf_ct_skb_nfct_cached() has no exclusivity test either.
> 
> For what it is worth, the follow-up patch in this series, "net/sched:
> act_ct: avoid modifying shared unconfirmed ct entry", adds the same guard to
> tcf_ct_act() and extends it to the NAT-without-commit case, so the gap is
> closed by the end of the series.  Mentioning that ordering in this changelog
> might help backporters.

Preexisting, fixed later in the set.

> 
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index 0f433688e17b9..a733029c28dd0 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -734,6 +734,18 @@ static int __ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
>>  	enum ip_conntrack_info ctinfo;
>>  	struct nf_conn *ct;
>>  
>> +	/* If the ct entry is not confirmed and shared with some other skb,
>> +	 * e.g., a cloned one, we can't just modify it with the commit as we
>> +	 * must not modify the extension set.  Reset.
>> +	 */
>> +	if (cached && info->commit) {
>> +		ct = nf_ct_get(skb, &ctinfo);
>> +		if (ct && !nf_ct_is_confirmed(ct) && nf_ct_shared(ct)) {
>> +			nf_reset_ct(skb);
>> +			cached = false;
>> +		}
>> +	}
>> +
> 
> [Severity: Medium]
> Since the test is a point-in-time refcount snapshot, does it exempt the last
> remaining holder?
> 
> With two skbs sharing one unconfirmed entry and both executing ct(commit) in
> the same zone:
> 
>   - the first one to reach the check sees use == 2, so nf_reset_ct() drops
>     its reference (2 -> 1), nf_conntrack_in() runs again and it commits a
>     brand new entry with the same tuple.
> 
>   - the second holder now sees use == 1, so nf_ct_shared() is false, it
>     keeps the stale unconfirmed entry and commits that.
> 
> __nf_conntrack_confirm() then finds the equal tuple already in
> nf_conntrack_hash and falls into nf_ct_resolve_clash(), where:
> 
> 	l4proto = nf_ct_l4proto_find(nf_ct_protonum(ct));
> 	if (!l4proto->allow_clash)
> 		goto drop;
> 
> For TCP, SCTP and DCCP that returns NF_DROP, and verdict_to_errno(NF_DROP)
> == -EINVAL propagates through ovs_ct_commit() and ovs_ct_execute(), so that
> packet is dropped with OVS_DROP_CONNTRACK.  Before the patch both clones
> were accepted, as the second nf_conntrack_confirm() was a no-op on the
> already confirmed shared entry.
> 
> For UDP and ICMP the clash resolution succeeds, but __nf_ct_resolve_clash()
> discards the losing entry, so the mark and labels the second clone just
> wrote are silently lost.
> 
> The changelog says the reset "doesn't affect any known use cases".  Should
> the drop and the mark/label loss be spelled out there, given both are
> visible consequences for a pipeline that commits two clones into the same
> zone?
> 
> The clone precondition itself comes from __nf_copy() in
> include/linux/skbuff.h, which unconditionally takes a ct reference:
> 
> 	dst->_nfct = src->_nfct;
> 	nf_conntrack_get(skb_nfct(src));
> 
> and skb_nfct_cached() returns true for the surviving clone since it only
> checks netns, zone, helper, timeout and force.
> 

All of that can happen even without cloning.  With or without the changes
applied.  The entry can be created separately and then clash with the one
in the skb causing a drop.  This is expected.  The "use case" is also not
meaningful in practice.

Best regards, Ilya Maximets.