On Thu, Mar 10, 2022 at 2:42 PM <[email protected]> wrote:
> From: Numan Siddique <[email protected]>
>
> Presently, if the inport or outport is a peer port (of router port),
> then we skip sending the packet to conntrack by not setting the
> reg0[0]/reg0[1] bits. But the packet still goes through the
> stages - ls_in_acl_hint, ls_in_acl and ls_in_stateful in the ingress
> pipeline and similarly ls_out_acl_hint, ls_out_acl and ls_out_stateful
> in the egress pipeline of the logical switch.
>
> In these mentioned stages, the logical flows match on the ct states
> (ct.new, ct.est etc) and this can be problematic if the packet was
> sent to conntrack and committed in the ingress pipeline. When
> that packet enters the egress pipeline with outport set to the peer
> port, we skip sending the packet to conntrack (which is expected)
> but ct state values carry over from the ingress state. If the ct.new
> flag was set in the ingress pipeline, then the below flows are hit and
> the packet gets committed in the conntrack zone of the inport logical port.
>
> table=4 (ls_out_acl ), priority=1 ,
> match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))),
> action=(reg0[1] = 1; next;)
> table=7 (ls_out_stateful ), priority=100 ,
> match=(reg0[1] == 1 && reg0[13] == 0),
> action=(ct_commit { ct_label.blocked = 0; }; next;)
>
> With this extra commit to the same conntrack zone, sometimes the packet
> gets
> dropped or doesn't reach the destination. It is not very clear how
> the packet gets misplaced, but avoiding this resolves the issue.
> And OVN ideally should not do this extra commit.
>
> This patch addresses this issue by adding the below logical flow
> both in ls_in_acl and ls_out_acl stage
>
> priority=2 , match=(reg0[0] == 0 && reg0[1] == 0), action=(next;)
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2060688
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2062431
> Reported-by: Michael Washer <[email protected]>
> Signed-off-by: Numan Siddique <[email protected]>
>
ovn-k8s tests are failing with this patch in the github CI.
So please ignore this patch. I'll submit v3 addressing the test failures.
Numan
> ---
>
> v1 -> v2
> -------
> * Changed the approach to solve the issue.
>
> northd/northd.c | 17 +++++++++++++++--
> northd/ovn-northd.8.xml | 6 ++++++
> tests/ovn-northd.at | 5 +++++
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b264fb850b..23ab5ac260 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -6529,7 +6529,7 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows,
> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB, 0, "1", "next;");
>
> if (has_stateful) {
> - /* Ingress and Egress ACL Table (Priority 1).
> + /* Ingress and Egress ACL Table (Priority 1 and 2).
> *
> * By default, traffic is allowed. This is partially handled by
> * the Priority 0 ACL flows added earlier, but we also need to
> @@ -6549,7 +6549,20 @@ build_acls(struct ovn_datapath *od, struct hmap
> *lflows,
> * We need to set ct_label.blocked=0 to let the connection
> continue,
> * which will be done by ct_commit() in the "stateful" stage.
> * Subsequent packets will hit the flow at priority 0 that just
> - * uses "next;". */
> + * uses "next;".
> + *
> + * However, we don't want to commit if the packet was not sent to
> + * conntrack in the first place.
> + * Eg. if inport or outport is a router peer port. */
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 2,
> + REGBIT_CONNTRACK_DEFRAG" == 0 && "
> + REGBIT_CONNTRACK_NAT" == 0",
> + "next;");
> + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 2,
> + REGBIT_CONNTRACK_DEFRAG" == 0 && "
> + REGBIT_CONNTRACK_NAT" == 0",
> + "next;");
> +
> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1,
> "ip && (!ct.est || (ct.est && ct_label.blocked ==
> 1))",
> REGBIT_CONNTRACK_COMMIT" = 1; next;");
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 4784bff041..d788efe70b 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -784,6 +784,12 @@
> </p>
>
> <ul>
> + <li>
> + A priority-2 flow that advances the packet to the next stage if
> the
> + packet was not sent to conntrack earlier, by matching on
> + <code>reg0[0] == 0 && reg0[1] == 1</code>.
> + </li>
> +
> <li>
> A priority-1 flow that sets the hint to commit IP traffic to the
> connection tracker (with action <code>reg0[1] = 1;
> next;</code>). This
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 60d91a7717..bfd0f2382b 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2265,6 +2265,7 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e
> ls_in_acl_hint -e ls_out_acl_hint -e
> table=4 (ls_out_acl ), priority=1 , match=(ip && (!ct.est ||
> (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
> table=4 (ls_out_acl ), priority=1001 , match=(reg0[[7]] == 1 &&
> (ip)), action=(reg0[[1]] = 1; next;)
> table=4 (ls_out_acl ), priority=1001 , match=(reg0[[8]] == 1 &&
> (ip)), action=(next;)
> + table=4 (ls_out_acl ), priority=2 , match=(reg0[[0]] == 0 &&
> reg0[[2]] == 0), action=(next;)
> table=4 (ls_out_acl ), priority=34000, match=(eth.src ==
> $svc_monitor_mac), action=(next;)
> table=4 (ls_out_acl ), priority=65532, match=(!ct.est && ct.rel
> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> table=4 (ls_out_acl ), priority=65532, match=(ct.est && !ct.rel
> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
> @@ -2282,6 +2283,7 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e
> ls_in_acl_hint -e ls_out_acl_hint -e
> table=9 (ls_in_acl ), priority=1 , match=(ip && (!ct.est ||
> (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
> table=9 (ls_in_acl ), priority=1001 , match=(reg0[[7]] == 1 &&
> (ip)), action=(reg0[[1]] = 1; next;)
> table=9 (ls_in_acl ), priority=1001 , match=(reg0[[8]] == 1 &&
> (ip)), action=(next;)
> + table=9 (ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 &&
> reg0[[2]] == 0), action=(next;)
> table=9 (ls_in_acl ), priority=34000, match=(eth.dst ==
> $svc_monitor_mac), action=(next;)
> table=9 (ls_in_acl ), priority=65532, match=(!ct.est && ct.rel
> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> table=9 (ls_in_acl ), priority=65532, match=(ct.est && !ct.rel
> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
> action=(reg0[[9]] = 0; reg0[[10]] = 0; next;)
> @@ -6328,6 +6330,7 @@ AT_CAPTURE_FILE([lsflows])
> AT_CHECK([grep -e "ls_in_acl" lsflows | sed 's/table=../table=??/' |
> sort], [0], [dnl
> table=??(ls_in_acl ), priority=0 , match=(1), action=(next;)
> table=??(ls_in_acl ), priority=1 , match=(ip && (!ct.est ||
> (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
> + table=??(ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 &&
> reg0[[2]] == 0), action=(next;)
> table=??(ls_in_acl ), priority=2001 , match=(reg0[[10]] == 1
> && (ip4)), action=(ct_commit { ct_label.blocked = 1; }; /* drop */)
> table=??(ls_in_acl ), priority=2001 , match=(reg0[[9]] == 1 &&
> (ip4)), action=(/* drop */)
> table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 &&
> (ip4 && tcp)), action=(reg0[[1]] = 1; next;)
> @@ -6380,6 +6383,7 @@ AT_CAPTURE_FILE([lsflows])
> AT_CHECK([grep -e "ls_in_acl" lsflows | sed 's/table=../table=??/' |
> sort], [0], [dnl
> table=??(ls_in_acl ), priority=0 , match=(1), action=(next;)
> table=??(ls_in_acl ), priority=1 , match=(ip && (!ct.est ||
> (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
> + table=??(ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 &&
> reg0[[2]] == 0), action=(next;)
> table=??(ls_in_acl ), priority=34000, match=(eth.dst ==
> $svc_monitor_mac), action=(next;)
> table=??(ls_in_acl ), priority=65532, match=(!ct.est && ct.rel
> && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> table=??(ls_in_acl ), priority=65532, match=(ct.est && !ct.rel
> && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
> action=(reg0[[9]] = 0; reg0[[10]] = 0; next;)
> @@ -6432,6 +6436,7 @@ AT_CAPTURE_FILE([lsflows])
> AT_CHECK([grep -e "ls_in_acl" lsflows | sed 's/table=../table=??/' |
> sort], [0], [dnl
> table=??(ls_in_acl ), priority=0 , match=(1), action=(next;)
> table=??(ls_in_acl ), priority=1 , match=(ip && (!ct.est ||
> (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
> + table=??(ls_in_acl ), priority=2 , match=(reg0[[0]] == 0 &&
> reg0[[2]] == 0), action=(next;)
> table=??(ls_in_acl ), priority=2002 , match=(reg0[[7]] == 1 &&
> (ip4 && tcp)), action=(reg0[[1]] = 1; next;)
> table=??(ls_in_acl ), priority=2002 , match=(reg0[[8]] == 1 &&
> (ip4 && tcp)), action=(next;)
> table=??(ls_in_acl ), priority=2003 , match=(reg0[[7]] == 1 &&
> (ip4 && icmp)), action=(reg0[[1]] = 1; next;)
> --
> 2.34.1
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
