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
