[ovs-dev] [PATCH] [PATCH, v7] ovn: Add 'na' action and lflow for ND

Ben Pfaff blp at ovn.org
Sat Jul 2 18:43:41 UTC 2016


On Mon, Jun 27, 2016 at 02:54:52PM +0800, Zong Kai LI wrote:
> This patch tries to support ND versus ARP for OVN.
> 
> It adds a new OVN action 'na' in ovn-controller side, and modify lflows
> for 'na' action and relevant packets in ovn-northd.
> 
> First, for ovn-northd, it will generate lflows per each lport with its
> IPv6 addresses and mac addresss, with 'na' action, such as:
>   match=(icmp6 && icmp6.type == 135 &&
>          (nd.target == fd81:ce49:a948:0:f816:3eff:fe46:8a42 ||
>           nd.target == fd81:ce49:b123:0:f816:3eff:fe46:8a42)),
>   action=(na { eth.src = fa:16:3e:46:8a:42; nd.tll = fa:16:3e:46:8a:42;
>                outport = inport;
>                inport = ""; /* Allow sending out inport. */ output; };)
> 
> and new lflows will be set in tabel ls_in_arp_nd_rsp, which is renamed
> from previous ls_in_arp_rsp.
> 
> Later, for ovn-controller, when it received a ND packet, it frames a
> template NA packet for reply. The NA packet will be initialized based on
> ND packet, such as NA packet will use:
>  - ND packet eth.src as eth.dst,
>  - ND packet eth.dst as eth.src,
>  - ND packet ip6.src as ip6.dst,
>  - ND packet nd.target as ip6.src,
>  - ND packet eth.dst as nd.tll.
> 
> Finally, nested actions in 'na' action will update necessary fileds
> for NA packet, such as:
>  - eth.src, nd.tll
>  - inport, outport
> 
> Since patch port for IPv6 router interface is not ready yet, this
> patch will only try to deal with ND from VM. This patch will set
> RSO flags to 011 for NA packets.
> 
> This patch also modified current ACL lflows for ND, not to do conntrack
> on ND and NA packets in following tables:
>  - S_SWITCH_IN_PRE_ACL
>  - S_SWITCH_OUT_PRE_ACL
>  - S_SWITCH_IN_ACL
>  - S_SWITCH_OUT_ACL
> 
> (Rebase on upstream)
> 
> Signed-off-by: Zong Kai LI <zealokii at gmail.com>

Thanks for the patch!

I made some minor simplifications and improvements.  Most notably, one
of the tests didn't pass because the wrong opcode was being used (2
instead of 3); I fixed that.  I'm appending an incremental diff below.
I also added a preparatory patch just before it.  I'll send the
resulting two-patch series separately.

I applied both patches to master.

--8<--------------------------cut here-------------------------->8--

diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index f7edfd3..f960ea9 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -25,7 +25,6 @@
 #include "lport.h"
 #include "nx-match.h"
 #include "ovn-controller.h"
-#include "lib/byte-order.h"
 #include "lib/packets.h"
 #include "lib/sset.h"
 #include "openvswitch/ofp-actions.h"
