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

[netdev-bot+sashiko]

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".

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"?

[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.

> 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.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921145655.3167436-1-i.maximets%40ovn.org