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://mail.openvswitch.org/pipermail/ovs-dev/2026-May/432488.html 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://mail.openvswitch.org/pipermail/ovs-dev/2022-July/396578.html 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. 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
