[ovs-dev] [PATCH ovn] northd: Remove the usage of 'ct.inv' in logical flows.
Numan Siddique
numans at ovn.org
Thu Feb 25 09:24:53 UTC 2021
On Thu, Feb 25, 2021 at 1:12 PM Han Zhou <hzhou at ovn.org> wrote:
>
> On Wed, Feb 24, 2021 at 5:27 AM <numans at ovn.org> wrote:
> >
> > From: Numan Siddique <numans at ovn.org>
> >
> > 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 <numans at ovn.org>
> > ---
> > 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
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list