[ovs-dev] [PATCH ovn] northd: Remove the usage of 'ct.inv' in logical flows.
Han Zhou
hzhou at ovn.org
Thu Feb 25 07:41:44 UTC 2021
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?
>
> • 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?
> • 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).
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.
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
More information about the dev
mailing list