Message ID | 20200611131441.1600359-1-numans@ovn.org |
---|---|
State | Superseded |
Headers | show |
Series | [ovs-dev,ovn] northd: By pass IPv6 Router Adv and Router Solicitation packets from ACL stages. | expand |
Hi Numan, I was a bit surprised to find that "nd" did not cover "nd_rs" and "nd_ra" packets already. Is there a reason not to expand the scope of "nd" to cover ICMP6 type 133 and 134? On 6/11/20 9:14 AM, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > We already add below logical flows to by pass IPv6 Neighbor discovery packets > from in/out ACL stage. > > table=6 (ls_in_acl ), priority=65535, match=(nd), action=(next;) > table=4 (ls_out_acl ), priority=65535, match=(nd), action=(next;) > > This patch also adds nd_rs and nd_ra to these logical flows. Without these > the IPv6 Router Adv packets generated by ovn-controller are dropped if > CMS has configured ACLs. > > Reported-by: Jakub Libosvar <jlibosva@redhat.com> > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > northd/ovn-northd.8.xml | 6 ++++++ > northd/ovn-northd.c | 6 ++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 7281eeecc..a7639f33a 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -467,6 +467,12 @@ > ACL re-allow this connection. > </li> > > + <li> > + A priority-65535 flow that allows IPv6 Neighbor solicitation, > + Neighbor discover, Router solicitation and Router advertisement > + packets. > + </li> > + > <li> > A priority 34000 logical flow is added for each logical switch datapath > with the match <code>eth.dst = <var>E</var></code> to allow the service > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 0fc62bf91..b8c9e9325 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -5389,8 +5389,10 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, > /* Ingress and Egress ACL Table (Priority 65535). > * > * Not to do conntrack on ND packets. */ > - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "nd", "next;"); > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "nd", "next;"); > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > + "nd || nd_ra || nd_rs", "next;"); > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > + "nd || nd_ra || nd_rs", "next;"); > } > > /* Ingress or Egress ACL Table (Various priorities). */ >
On Thu, Jun 11, 2020 at 8:13 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Numan, > > I was a bit surprised to find that "nd" did not cover "nd_rs" and > "nd_ra" packets already. Is there a reason not to expand the scope of > "nd" to cover ICMP6 type 133 and 134? > The document in ovn-sb.xml says: · nd expands to icmp6.type == {135, 136} && icmp6.code == 0 && ip.ttl == 255 So I think it was intended to be used for IPv6 Neighbor discovery packets and not for Router Adv/Router solicitation packets. Thanks Numan > On 6/11/20 9:14 AM, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > We already add below logical flows to by pass IPv6 Neighbor discovery > packets > > from in/out ACL stage. > > > > table=6 (ls_in_acl ), priority=65535, match=(nd), action=(next;) > > table=4 (ls_out_acl ), priority=65535, match=(nd), action=(next;) > > > > This patch also adds nd_rs and nd_ra to these logical flows. Without > these > > the IPv6 Router Adv packets generated by ovn-controller are dropped if > > CMS has configured ACLs. > > > > Reported-by: Jakub Libosvar <jlibosva@redhat.com> > > Signed-off-by: Numan Siddique <numans@ovn.org> > > --- > > northd/ovn-northd.8.xml | 6 ++++++ > > northd/ovn-northd.c | 6 ++++-- > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 7281eeecc..a7639f33a 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -467,6 +467,12 @@ > > ACL re-allow this connection. > > </li> > > > > + <li> > > + A priority-65535 flow that allows IPv6 Neighbor solicitation, > > + Neighbor discover, Router solicitation and Router advertisement > > + packets. > > + </li> > > + > > <li> > > A priority 34000 logical flow is added for each logical switch > datapath > > with the match <code>eth.dst = <var>E</var></code> to allow > the service > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 0fc62bf91..b8c9e9325 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -5389,8 +5389,10 @@ build_acls(struct ovn_datapath *od, struct hmap > *lflows, > > /* Ingress and Egress ACL Table (Priority 65535). > > * > > * Not to do conntrack on ND packets. */ > > - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "nd", > "next;"); > > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "nd", > "next;"); > > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > > + "nd || nd_ra || nd_rs", "next;"); > > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > > + "nd || nd_ra || nd_rs", "next;"); > > } > > > > /* Ingress or Egress ACL Table (Various priorities). */ > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Thu, Jun 11, 2020 at 8:59 PM Numan Siddique <numans@ovn.org> wrote: > > > On Thu, Jun 11, 2020 at 8:13 PM Mark Michelson <mmichels@redhat.com> > wrote: > >> Hi Numan, >> >> I was a bit surprised to find that "nd" did not cover "nd_rs" and >> "nd_ra" packets already. Is there a reason not to expand the scope of >> "nd" to cover ICMP6 type 133 and 134? >> > > The document in ovn-sb.xml says: > > · nd expands to icmp6.type == {135, 136} && icmp6.code == 0 && > ip.ttl == 255 > > So I think it was intended to be used for IPv6 Neighbor discovery packets > and not > for Router Adv/Router solicitation packets. > After your email, I checked if we can expand "nd" to cover RA/RS too. But since there is this match - ip.ttl == 255, I think it's better not to. IPv6 RA/RS packets most definitely enter the router pipeline and ttl of the packet could be decremented. Thanks Numan > > Thanks > Numan > > >> On 6/11/20 9:14 AM, numans@ovn.org wrote: >> > From: Numan Siddique <numans@ovn.org> >> > >> > We already add below logical flows to by pass IPv6 Neighbor discovery >> packets >> > from in/out ACL stage. >> > >> > table=6 (ls_in_acl ), priority=65535, match=(nd), >> action=(next;) >> > table=4 (ls_out_acl ), priority=65535, match=(nd), >> action=(next;) >> > >> > This patch also adds nd_rs and nd_ra to these logical flows. Without >> these >> > the IPv6 Router Adv packets generated by ovn-controller are dropped if >> > CMS has configured ACLs. >> > >> > Reported-by: Jakub Libosvar <jlibosva@redhat.com> >> > Signed-off-by: Numan Siddique <numans@ovn.org> >> > --- >> > northd/ovn-northd.8.xml | 6 ++++++ >> > northd/ovn-northd.c | 6 ++++-- >> > 2 files changed, 10 insertions(+), 2 deletions(-) >> > >> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml >> > index 7281eeecc..a7639f33a 100644 >> > --- a/northd/ovn-northd.8.xml >> > +++ b/northd/ovn-northd.8.xml >> > @@ -467,6 +467,12 @@ >> > ACL re-allow this connection. >> > </li> >> > >> > + <li> >> > + A priority-65535 flow that allows IPv6 Neighbor solicitation, >> > + Neighbor discover, Router solicitation and Router advertisement >> > + packets. >> > + </li> >> > + >> > <li> >> > A priority 34000 logical flow is added for each logical >> switch datapath >> > with the match <code>eth.dst = <var>E</var></code> to allow >> the service >> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c >> > index 0fc62bf91..b8c9e9325 100644 >> > --- a/northd/ovn-northd.c >> > +++ b/northd/ovn-northd.c >> > @@ -5389,8 +5389,10 @@ build_acls(struct ovn_datapath *od, struct hmap >> *lflows, >> > /* Ingress and Egress ACL Table (Priority 65535). >> > * >> > * Not to do conntrack on ND packets. */ >> > - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "nd", >> "next;"); >> > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "nd", >> "next;"); >> > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, >> > + "nd || nd_ra || nd_rs", "next;"); >> > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, >> > + "nd || nd_ra || nd_rs", "next;"); >> > } >> > >> > /* Ingress or Egress ACL Table (Various priorities). */ >> > >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >>
On 6/11/20 12:12 PM, Numan Siddique wrote: > > > On Thu, Jun 11, 2020 at 8:59 PM Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org>> wrote: > > > > On Thu, Jun 11, 2020 at 8:13 PM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> wrote: > > Hi Numan, > > I was a bit surprised to find that "nd" did not cover "nd_rs" and > "nd_ra" packets already. Is there a reason not to expand the > scope of > "nd" to cover ICMP6 type 133 and 134? > > > The document in ovn-sb.xml says: > > · nd expands to icmp6.type == {135, 136} && icmp6.code == 0 > && ip.ttl == 255 > > So I think it was intended to be used for IPv6 Neighbor discovery > packets and not > for Router Adv/Router solicitation packets. > > > After your email, I checked if we can expand "nd" to cover RA/RS too. > But since there is this > match - ip.ttl == 255, I think it's better not to. IPv6 RA/RS packets > most definitely enter the router pipeline and ttl > of the packet could be decremented. > > Thanks > Numan Thanks for the answer, Numan. In that case, Acked-by: Mark Michelson <mmichels@redhat.com> > > > Thanks > Numan > > > On 6/11/20 9:14 AM, numans@ovn.org <mailto:numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > We already add below logical flows to by pass IPv6 Neighbor > discovery packets > > from in/out ACL stage. > > > > table=6 (ls_in_acl ), priority=65535, match=(nd), > action=(next;) > > table=4 (ls_out_acl ), priority=65535, match=(nd), > action=(next;) > > > > This patch also adds nd_rs and nd_ra to these logical flows. > Without these > > the IPv6 Router Adv packets generated by ovn-controller are > dropped if > > CMS has configured ACLs. > > > > Reported-by: Jakub Libosvar <jlibosva@redhat.com > <mailto:jlibosva@redhat.com>> > > Signed-off-by: Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org>> > > --- > > northd/ovn-northd.8.xml | 6 ++++++ > > northd/ovn-northd.c | 6 ++++-- > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index 7281eeecc..a7639f33a 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -467,6 +467,12 @@ > > ACL re-allow this connection. > > </li> > > > > + <li> > > + A priority-65535 flow that allows IPv6 Neighbor > solicitation, > > + Neighbor discover, Router solicitation and Router > advertisement > > + packets. > > + </li> > > + > > <li> > > A priority 34000 logical flow is added for each > logical switch datapath > > with the match <code>eth.dst = <var>E</var></code> > to allow the service > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 0fc62bf91..b8c9e9325 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -5389,8 +5389,10 @@ build_acls(struct ovn_datapath *od, > struct hmap *lflows, > > /* Ingress and Egress ACL Table (Priority 65535). > > * > > * Not to do conntrack on ND packets. */ > > - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, > UINT16_MAX, "nd", "next;"); > > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, > UINT16_MAX, "nd", "next;"); > > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, > > + "nd || nd_ra || nd_rs", "next;"); > > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > > + "nd || nd_ra || nd_rs", "next;"); > > } > > > > /* Ingress or Egress ACL Table (Various priorities). */ > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jun 12, 2020 at 12:41 AM Mark Michelson <mmichels@redhat.com> wrote: > On 6/11/20 12:12 PM, Numan Siddique wrote: > > > > > > On Thu, Jun 11, 2020 at 8:59 PM Numan Siddique <numans@ovn.org > > <mailto:numans@ovn.org>> wrote: > > > > > > > > On Thu, Jun 11, 2020 at 8:13 PM Mark Michelson <mmichels@redhat.com > > <mailto:mmichels@redhat.com>> wrote: > > > > Hi Numan, > > > > I was a bit surprised to find that "nd" did not cover "nd_rs" and > > "nd_ra" packets already. Is there a reason not to expand the > > scope of > > "nd" to cover ICMP6 type 133 and 134? > > > > > > The document in ovn-sb.xml says: > > > > · nd expands to icmp6.type == {135, 136} && icmp6.code == 0 > > && ip.ttl == 255 > > > > So I think it was intended to be used for IPv6 Neighbor discovery > > packets and not > > for Router Adv/Router solicitation packets. > > > > > > After your email, I checked if we can expand "nd" to cover RA/RS too. > > But since there is this > > match - ip.ttl == 255, I think it's better not to. IPv6 RA/RS packets > > most definitely enter the router pipeline and ttl > > of the packet could be decremented. > > > > Thanks > > Numan > > Thanks for the answer, Numan. In that case, > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Thanks for the review. I applied this patch to master, branch-20.06 and branch-20.03. Numan > > > > > Thanks > > Numan > > > > > > On 6/11/20 9:14 AM, numans@ovn.org <mailto:numans@ovn.org> > wrote: > > > From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > > > We already add below logical flows to by pass IPv6 Neighbor > > discovery packets > > > from in/out ACL stage. > > > > > > table=6 (ls_in_acl ), priority=65535, match=(nd), > > action=(next;) > > > table=4 (ls_out_acl ), priority=65535, match=(nd), > > action=(next;) > > > > > > This patch also adds nd_rs and nd_ra to these logical flows. > > Without these > > > the IPv6 Router Adv packets generated by ovn-controller are > > dropped if > > > CMS has configured ACLs. > > > > > > Reported-by: Jakub Libosvar <jlibosva@redhat.com > > <mailto:jlibosva@redhat.com>> > > > Signed-off-by: Numan Siddique <numans@ovn.org > > <mailto:numans@ovn.org>> > > > --- > > > northd/ovn-northd.8.xml | 6 ++++++ > > > northd/ovn-northd.c | 6 ++++-- > > > 2 files changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > > index 7281eeecc..a7639f33a 100644 > > > --- a/northd/ovn-northd.8.xml > > > +++ b/northd/ovn-northd.8.xml > > > @@ -467,6 +467,12 @@ > > > ACL re-allow this connection. > > > </li> > > > > > > + <li> > > > + A priority-65535 flow that allows IPv6 Neighbor > > solicitation, > > > + Neighbor discover, Router solicitation and Router > > advertisement > > > + packets. > > > + </li> > > > + > > > <li> > > > A priority 34000 logical flow is added for each > > logical switch datapath > > > with the match <code>eth.dst = <var>E</var></code> > > to allow the service > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > > index 0fc62bf91..b8c9e9325 100644 > > > --- a/northd/ovn-northd.c > > > +++ b/northd/ovn-northd.c > > > @@ -5389,8 +5389,10 @@ build_acls(struct ovn_datapath *od, > > struct hmap *lflows, > > > /* Ingress and Egress ACL Table (Priority 65535). > > > * > > > * Not to do conntrack on ND packets. */ > > > - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, > > UINT16_MAX, "nd", "next;"); > > > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, > > UINT16_MAX, "nd", "next;"); > > > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, > UINT16_MAX, > > > + "nd || nd_ra || nd_rs", "next;"); > > > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, > UINT16_MAX, > > > + "nd || nd_ra || nd_rs", "next;"); > > > } > > > > > > /* Ingress or Egress ACL Table (Various priorities). */ > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 7281eeecc..a7639f33a 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -467,6 +467,12 @@ ACL re-allow this connection. </li> + <li> + A priority-65535 flow that allows IPv6 Neighbor solicitation, + Neighbor discover, Router solicitation and Router advertisement + packets. + </li> + <li> A priority 34000 logical flow is added for each logical switch datapath with the match <code>eth.dst = <var>E</var></code> to allow the service diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 0fc62bf91..b8c9e9325 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -5389,8 +5389,10 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, /* Ingress and Egress ACL Table (Priority 65535). * * Not to do conntrack on ND packets. */ - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, "nd", "next;"); - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, "nd", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX, + "nd || nd_ra || nd_rs", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, + "nd || nd_ra || nd_rs", "next;"); } /* Ingress or Egress ACL Table (Various priorities). */