> On 26 May 2026, at 6:37 PM, Ilya Maximets <[email protected]> wrote: > > !-------------------------------------------------------------------| > CAUTION: External Email > > |-------------------------------------------------------------------! > > On 5/26/26 5:53 AM, Naveen Yerramneni wrote: >> >> Hi Ilya, >> >> Just checking whether you got a chance to look at this patch? > > Hi, Naveen. Yes, I did. The problem however, is that the entire logic > behind the reversible_actions() call is broken by design. The issue you > described is valid, but the solution should not be specific to conntrack > or ct_clear. Just looking at the OpenFlow actions we can't really tell > what kind of datapath actions will be emitted and if they are reversible > or not. For these pipeline actions, there can be anything in there, > so the right solution here will be to always unconditionally clone. > But that is not a good solution, as it will trigger a lot of unnecessary > clones on the datapath level. And the previous patch for ct_clear > already cause that problem here: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2026-2DMay_432488.html&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA&m=oXA2IV6TKkiyx4caK-10xaGaX-sNubwSQ7NCCYwn34OqyiklsnQcA0g_arahwVoK&s=AHRsco8GLjLT41m1iLxmu8ws54BXnFX8tsIP8C_PfJM&e= > > > We should be checking the actual datapath actions, and not OpenFlow. > There was an old thread 4 years ago where I raised this same point: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2022-2DJuly_396578.html&d=DwIDaQ&c=s883GpUCOChKOHiocYtGcg&r=2PQjSDR7A28z1kXE1ptSm6X36oL_nCq1XxeEt7FkLmA&m=oXA2IV6TKkiyx4caK-10xaGaX-sNubwSQ7NCCYwn34OqyiklsnQcA0g_arahwVoK&s=ROURudnNSvanPQpv3Ba3MHYE4GKWS4j2gUwbCitkTYw&e= > > > I do have some code for it and I'll be posting it today or a bit later > this week. It will be a noticeably large change though and it has > its own side effects, but should be much more correct.
Thanks Ilya for sharing the details! > > Best regards, Ilya Maximets. > >> >> Thanks, >> Naveen >> >> >> >>> On 19 May 2026, at 9:58 PM, Naveen Yerramneni >>> <[email protected]> wrote: >>> >>> Commit 2df0987e8222 moved OFPACT_CT_CLEAR to the non-reversible list, so >>> clone(ct_clear, ...) goes through the datapath-clone path and the >>> ct_clear is isolated from the outer context. >>> >>> reversible_actions() only inspects the immediate actions of the clone. >>> When those actions are just a resubmit (or goto_table, group, nested >>> clone), the clone is still classified as reversible. The rule reached >>> through that indirection can still issue ct_clear (or ct/nat). On a >>> tracked packet, the reversible path then emits that inner ct_clear >>> inline, which clears ct_state on the original packet, so a later >>> ct_clear after the clone becomes a no-op. >>> >>> Example flow: >>> table=1,ct_state=+trk,ip,actions=clone(resubmit(,10)), >>> ct_clear,resubmit(,20) >>> table=10,ip,actions=ct_clear,output:2 >>> The original packet loses its ct_state inside the clone and the outer >>> ct_clear is skipped. >>> >>> Treat resubmit, goto_table, group and nested clone as non-reversible >>> when the packet is tracked. The clone body is then wrapped in a >>> datapath clone() and its conntrack effects stay inside the clone. >>> Untracked packets keep the existing optimization. >>> >>> Test added in tests/ofproto-dpif.at. >>> >>> Fixes: 2df0987e8222 ("ofproto-dpif-xlate: Classify ct_clear as >>> non-reversible for clone().") >>> Signed-off-by: Naveen Yerramneni <[email protected]> >>> --- >>> ofproto/ofproto-dpif-xlate.c | 26 +++++++++++++++++++------- >>> tests/ofproto-dpif.at | 21 +++++++++++++++++++++ >>> 2 files changed, 40 insertions(+), 7 deletions(-) >>> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>> index c1ba447e9..6ac525e1d 100644 >>> --- a/ofproto/ofproto-dpif-xlate.c >>> +++ b/ofproto/ofproto-dpif-xlate.c >>> @@ -6113,9 +6113,15 @@ xlate_sample_action(struct xlate_ctx *ctx, >>> * >>> * Openflow actions that do not emit datapath actions are trivially >>> * reversible. Reversiblity of other actions depends on nature of >>> - * action and their translation. */ >>> + * action and their translation. >>> + * >>> + * if conntracked is true, actions that indirectly run other OF actions >>> + * (resubmit, goto_table, group, nested clone) are treated as >>> non-reversible >>> + * since they may reach a ct_clear/ct/nat that would mutate the original >>> + * tracked packet. */ >>> static bool >>> -reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len) >>> +reversible_actions(const struct ofpact *ofpacts, size_t ofpacts_len, >>> + bool conntracked) >>> { >>> const struct ofpact *a; >>> >>> @@ -6123,7 +6129,6 @@ reversible_actions(const struct ofpact *ofpacts, >>> size_t ofpacts_len) >>> switch (a->type) { >>> case OFPACT_BUNDLE: >>> case OFPACT_CLEAR_ACTIONS: >>> - case OFPACT_CLONE: >>> case OFPACT_CONJUNCTION: >>> case OFPACT_CONTROLLER: >>> case OFPACT_DEBUG_RECIRC: >>> @@ -6133,8 +6138,6 @@ reversible_actions(const struct ofpact *ofpacts, >>> size_t ofpacts_len) >>> case OFPACT_ENQUEUE: >>> case OFPACT_EXIT: >>> case OFPACT_FIN_TIMEOUT: >>> - case OFPACT_GOTO_TABLE: >>> - case OFPACT_GROUP: >>> case OFPACT_LEARN: >>> case OFPACT_MULTIPATH: >>> case OFPACT_NOTE: >>> @@ -6145,7 +6148,6 @@ reversible_actions(const struct ofpact *ofpacts, >>> size_t ofpacts_len) >>> case OFPACT_PUSH_MPLS: >>> case OFPACT_PUSH_VLAN: >>> case OFPACT_REG_MOVE: >>> - case OFPACT_RESUBMIT: >>> case OFPACT_SAMPLE: >>> case OFPACT_SET_ETH_DST: >>> case OFPACT_SET_ETH_SRC: >>> @@ -6174,6 +6176,15 @@ reversible_actions(const struct ofpact *ofpacts, >>> size_t ofpacts_len) >>> case OFPACT_DELETE_FIELD: >>> break; >>> >>> + case OFPACT_CLONE: >>> + case OFPACT_GOTO_TABLE: >>> + case OFPACT_GROUP: >>> + case OFPACT_RESUBMIT: >>> + if (conntracked) { >>> + return false; >>> + } >>> + break; >>> + >>> case OFPACT_CT: >>> case OFPACT_CT_CLEAR: >>> case OFPACT_METER: >>> @@ -6198,7 +6209,8 @@ clone_xlate_actions(const struct ofpact *actions, >>> size_t actions_len, >>> >>> retained_state = xretain_state_save(ctx); >>> >>> - if (reversible_actions(actions, actions_len) || is_last_action) { >>> + if (reversible_actions(actions, actions_len, ctx->conntracked) >>> + || is_last_action) { >>> do_xlate_actions(actions, actions_len, ctx, is_last_action, false); >>> if (!ctx->freezing) { >>> xlate_action_set(ctx); >>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >>> index dcab7bba4..0cc8c90d5 100644 >>> --- a/tests/ofproto-dpif.at >>> +++ b/tests/ofproto-dpif.at >>> @@ -9322,6 +9322,27 @@ Datapath actions: ct,recirc(X) >>> Datapath actions: clone(ct_clear,2),ct_clear,3 >>> ]) >>> >>> +dnl On a tracked packet, a resubmit/goto_table/group/nested-clone inside >>> +dnl clone() may reach a ct_clear in another table. Such an inner ct_clear >>> +dnl must be emitted inside a datapath clone() wrapper so it cannot mutate >>> +dnl the original packet, and the outer ct_clear must still take effect. >>> +AT_DATA([flows.txt], [dnl >>> +table=0,in_port=1,ip,actions=ct(table=1) >>> +table=1,ct_state=+trk,ip,actions=clone(resubmit(,10)),ct_clear,resubmit(,20) >>> +table=10,ip,actions=ct_clear,output:2 >>> +table=20,ct_state=+trk,ip,actions=drop >>> +table=20,priority=10,ip,actions=output:3 >>> +]) >>> +AT_CHECK([ovs-ofctl del-flows br0]) >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore]) >>> + >>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy >>> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'], >>> [0], [stdout]) >>> + >>> +AT_CHECK([grep Datapath stdout | sed 's/recirc(.*)/recirc(X)/'], [0], [dnl >>> +Datapath actions: ct,recirc(X) >>> +Datapath actions: clone(ct_clear,2),ct_clear,3 >>> +]) >>> + >>> OVS_VSWITCHD_STOP >>> AT_CLEANUP >>> >>> -- >>> 2.43.5 >>> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
