diff mbox series

[ovs-dev,ovn] northd: By pass IPv6 Router Adv and Router Solicitation packets from ACL stages.

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

Commit Message

Numan Siddique June 11, 2020, 1:14 p.m. UTC 
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(-)

Comments

Mark Michelson June 11, 2020, 2:43 p.m. UTC | #1 
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). */
>
Numan Siddique June 11, 2020, 3:29 p.m. UTC | #2 
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
>
>
Numan Siddique June 11, 2020, 4:12 p.m. UTC | #3 
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
>>
>>
Mark Michelson June 11, 2020, 7:11 p.m. UTC | #4 
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
>
Numan Siddique June 15, 2020, 8:57 a.m. UTC | #5 
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 mbox series

Patch

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). */