@@ -943,12 +942,11 @@ pinctrl_handle_na(const struct flow *ip_flow,
                   const struct match *md,
                   struct ofpbuf *userdata)
 {
-    /* This action only works for IPv6 packets, and the switch should only send
-     * us IPv6 packets this way, but check here just to be sure. */
-    if (ip_flow->dl_type != htons(ETH_TYPE_IPV6)) {
+    /* This action only works for IPv6 ND packets, and the switch should only
+     * send us ND packets this way, but check here just to be sure. */
+    if (!is_nd(ip_flow, NULL)) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
-        VLOG_WARN_RL(&rl, "NA action on non-IPv6 packet (Ethertype %"PRIx16")",
-                     ntohs(ip_flow->dl_type));
+        VLOG_WARN_RL(&rl, "NA action on non-ND packet");
         return;
     }
 
@@ -958,17 +956,11 @@ pinctrl_handle_na(const struct flow *ip_flow,
     uint64_t packet_stub[128 / 8];
     struct dp_packet packet;
     dp_packet_use_stub(&packet, packet_stub, sizeof packet_stub);
+
     ovs_be32 ipv6_src[4], ipv6_dst[4];
-    for (int i = 0; i < 4; i++) {
-        ipv6_dst[i] = BYTES_TO_BE32(ip_flow->ipv6_src.s6_addr[4*i],
-                                    ip_flow->ipv6_src.s6_addr[4*i+1],
-                                    ip_flow->ipv6_src.s6_addr[4*i+2],
-                                    ip_flow->ipv6_src.s6_addr[4*i+3]);
-        ipv6_src[i] = BYTES_TO_BE32(ip_flow->nd_target.s6_addr[4*i],
-                                    ip_flow->nd_target.s6_addr[4*i+1],
-                                    ip_flow->nd_target.s6_addr[4*i+2],
-                                    ip_flow->nd_target.s6_addr[4*i+3]);
-    }
+    memcpy(ipv6_dst, &ip_flow->ipv6_src, sizeof ipv6_src);
+    memcpy(ipv6_src, &ip_flow->nd_target, sizeof ipv6_dst);
+
     /* Frame the NA packet with RSO=011. */
     compose_na(&packet,
                ip_flow->dl_dst, ip_flow->dl_src,
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index 5ec73be..550bb87 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -246,8 +246,11 @@ put_controller_op(struct ofpbuf *ofpacts, enum action_opcode opcode)
     finish_controller_op(ofpacts, ofs);
 }
 
+/* Implements the "arp" and "na" actions, which execute nested actions on a
+ * packet derived from the one being processed. */
 static void
-parse_arp_action(struct action_context *ctx)
+parse_nested_action(struct action_context *ctx, enum action_opcode opcode,
+                    const char *prereq)
 {
     if (!lexer_match(ctx->lexer, LEX_T_LCURLY)) {
         action_syntax_error(ctx, "expecting `{'");
@@ -272,13 +275,11 @@ parse_arp_action(struct action_context *ctx)
 
     ctx->ofpacts = outer_ofpacts;
 
-    /* Add a "controller" action with the actions nested inside "arp {...}",
+    /* Add a "controller" action with the actions nested inside "{...}",
      * converted to OpenFlow, as its userdata.  ovn-controller will convert the
-     * packet to an ARP and then send the packet and actions back to the switch
-     * inside an OFPT_PACKET_OUT message. */
-    /* controller. */
-    size_t oc_offset = start_controller_op(ctx->ofpacts, ACTION_OPCODE_ARP,
-                                           false);
+     * packet to ARP or NA and then send the packet and actions back to the
+     * switch inside an OFPT_PACKET_OUT message. */
+    size_t oc_offset = start_controller_op(ctx->ofpacts, opcode, false);
     ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size,
                                  ctx->ofpacts, OFP13_VERSION);
     finish_controller_op(ctx->ofpacts, oc_offset);
@@ -286,7 +287,7 @@ parse_arp_action(struct action_context *ctx)
     /* Restore prerequisites. */
     expr_destroy(ctx->prereqs);
     ctx->prereqs = outer_prereqs;
-    add_prerequisite(ctx, "ip4");
+    add_prerequisite(ctx, prereq);
 
     /* Free memory. */
     ofpbuf_uninit(&inner_ofpacts);
@@ -876,51 +877,6 @@ parse_ct_nat(struct action_context *ctx, bool snat)
     ofpbuf_push_uninit(ctx->ofpacts, ct_offset);
 }
 
-static void
-parse_na_action(struct action_context *ctx)
-{
-    if (!lexer_match(ctx->lexer, LEX_T_LCURLY)) {
-        action_syntax_error(ctx, "expecting `{'");
-        return;
-    }
-
-    struct ofpbuf *outer_ofpacts = ctx->ofpacts;
-    uint64_t inner_ofpacts_stub[1024 / 8];
-    struct ofpbuf inner_ofpacts = OFPBUF_STUB_INITIALIZER(inner_ofpacts_stub);
-    ctx->ofpacts = &inner_ofpacts;
-
-    /* Save prerequisites.  (XXX What is the right treatment for prereqs?) */
-    struct expr *outer_prereqs = ctx->prereqs;
-    ctx->prereqs = NULL;
-
-    /* Parse inner actions. */
-    while (!lexer_match(ctx->lexer, LEX_T_RCURLY)) {
-        if (!parse_action(ctx)) {
-            break;
-        }
-    }
-
-    ctx->ofpacts = outer_ofpacts;
-
-    /* Add a "controller" action with the actions nested inside "na {...}",
-     * converted to OpenFlow, as its userdata.  ovn-controller will convert the
-     * packet to an NA and then send the packet and actions back to the switch
-     * inside an OFPT_PACKET_OUT message. */
-    size_t oc_offset = start_controller_op(ctx->ofpacts, ACTION_OPCODE_NA,
-                                           true);
-    ofpacts_put_openflow_actions(inner_ofpacts.data, inner_ofpacts.size,
-                                 ctx->ofpacts, OFP13_VERSION);
-    finish_controller_op(ctx->ofpacts, oc_offset);
-
-    /* Restore prerequisites. */
-    expr_destroy(ctx->prereqs);
-    ctx->prereqs = outer_prereqs;
-    add_prerequisite(ctx, "nd");
-
-    /* Free memory. */
-    ofpbuf_uninit(&inner_ofpacts);
-}
-
 static bool
 parse_action(struct action_context *ctx)
 {
@@ -953,13 +909,13 @@ parse_action(struct action_context *ctx)
     } else if (lexer_match_id(ctx->lexer, "ct_snat")) {
         parse_ct_nat(ctx, true);
     } else if (lexer_match_id(ctx->lexer, "arp")) {
-        parse_arp_action(ctx);
+        parse_nested_action(ctx, ACTION_OPCODE_ARP, "ip4");
+    } else if (lexer_match_id(ctx->lexer, "na")) {
+        parse_nested_action(ctx, ACTION_OPCODE_NA, "nd");
     } else if (lexer_match_id(ctx->lexer, "get_arp")) {
         parse_get_arp_action(ctx);
     } else if (lexer_match_id(ctx->lexer, "put_arp")) {
         parse_put_arp_action(ctx);
-    } else if (lexer_match_id(ctx->lexer, "na")) {
-        parse_na_action(ctx);
     } else {
         action_syntax_error(ctx, "expecting action");
     }
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index dcf98e5..ebc5cf0 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1048,10 +1048,11 @@
 
         <dd>
           <p>
-            Temporarily replaces the IPv6 packet being processed by an NA
-            packet and executes each nested <var>action</var> on the NA
-            packet.  Actions following the <var>na</var> action, if any, apply
-            to the original, unmodified packet.
+            Temporarily replaces the IPv6 packet being processed by an IPv6
+            neighbor advertisement (NA) packet and executes each nested
+            <var>action</var> on the NA packet.  Actions following the
+            <var>na</var> action, if any, apply to the original, unmodified
+            packet.
           </p>
 
           <p>
diff --git a/tests/ovn.at b/tests/ovn.at
index fb1d6f1..778cac7 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -574,7 +574,7 @@ reg1[0] = put_dhcp_opts(offerip="xyzzy"); => DHCP option offerip requires numeri
 reg1[0] = put_dhcp_opts(offerip=1.2.3.4, domain=1.2.3.4); => DHCP option domain requires string value.
 
 # na
-na { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow sending out inport. */ output; }; => actions=controller(userdata=00.00.00.02.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.0c.04.00.01.0e.04.00.19.00.10.00.01.0c.04.00.00.00.00.00.00.00.00.00.19.00.10.00.00.00.02.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00), prereqs=nd
+na { eth.src = 12:34:56:78:9a:bc; nd.tll = 12:34:56:78:9a:bc; outport = inport; inport = ""; /* Allow sending out inport. */ output; }; => actions=controller(userdata=00.00.00.03.00.00.00.00.00.19.00.10.80.00.08.06.12.34.56.78.9a.bc.00.00.00.19.00.10.80.00.42.06.12.34.56.78.9a.bc.00.00.ff.ff.00.18.00.00.23.20.00.06.00.20.00.00.00.00.00.01.0c.04.00.01.0e.04.00.19.00.10.00.01.0c.04.00.00.00.00.00.00.00.00.00.19.00.10.00.00.00.02.00.00.00.00.00.00.00.00.ff.ff.00.10.00.00.23.20.00.0e.ff.f8.40.00.00.00), prereqs=nd
 
 # Contradictionary prerequisites (allowed but not useful):
 ip4.src = ip6.src[0..31]; => actions=move:NXM_NX_IPV6_SRC[0..31]->NXM_OF_IP_SRC[], prereqs=eth.type == 0x800 && eth.type == 0x86dd



More information about the dev mailing list