clone_xlate_actions() has two code paths. The non-reversible path
saves and restores ctx->conntracked across the inner actions, but
the reversible path does not.
This is a problem for flows like:
clone(ct_clear, <actions>), ct_clear, resubmit(,N)
The ct_clear inside the clone sets ctx->conntracked to false. After
the clone returns, the flag stays false but flow.ct_state is
restored by xretain_state_restore_and_free(). The ct_clear that
follows the clone then does nothing, because OFPACT_CT_CLEAR runs
only when ctx->conntracked is true. The original packet keeps its
old ct_state, and the ct_clear did not take effect.
Move OFPACT_CT_CLEAR to the non-reversible list so clone(ct_clear,
<actions>) goes through the path that wraps the inner actions in a
datapath clone and properly isolates ctx->conntracked from the
outer context.
Test added in tests/ofproto-dpif.at.
Fixes: 1fe178d251c8 ("dpif: Add support for OVS_ACTION_ATTR_CT_CLEAR")
Reported-by: Ashwin Balaji S <[email protected]>
Signed-off-by: Naveen Yerramneni <[email protected]>
---
v2:
- Addressed review comments from Ilya Maximets.
---
ofproto/ofproto-dpif-xlate.c | 2 +-
tests/ofproto-dpif.at | 24 ++++++++++++++++++++++++
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0b38dcf55..c1ba447e9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6126,7 +6126,6 @@ reversible_actions(const struct ofpact *ofpacts, size_t
ofpacts_len)
case OFPACT_CLONE:
case OFPACT_CONJUNCTION:
case OFPACT_CONTROLLER:
- case OFPACT_CT_CLEAR:
case OFPACT_DEBUG_RECIRC:
case OFPACT_DEBUG_SLOW:
case OFPACT_DEC_MPLS_TTL:
@@ -6176,6 +6175,7 @@ reversible_actions(const struct ofpact *ofpacts, size_t
ofpacts_len)
break;
case OFPACT_CT:
+ case OFPACT_CT_CLEAR:
case OFPACT_METER:
case OFPACT_NAT:
case OFPACT_OUTPUT_TRUNC:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 39e43d376..dcb9e91e0 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -9298,6 +9298,30 @@ Datapath actions:
set(ipv4(dst=192.168.4.4)),2,set(ipv4(dst=10.10.10.1)),4
])
AT_CHECK([grep "Failed to compose clone action" stdout], [0], [ignore])
+dnl ct_clear inside clone() is irreversible. A datapath clone action
+dnl should be generated and any ct_clear after the clone must still be
+dnl emitted.
+
+dnl Restore the datapath support level.
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 clone true], [0], [])
+AT_CHECK([ovs-appctl dpif/set-dp-features br0 sample_nesting 10], [0], [])
+
+dnl Recirculate after ct() so the conntracked flag is set when the
+dnl clone is processed.
+AT_DATA([flows.txt], [dnl
+table=0,in_port=1,ip,actions=ct(table=1)
+table=1,in_port=1,actions=clone(ct_clear,output:2),ct_clear,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],
+ [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
