| Message ID | 20220310194243.2624700-1-numans@ovn.org |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev,v2] ovn-northd: Don't commit to conntrack if the conntrack was skipped. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Thu, Mar 10, 2022 at 2:42 PM <numans@ovn.org> wrote: > From: Numan Siddique <numans@ovn.org> > > 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 <mwasher@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > 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 > >
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;)