| Message ID | 1599814331-20105-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-northd: Skip conntrack for MLD packets. | expand |
On Fri, Sep 11, 2020 at 2:25 PM Dumitru Ceara <dceara@redhat.com> wrote: > > We currently skip conntrack for IPv6 Neighbor Discovery packets because > conntrack marks all ND packets as invalid [0]. > > The same thing should be done for MLD packets. Otherwise, as soon as an > allow-related ACL or load balancer is added, MLD packets will go to > conntrack and get dropped because they are marked "invalid". > > This commit also fixes the MLD test to use a link local IPv6 source > address. > > [0] https://bugzilla.kernel.org/show_bug.cgi?id=11797 > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks Dumitru. I applied this patch to master. Does it need a backport to 20.06 ? Numan > --- > northd/ovn-northd.8.xml | 16 +++++++++------- > northd/ovn-northd.c | 12 ++++++------ > tests/ovn.at | 23 +++++++++++++++++++---- > 3 files changed, 34 insertions(+), 17 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index a275442..e09b28a 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -318,7 +318,8 @@ > <code>Pre-stateful</code> to send IP packets to the connection tracker > before eventually advancing to ingress table <code>ACLs</code>. If > special ports such as route ports or localnet ports can't use ct(), a > - priority-110 flow is added to skip over stateful ACLs. > + priority-110 flow is added to skip over stateful ACLs. IPv6 Neighbor > + Discovery and MLD traffic also skips stateful ACLs. > </p> > > <p> > @@ -337,11 +338,12 @@ > This table prepares flows for possible stateful load balancing processing > in ingress table <code>LB</code> and <code>Stateful</code>. It contains > a priority-0 flow that simply moves traffic to the next table. Moreover > - it contains a priority-110 flow to move IPv6 Neighbor Discovery traffic > - to the next table. If load balancing rules with virtual IP addresses > - (and ports) are configured in <code>OVN_Northbound</code> database for a > - logical switch datapath, a priority-100 flow is added for each configured > - virtual IP address <var>VIP</var>. For IPv4 <var>VIPs</var>, the match is > + it contains a priority-110 flow to move IPv6 Neighbor Discovery and MLD > + traffic to the next table. If load balancing rules with virtual IP > + addresses (and ports) are configured in <code>OVN_Northbound</code> > + database for a logical switch datapath, a priority-100 flow is added for > + each configured virtual IP address <var>VIP</var>. For IPv4 > + <var>VIPs</var>, the match is > <code>ip && ip4.dst == <var>VIP</var></code>. For IPv6 > <var>VIPs</var>, the match is <code>ip && > ip6.dst == <var>VIP</var></code>. The flow sets an action > @@ -478,7 +480,7 @@ > > <li> > A priority-65535 flow that allows IPv6 Neighbor solicitation, > - Neighbor discover, Router solicitation and Router advertisement > + Neighbor discover, Router solicitation, Router advertisement and MLD > packets. > </li> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index b95d6cd..48d17a4 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -4924,10 +4924,10 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) > * Not to do conntrack on ND and ICMP destination > * unreachable packets. */ > ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, > - "nd || nd_rs || nd_ra || " > + "nd || nd_rs || nd_ra || mldv1 || mldv2 || " > "(udp && udp.src == 546 && udp.dst == 547)", "next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, > - "nd || nd_rs || nd_ra || " > + "nd || nd_rs || nd_ra || mldv1 || mldv2 || " > "(udp && udp.src == 546 && udp.dst == 547)", "next;"); > > /* Ingress and Egress Pre-ACL Table (Priority 100). > @@ -5040,10 +5040,10 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows, > { > /* Do not send ND packets to conntrack */ > ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110, > - "nd || nd_rs || nd_ra", > + "nd || nd_rs || nd_ra || mldv1 || mldv2", > "next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 110, > - "nd || nd_rs || nd_ra", > + "nd || nd_rs || nd_ra || mldv1 || mldv2", > "next;"); > > /* Do not send service monitor packets to conntrack. */ > @@ -5575,9 +5575,9 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, > * > * Not to do conntrack on ND packets. */ > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > - "nd || nd_ra || nd_rs", "next;"); > + "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > - "nd || nd_ra || nd_rs", "next;"); > + "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;"); > } > > /* Ingress or Egress ACL Table (Various priorities). */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 4e58722..1898728 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -17339,7 +17339,7 @@ store_mld_query() { > local mld_type=82 > local mld_code=00 > local max_resp=03e8 > - local mld_chksum=59be > + local mld_chksum=7b3d > local addr=00000000000000000000000000000000 > > local eth=${eth_dst}${eth_src}86dd > @@ -17419,6 +17419,21 @@ ovn-nbctl lsp-add sw3 sw3-rtr \ > -- lsp-set-addresses sw3-rtr 00:00:00:00:03:00 \ > -- lsp-set-options sw3-rtr router-port=rtr-sw3 > > +# Conntrack marks all IPv6 Neighbor Discovery and MLD packets as invalid, > +# make sure to test that conntrack is bypassed for MLD by adding an empty > +# allow-related ACL and an empty load balancer. > +ovn-nbctl acl-add sw1 from-lport 1 "1" allow-related > +ovn-nbctl acl-add sw2 from-lport 1 "1" allow-related > +ovn-nbctl acl-add sw3 from-lport 1 "1" allow-related > +ovn-nbctl acl-add sw1 to-lport 1 "1" allow-related > +ovn-nbctl acl-add sw2 to-lport 1 "1" allow-related > +ovn-nbctl acl-add sw3 to-lport 1 "1" allow-related > + > +ovn-nbctl lb-add lb0 [[4242::1]]:80 "" > +ovn-nbctl ls-lb-add sw1 lb0 > +ovn-nbctl ls-lb-add sw2 lb0 > +ovn-nbctl ls-lb-add sw3 lb0 > + > net_add n1 > sim_add hv1 > as hv1 > @@ -17614,12 +17629,12 @@ ovn-nbctl set Logical_Switch sw2 \ > other_config:mcast_querier="true" \ > other_config:mcast_query_interval=1 \ > other_config:mcast_eth_src="00:00:00:00:02:fe" \ > - other_config:mcast_ip6_src="2000::fe" > + other_config:mcast_ip6_src="fe80::fe" > > # Wait for 1 query interval (1 sec) and check that two queries are generated. > > expected > -store_mld_query 0000000002fe 200000000000000000000000000000fe expected > -store_mld_query 0000000002fe 200000000000000000000000000000fe expected > +store_mld_query 0000000002fe fe8000000000000000000000000000fe expected > +store_mld_query 0000000002fe fe8000000000000000000000000000fe expected > sleep 1 > > OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected]) > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 9/11/20 11:46 AM, Numan Siddique wrote: > On Fri, Sep 11, 2020 at 2:25 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> We currently skip conntrack for IPv6 Neighbor Discovery packets because >> conntrack marks all ND packets as invalid [0]. >> >> The same thing should be done for MLD packets. Otherwise, as soon as an >> allow-related ACL or load balancer is added, MLD packets will go to >> conntrack and get dropped because they are marked "invalid". >> >> This commit also fixes the MLD test to use a link local IPv6 source >> address. >> >> [0] https://bugzilla.kernel.org/show_bug.cgi?id=11797 >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Thanks Dumitru. I applied this patch to master. Does it need a > backport to 20.06 ? > Thanks Numan! Yes, this could go down to 20.03 if it's not too much trouble. Regards, Dumitru
On Fri, Sep 11, 2020 at 3:24 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 9/11/20 11:46 AM, Numan Siddique wrote: > > On Fri, Sep 11, 2020 at 2:25 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> > >> We currently skip conntrack for IPv6 Neighbor Discovery packets because > >> conntrack marks all ND packets as invalid [0]. > >> > >> The same thing should be done for MLD packets. Otherwise, as soon as an > >> allow-related ACL or load balancer is added, MLD packets will go to > >> conntrack and get dropped because they are marked "invalid". > >> > >> This commit also fixes the MLD test to use a link local IPv6 source > >> address. > >> > >> [0] https://bugzilla.kernel.org/show_bug.cgi?id=11797 > >> > >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > > > Thanks Dumitru. I applied this patch to master. Does it need a > > backport to 20.06 ? > > > > Thanks Numan! Yes, this could go down to 20.03 if it's not too much trouble. I applied to branch-20.06. It doesn't apply cleanly to branch-20.03. Can you please post a branch-20.03. Thanks Numan > > Regards, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 9/11/20 12:26 PM, Numan Siddique wrote: > On Fri, Sep 11, 2020 at 3:24 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 9/11/20 11:46 AM, Numan Siddique wrote: >>> On Fri, Sep 11, 2020 at 2:25 PM Dumitru Ceara <dceara@redhat.com> wrote: >>>> >>>> We currently skip conntrack for IPv6 Neighbor Discovery packets because >>>> conntrack marks all ND packets as invalid [0]. >>>> >>>> The same thing should be done for MLD packets. Otherwise, as soon as an >>>> allow-related ACL or load balancer is added, MLD packets will go to >>>> conntrack and get dropped because they are marked "invalid". >>>> >>>> This commit also fixes the MLD test to use a link local IPv6 source >>>> address. >>>> >>>> [0] https://bugzilla.kernel.org/show_bug.cgi?id=11797 >>>> >>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> >>> Thanks Dumitru. I applied this patch to master. Does it need a >>> backport to 20.06 ? >>> >> >> Thanks Numan! Yes, this could go down to 20.03 if it's not too much trouble. > > I applied to branch-20.06. It doesn't apply cleanly to branch-20.03. > Can you please post a branch-20.03. > Done: http://patchwork.ozlabs.org/project/ovn/patch/1599820722-31315-1-git-send-email-dceara@redhat.com/ Thanks!
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index a275442..e09b28a 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -318,7 +318,8 @@ <code>Pre-stateful</code> to send IP packets to the connection tracker before eventually advancing to ingress table <code>ACLs</code>. If special ports such as route ports or localnet ports can't use ct(), a - priority-110 flow is added to skip over stateful ACLs. + priority-110 flow is added to skip over stateful ACLs. IPv6 Neighbor + Discovery and MLD traffic also skips stateful ACLs. </p> <p> @@ -337,11 +338,12 @@ This table prepares flows for possible stateful load balancing processing in ingress table <code>LB</code> and <code>Stateful</code>. It contains a priority-0 flow that simply moves traffic to the next table. Moreover - it contains a priority-110 flow to move IPv6 Neighbor Discovery traffic - to the next table. If load balancing rules with virtual IP addresses - (and ports) are configured in <code>OVN_Northbound</code> database for a - logical switch datapath, a priority-100 flow is added for each configured - virtual IP address <var>VIP</var>. For IPv4 <var>VIPs</var>, the match is + it contains a priority-110 flow to move IPv6 Neighbor Discovery and MLD + traffic to the next table. If load balancing rules with virtual IP + addresses (and ports) are configured in <code>OVN_Northbound</code> + database for a logical switch datapath, a priority-100 flow is added for + each configured virtual IP address <var>VIP</var>. For IPv4 + <var>VIPs</var>, the match is <code>ip && ip4.dst == <var>VIP</var></code>. For IPv6 <var>VIPs</var>, the match is <code>ip && ip6.dst == <var>VIP</var></code>. The flow sets an action @@ -478,7 +480,7 @@ <li> A priority-65535 flow that allows IPv6 Neighbor solicitation, - Neighbor discover, Router solicitation and Router advertisement + Neighbor discover, Router solicitation, Router advertisement and MLD packets. </li> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index b95d6cd..48d17a4 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -4924,10 +4924,10 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) * Not to do conntrack on ND and ICMP destination * unreachable packets. */ ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, - "nd || nd_rs || nd_ra || " + "nd || nd_rs || nd_ra || mldv1 || mldv2 || " "(udp && udp.src == 546 && udp.dst == 547)", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, - "nd || nd_rs || nd_ra || " + "nd || nd_rs || nd_ra || mldv1 || mldv2 || " "(udp && udp.src == 546 && udp.dst == 547)", "next;"); /* Ingress and Egress Pre-ACL Table (Priority 100). @@ -5040,10 +5040,10 @@ build_pre_lb(struct ovn_datapath *od, struct hmap *lflows, { /* Do not send ND packets to conntrack */ ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_LB, 110, - "nd || nd_rs || nd_ra", + "nd || nd_rs || nd_ra || mldv1 || mldv2", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_LB, 110, - "nd || nd_rs || nd_ra", + "nd || nd_rs || nd_ra || mldv1 || mldv2", "next;"); /* Do not send service monitor packets to conntrack. */ @@ -5575,9 +5575,9 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, * * Not to do conntrack on ND packets. */ ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, - "nd || nd_ra || nd_rs", "next;"); + "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, - "nd || nd_ra || nd_rs", "next;"); + "nd || nd_ra || nd_rs || mldv1 || mldv2", "next;"); } /* Ingress or Egress ACL Table (Various priorities). */ diff --git a/tests/ovn.at b/tests/ovn.at index 4e58722..1898728 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -17339,7 +17339,7 @@ store_mld_query() { local mld_type=82 local mld_code=00 local max_resp=03e8 - local mld_chksum=59be + local mld_chksum=7b3d local addr=00000000000000000000000000000000 local eth=${eth_dst}${eth_src}86dd @@ -17419,6 +17419,21 @@ ovn-nbctl lsp-add sw3 sw3-rtr \ -- lsp-set-addresses sw3-rtr 00:00:00:00:03:00 \ -- lsp-set-options sw3-rtr router-port=rtr-sw3 +# Conntrack marks all IPv6 Neighbor Discovery and MLD packets as invalid, +# make sure to test that conntrack is bypassed for MLD by adding an empty +# allow-related ACL and an empty load balancer. +ovn-nbctl acl-add sw1 from-lport 1 "1" allow-related +ovn-nbctl acl-add sw2 from-lport 1 "1" allow-related +ovn-nbctl acl-add sw3 from-lport 1 "1" allow-related +ovn-nbctl acl-add sw1 to-lport 1 "1" allow-related +ovn-nbctl acl-add sw2 to-lport 1 "1" allow-related +ovn-nbctl acl-add sw3 to-lport 1 "1" allow-related + +ovn-nbctl lb-add lb0 [[4242::1]]:80 "" +ovn-nbctl ls-lb-add sw1 lb0 +ovn-nbctl ls-lb-add sw2 lb0 +ovn-nbctl ls-lb-add sw3 lb0 + net_add n1 sim_add hv1 as hv1 @@ -17614,12 +17629,12 @@ ovn-nbctl set Logical_Switch sw2 \ other_config:mcast_querier="true" \ other_config:mcast_query_interval=1 \ other_config:mcast_eth_src="00:00:00:00:02:fe" \ - other_config:mcast_ip6_src="2000::fe" + other_config:mcast_ip6_src="fe80::fe" # Wait for 1 query interval (1 sec) and check that two queries are generated. > expected -store_mld_query 0000000002fe 200000000000000000000000000000fe expected -store_mld_query 0000000002fe 200000000000000000000000000000fe expected +store_mld_query 0000000002fe fe8000000000000000000000000000fe expected +store_mld_query 0000000002fe fe8000000000000000000000000000fe expected sleep 1 OVN_CHECK_PACKETS([hv1/vif3-tx.pcap], [expected])
We currently skip conntrack for IPv6 Neighbor Discovery packets because conntrack marks all ND packets as invalid [0]. The same thing should be done for MLD packets. Otherwise, as soon as an allow-related ACL or load balancer is added, MLD packets will go to conntrack and get dropped because they are marked "invalid". This commit also fixes the MLD test to use a link local IPv6 source address. [0] https://bugzilla.kernel.org/show_bug.cgi?id=11797 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.8.xml | 16 +++++++++------- northd/ovn-northd.c | 12 ++++++------ tests/ovn.at | 23 +++++++++++++++++++---- 3 files changed, 34 insertions(+), 17 deletions(-)