Hi Ilya, Just checking whether you got a chance to look at this patch?
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
