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

On 9/22/26 4:58 PM, netdev-bot+sashiko@kernel.org 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.

quoted

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.

quoted

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.