linux-cp: Fix add vs update on routes

Linux uses NLM_F_REPLACE in the netlink message to signal a FIB update
The code invariably does a FIB update for IPv4 and a addition for IPv6.
Without this fix, the following:
 ip route add 2001:db8::/48 via 2001:db8::1
 ip route replace 2001:db8::/48 via 2001:db8::2

ends up as two separate FIB entries in VPP. With the fix, there will be one FIB entry (the second one with nexthop ::2).

Type: fix
Change-Id: I8f98d6ded52ae0c60bfddaa7fc39acbbaa19d34a
Signed-off-by: Pim van Pelt <pim@ipng.nl>

diff --git a/src/plugins/linux-cp/lcp_nl.c b/src/plugins/linux-cp/lcp_nl.c
index 8f2bffd..b4fef7e 100644 (file)
--- a/src/plugins/linux-cp/lcp_nl.c
+++ b/src/plugins/linux-cp/lcp_nl.c
@@ -205,7 +205,10 @@ nl_route_del (struct rtnl_route *rr, void *arg)
 static void
 nl_route_add (struct rtnl_route *rr, void *arg)
 {
-  FOREACH_VFT (nvl_rt_route_add, rr);
+  nl_msg_info_t *msg_info = (nl_msg_info_t *) arg;
+  struct nlmsghdr *nlh = nlmsg_hdr (msg_info->msg);
+
+  FOREACH_VFT_CTX (nvl_rt_route_add, rr, (nlh->nlmsg_flags & NLM_F_REPLACE));
 }
 
 static void

diff --git a/src/plugins/linux-cp/lcp_nl.h b/src/plugins/linux-cp/lcp_nl.h
index 7b2fccc..41757e9 100644 (file)
--- a/src/plugins/linux-cp/lcp_nl.h
+++ b/src/plugins/linux-cp/lcp_nl.h
@@ -26,7 +26,8 @@ typedef void (*nl_rt_addr_cb_t) (struct rtnl_addr *ra);
 typedef void (*nl_rt_addr_sync_cb_t) (void);
 typedef void (*nl_rt_neigh_cb_t) (struct rtnl_neigh *rr);
 typedef void (*nl_rt_neigh_sync_cb_t) (void);
-typedef void (*nl_rt_route_cb_t) (struct rtnl_route *rn);
+typedef void (*nl_rt_route_add_cb_t) (struct rtnl_route *rn, int is_replace);
+typedef void (*nl_rt_route_del_cb_t) (struct rtnl_route *rn);
 typedef void (*nl_rt_route_sync_cb_t) (void);
 
 #define NL_RT_COMMON uword is_mp_safe
@@ -73,12 +74,19 @@ typedef struct nl_rt_neigh_sync_t_
   nl_rt_neigh_sync_cb_t cb;
 } nl_rt_neigh_sync_t;
 
-typedef struct nl_rt_route_t_
+typedef struct nl_rt_route_add_t_
 {
   NL_RT_COMMON;
 
-  nl_rt_route_cb_t cb;
-} nl_rt_route_t;
+  nl_rt_route_add_cb_t cb;
+} nl_rt_route_add_t;
+
+typedef struct nl_rt_route_del_t_
+{
+  NL_RT_COMMON;
+
+  nl_rt_route_del_cb_t cb;
+} nl_rt_route_del_t;
 
 typedef struct nl_rt_route_sync_t_
 {
@@ -103,8 +111,8 @@ typedef struct nl_vft_t_
   nl_rt_neigh_t nvl_rt_neigh_del;
   nl_rt_neigh_sync_t nvl_rt_neigh_sync_begin;
   nl_rt_neigh_sync_t nvl_rt_neigh_sync_end;
-  nl_rt_route_t nvl_rt_route_add;
-  nl_rt_route_t nvl_rt_route_del;
+  nl_rt_route_add_t nvl_rt_route_add;
+  nl_rt_route_del_t nvl_rt_route_del;
   nl_rt_route_sync_t nvl_rt_route_sync_begin;
   nl_rt_route_sync_t nvl_rt_route_sync_end;
 } nl_vft_t;

diff --git a/src/plugins/linux-cp/lcp_router.c b/src/plugins/linux-cp/lcp_router.c
index ad701c5..88b7c53 100644 (file)
--- a/src/plugins/linux-cp/lcp_router.c
+++ b/src/plugins/linux-cp/lcp_router.c
@@ -1184,7 +1184,7 @@ lcp_router_route_del (struct rtnl_route *rr)
 }
 
 static void
-lcp_router_route_add (struct rtnl_route *rr)
+lcp_router_route_add (struct rtnl_route *rr, int is_replace)
 {
   fib_entry_flag_t entry_flags;
   uint32_t table_id;
@@ -1213,71 +1213,71 @@ lcp_router_route_add (struct rtnl_route *rr)
       LCP_ROUTER_DBG ("route skip: %d:%U %U", rtnl_route_get_table (rr),
                      format_fib_prefix, &pfx, format_fib_entry_flags,
                      entry_flags);
+      return;
     }
