On 9/22/26 4:58 PM, [email protected] 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
