On Fri, Feb 26, 2021 at 1:15 AM Han Zhou <[email protected]> wrote: > > On Thu, Feb 25, 2021 at 1:25 AM Numan Siddique <[email protected]> wrote: > > > > On Thu, Feb 25, 2021 at 1:12 PM Han Zhou <[email protected]> wrote: > > > > > > On Wed, Feb 24, 2021 at 5:27 AM <[email protected]> wrote: > > > > > > > > From: Numan Siddique <[email protected]> > > > > > > > > Presently we add 65535 priority lflows in the stages - > > > > 'ls_in_acl' and 'ls_out_acl' to drop packets which > > > > match on 'ct.inv'. > > > > > > > > As per the 'ovs-fields' man page, this > > > > ct state field can be used to identify problems such as: > > > > • L3/L4 protocol handler is not loaded/unavailable. > > > > > > > > • L3/L4 protocol handler determines that the packet is > > > > malformed. > > > > > > > > • Packets are unexpected length for protocol. > > > > > > > > This patch removes the usage of this field for the following > > > > reasons: > > > > > > > > • Some of the smart NICs which support offloading datapath > > > > flows don't support this field. > > > > > > What do you mean by "don't support this field"? Do you mean the NIC > > > offloading supports connection tracking, but cannot detect if a packet > is > > > invalid and always populate the ct.inv as 0? > > > > I think so. From what I understand, the kernel conntrack feature is used > > for the actual connection tracking. So NIC can't tell if the packet is > > invalid or not > > (say due to out-of-window tcp errors). > > > I know some NICs support CT offloading and some doesn't. > So here what you are referring are the NICs that doesn't support CT > offloading, which falls back to kernel datapath when CT is used, is it? If > this is the case, then even without ct.inv it still couldn't support > ct.est, etc. right? > Or, do you mean this is specifically for NICs that support CT offloading > but not ct.inv, i.e. it can do regular conntrack in NIC but just can't > identify out-of-window packets, and that's why it supports ct.est but not > ct.inv? > I am still quite confused. Could clarify a little more, which types of NICs > would benefit from this, and how?
I'm not sure if I can explain the issue well. Can you please look into this bugzilla - https://bugzilla.redhat.com/show_bug.cgi?id=1921946 We can discuss further if you have further questions or comments. > > > > > > > > > > > > • A recent commit in kernel ovs datapath sets the committed > > > > connection tracking entry to be liberal for out-of-window > > > > tcp packets (nf_ct_set_tcp_be_liberal()). Such TCP > > > > packets will not be marked as invalid. > > > > > > > > > > Could you share a link to this commit? > > > > Sure. > > > https://kernel.googlesource.com/pub/scm/linux/kernel/git/netdev/net-next/+/e2ef5203c817a60bfb591343ffd851b6537370ff > > > Thanks for sharing. So OVS is not capable of detecting a out-of-window > packet now. Could you explain more about the motivation? I couldn't get the > full picture from commit message of that patch. Do you have a link that > discusses more details? Let me share with you the patch which I first submitted to handle this issue. During the review, @Florian Westphal suggested being liberal for out-of-window packets to solve this issue. I think the patch discussions have enough information. Please let me know if you have further questions or comments and we can discuss further. Initial approach taken by me - https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/ Final approach taken - https://patchwork.ozlabs.org/project/netdev/patch/[email protected]/ > > > > > > > > • Even if a ct.inv packet is delivered to a VIF, the > > > > networking stack of the VIF's kernel can handle such > > > > packets. > > > > > > I have some concern for this point. We shouldn't make assumptions for > > > what's configured in the VIF's kernel, because it is independent of > what's > > > expected from OVN ACLs. In addition, egress rules are expected to drop > > > invalid packets sent by the VIF (regardless of how the VIF's kernel is > > > configured). > > > > Agree. I can delete this point from the commit message. > > > > > > > > However, I am not against this patch, but just want to double confirm. I > > > think this deserves a description in NEWS if we do so. > > > > Sure. I will add to the NEWS. > > > > If there are concerns about removing this, how about we use ct.inv by > > default and > > add a config option to not use this flag in ovn-northd ? > > Deployments who want to make use of HWOL nics can turn on this option ? > > > Yes, I think an option would help. However, before moving there, I am > wondering if the user could simply use stateless ACLs to achieve the same > outcome. What's the benefit of using stateful ACLs without the ability to > detect invalid packets v.s. using stateless ACLs? Given that we enable conntrack for all the packets if a logical switch has "allow-related" ACLs or a load balancers associated, I don't think we can take this path. In the case of ovn-k8s, the node logical switch will have a load balancer associated with it. Thanks Numan > > > Thanks for the review. > > > > Numan > > > > > > > > Thanks, > > > Han > > > > > > > > > > > Signed-off-by: Numan Siddique <[email protected]> > > > > --- > > > > northd/ovn-northd.c | 24 ++++++++++++------------ > > > > tests/ovn-northd.at | 24 ++++++++++++------------ > > > > 2 files changed, 24 insertions(+), 24 deletions(-) > > > > > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > > > index dcdb777a2..e30fb532c 100644 > > > > --- a/northd/ovn-northd.c > > > > +++ b/northd/ovn-northd.c > > > > @@ -5617,10 +5617,10 @@ build_acls(struct ovn_datapath *od, struct > hmap > > > *lflows, > > > > * > > > > * This is enforced at a higher priority than ACLs can be > > > defined. */ > > > > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > > > > - "ct.inv || (ct.est && ct.rpl && > ct_label.blocked > > > == 1)", > > > > + "ct.est && ct.rpl && ct_label.blocked == 1", > > > > "drop;"); > > > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > > > > - "ct.inv || (ct.est && ct.rpl && > ct_label.blocked > > > == 1)", > > > > + "ct.est && ct.rpl && ct_label.blocked == 1", > > > > "drop;"); > > > > > > > > /* Ingress and Egress ACL Table (Priority 65535). > > > > @@ -5633,12 +5633,12 @@ build_acls(struct ovn_datapath *od, struct > hmap > > > *lflows, > > > > * > > > > * This is enforced at a higher priority than ACLs can be > > > defined. */ > > > > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > > > > - "ct.est && !ct.rel && !ct.new && !ct.inv " > > > > - "&& ct.rpl && ct_label.blocked == 0", > > > > + "ct.est && !ct.rel && !ct.new && " > > > > + "ct.rpl && ct_label.blocked == 0", > > > > "next;"); > > > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > > > > - "ct.est && !ct.rel && !ct.new && !ct.inv " > > > > - "&& ct.rpl && ct_label.blocked == 0", > > > > + "ct.est && !ct.rel && !ct.new && " > > > > + "ct.rpl && ct_label.blocked == 0", > > > > "next;"); > > > > > > > > /* Ingress and Egress ACL Table (Priority 65535). > > > > @@ -5653,12 +5653,12 @@ build_acls(struct ovn_datapath *od, struct > hmap > > > *lflows, > > > > * related traffic such as an ICMP Port Unreachable through > > > > * that's generated from a non-listening UDP port. */ > > > > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > > > > - "!ct.est && ct.rel && !ct.new && !ct.inv " > > > > - "&& ct_label.blocked == 0", > > > > + "!ct.est && ct.rel && !ct.new && " > > > > + "ct_label.blocked == 0", > > > > "next;"); > > > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > > > > - "!ct.est && ct.rel && !ct.new && !ct.inv " > > > > - "&& ct_label.blocked == 0", > > > > + "!ct.est && ct.rel && !ct.new && " > > > > + "ct_label.blocked == 0", > > > > "next;"); > > > > > > > > /* Ingress and Egress ACL Table (Priority 65535). > > > > @@ -5846,11 +5846,11 @@ build_lb(struct ovn_datapath *od, struct hmap > > > *lflows) > > > > * > > > > * Send established traffic through conntrack for just NAT. > */ > > > > ovn_lflow_add(lflows, od, S_SWITCH_IN_LB, UINT16_MAX - 1, > > > > - "ct.est && !ct.rel && !ct.new && !ct.inv && " > > > > + "ct.est && !ct.rel && !ct.new && " > > > > "ct_label.natted == 1", > > > > REGBIT_CONNTRACK_NAT" = 1; next;"); > > > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_LB, UINT16_MAX - 1, > > > > - "ct.est && !ct.rel && !ct.new && !ct.inv && " > > > > + "ct.est && !ct.rel && !ct.new && " > > > > "ct_label.natted == 1", > > > > REGBIT_CONNTRACK_NAT" = 1; next;"); > > > > } > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > > index ad0f9f562..dc49c2543 100644 > > > > --- a/tests/ovn-northd.at > > > > +++ b/tests/ovn-northd.at > > > > @@ -1940,9 +1940,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e > > > ls_in_acl_hint -e ls_out_acl_hint -e > > > > table=4 (ls_out_acl_hint ), priority=6 , match=(!ct.new && > > > ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; > > > reg0[[9]] = 1; next;) > > > > table=4 (ls_out_acl_hint ), priority=7 , match=(ct.new && > > > !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) > > > > table=5 (ls_out_acl ), priority=1 , match=(ip && > (!ct.est > > > || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) > > > > - table=5 (ls_out_acl ), priority=65535, match=(!ct.est && > > > ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > > > > - table=5 (ls_out_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), > > > action=(next;) > > > > - table=5 (ls_out_acl ), priority=65535, match=(ct.inv || > > > (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) > > > > + table=5 (ls_out_acl ), priority=65535, match=(!ct.est && > > > ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) > > > > + table=5 (ls_out_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) > > > > + table=5 (ls_out_acl ), priority=65535, match=(ct.est && > ct.rpl > > > && ct_label.blocked == 1), action=(drop;) > > > > table=8 (ls_in_acl_hint ), priority=1 , match=(ct.est && > > > ct_label.blocked == 0), action=(reg0[[10]] = 1; next;) > > > > table=8 (ls_in_acl_hint ), priority=2 , match=(ct.est && > > > ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) > > > > table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), > > > action=(reg0[[9]] = 1; next;) > > > > @@ -1951,9 +1951,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e > > > ls_in_acl_hint -e ls_out_acl_hint -e > > > > table=8 (ls_in_acl_hint ), priority=6 , match=(!ct.new && > > > ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; > > > reg0[[9]] = 1; next;) > > > > table=8 (ls_in_acl_hint ), priority=7 , match=(ct.new && > > > !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) > > > > 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=65535, match=(!ct.est && > > > ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > > > > - table=9 (ls_in_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), > > > action=(next;) > > > > - table=9 (ls_in_acl ), priority=65535, match=(ct.inv || > > > (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) > > > > + table=9 (ls_in_acl ), priority=65535, match=(!ct.est && > > > ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) > > > > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) > > > > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && > ct.rpl > > > && ct_label.blocked == 1), action=(drop;) > > > > ]) > > > > > > > > AS_BOX([Check match ct_state with load balancer]) > > > > @@ -1972,9 +1972,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e > > > ls_in_acl_hint -e ls_out_acl_hint -e > > > > table=4 (ls_out_acl_hint ), priority=6 , match=(!ct.new && > > > ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; > > > reg0[[9]] = 1; next;) > > > > table=4 (ls_out_acl_hint ), priority=7 , match=(ct.new && > > > !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) > > > > table=5 (ls_out_acl ), priority=1 , match=(ip && > (!ct.est > > > || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;) > > > > - table=5 (ls_out_acl ), priority=65535, match=(!ct.est && > > > ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > > > > - table=5 (ls_out_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), > > > action=(next;) > > > > - table=5 (ls_out_acl ), priority=65535, match=(ct.inv || > > > (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) > > > > + table=5 (ls_out_acl ), priority=65535, match=(!ct.est && > > > ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) > > > > + table=5 (ls_out_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) > > > > + table=5 (ls_out_acl ), priority=65535, match=(ct.est && > ct.rpl > > > && ct_label.blocked == 1), action=(drop;) > > > > table=8 (ls_in_acl_hint ), priority=1 , match=(ct.est && > > > ct_label.blocked == 0), action=(reg0[[10]] = 1; next;) > > > > table=8 (ls_in_acl_hint ), priority=2 , match=(ct.est && > > > ct_label.blocked == 1), action=(reg0[[9]] = 1; next;) > > > > table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), > > > action=(reg0[[9]] = 1; next;) > > > > @@ -1983,9 +1983,9 @@ AT_CHECK([ovn-sbctl lflow-list ls | grep -e > > > ls_in_acl_hint -e ls_out_acl_hint -e > > > > table=8 (ls_in_acl_hint ), priority=6 , match=(!ct.new && > > > ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; > > > reg0[[9]] = 1; next;) > > > > table=8 (ls_in_acl_hint ), priority=7 , match=(ct.new && > > > !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;) > > > > 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=65535, match=(!ct.est && > > > ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;) > > > > - table=9 (ls_in_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), > > > action=(next;) > > > > - table=9 (ls_in_acl ), priority=65535, match=(ct.inv || > > > (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;) > > > > + table=9 (ls_in_acl ), priority=65535, match=(!ct.est && > > > ct.rel && !ct.new && ct_label.blocked == 0), action=(next;) > > > > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && > > > !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;) > > > > + table=9 (ls_in_acl ), priority=65535, match=(ct.est && > ct.rpl > > > && ct_label.blocked == 1), action=(drop;) > > > > ]) > > > > > > > > AT_CLEANUP > > > > -- > > > > 2.29.2 > > > > > > > > _______________________________________________ > > > > dev mailing list > > > > [email protected] > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > > > dev mailing list > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