-  else
-    {
-      LCP_ROUTER_DBG ("route add: %d:%U %U", rtnl_route_get_table (rr),
-                     format_fib_prefix, &pfx, format_fib_entry_flags,
-                     entry_flags);
+  LCP_ROUTER_DBG ("route %s: %d:%U %U", is_replace ? "replace" : "add",
+                 rtnl_route_get_table (rr), format_fib_prefix, &pfx,
+                 format_fib_entry_flags, entry_flags);
 
-      lcp_router_route_path_parse_t np = {
-       .route_proto = pfx.fp_proto,
-       .is_mcast = (rtype == RTN_MULTICAST),
-       .type_flags = lcp_router_route_type_frpflags[rtype],
-       .preference = (u8) rtnl_route_get_priority (rr),
-      };
+  lcp_router_route_path_parse_t np = {
+    .route_proto = pfx.fp_proto,
+    .is_mcast = (rtype == RTN_MULTICAST),
+    .type_flags = lcp_router_route_type_frpflags[rtype],
+    .preference = (u8) rtnl_route_get_priority (rr),
+  };
 
-      rtnl_route_foreach_nexthop (rr, lcp_router_route_path_parse, &np);
-      lcp_router_route_path_add_special (rr, &np);
+  rtnl_route_foreach_nexthop (rr, lcp_router_route_path_parse, &np);
+  lcp_router_route_path_add_special (rr, &np);
 
-      if (0 != vec_len (np.paths))
+  if (0 != vec_len (np.paths))
+    {
+      if (rtype == RTN_MULTICAST)
        {
-         if (rtype == RTN_MULTICAST)
-           {
-             /* it's not clear to me how linux expresses the RPF paramters
-              * so we'll allow from all interfaces and hope for the best */
-             mfib_prefix_t mpfx = {};
+         /* it's not clear to me how linux expresses the RPF paramters
+          * so we'll allow from all interfaces and hope for the best */
+         mfib_prefix_t mpfx = {};
 
-             lcp_router_route_mk_mprefix (rr, &mpfx);
+         lcp_router_route_mk_mprefix (rr, &mpfx);
 
-             mfib_table_entry_update (
-               nlt->nlt_mfib_index, &mpfx, MFIB_SOURCE_PLUGIN_LOW,
-               MFIB_RPF_ID_NONE, MFIB_ENTRY_FLAG_ACCEPT_ALL_ITF);
+         mfib_table_entry_update (nlt->nlt_mfib_index, &mpfx,
+                                  MFIB_SOURCE_PLUGIN_LOW, MFIB_RPF_ID_NONE,
+                                  MFIB_ENTRY_FLAG_ACCEPT_ALL_ITF);
 
-             mfib_table_entry_paths_update (nlt->nlt_mfib_index, &mpfx,
-                                            MFIB_SOURCE_PLUGIN_LOW,
-                                            MFIB_ENTRY_FLAG_NONE, np.paths);
-           }
-         else
-           {
-             fib_source_t fib_src;
-             const fib_route_path_t *rpath;
+         mfib_table_entry_paths_update (nlt->nlt_mfib_index, &mpfx,
+                                        MFIB_SOURCE_PLUGIN_LOW,
+                                        MFIB_ENTRY_FLAG_NONE, np.paths);
+       }
+      else
+       {
+         fib_source_t fib_src;
+         const fib_route_path_t *rpath;
 
-             vec_foreach (rpath, np.paths)
+         vec_foreach (rpath, np.paths)
+           {
+             if (fib_route_path_is_attached (rpath))
                {
-                 if (fib_route_path_is_attached (rpath))
-                   {
-                     entry_flags |= FIB_ENTRY_FLAG_ATTACHED;
-                     break;
-                   }
+                 entry_flags |= FIB_ENTRY_FLAG_ATTACHED;
+                 break;
                }
+           }
 
-             fib_src = lcp_router_proto_fib_source (rproto);
+         fib_src = lcp_router_proto_fib_source (rproto);
 
-             if (pfx.fp_proto == FIB_PROTOCOL_IP6)
-               fib_table_entry_path_add2 (nlt->nlt_fib_index, &pfx, fib_src,
-                                          entry_flags, np.paths);
-             else
-               fib_table_entry_update (nlt->nlt_fib_index, &pfx, fib_src,
-                                       entry_flags, np.paths);
-           }
+         if (is_replace)
+           fib_table_entry_update (nlt->nlt_fib_index, &pfx, fib_src,
+                                   entry_flags, np.paths);
+         else
+           fib_table_entry_path_add2 (nlt->nlt_fib_index, &pfx, fib_src,
+                                      entry_flags, np.paths);
        }
-      else
-       LCP_ROUTER_DBG ("no paths for route add: %d:%U %U",
-                       rtnl_route_get_table (rr), format_fib_prefix, &pfx,
-                       format_fib_entry_flags, entry_flags);
-      vec_free (np.paths);
     }
+  else
+    {
+      LCP_ROUTER_DBG ("no paths for route: %d:%U %U",
+                     rtnl_route_get_table (rr), format_fib_prefix, &pfx,
+                     format_fib_entry_flags, entry_flags);
+    }
+  vec_free (np.paths);
 }
 
 static void