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). > > > > > • 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 > > > • 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 ? 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
