From b98a3a87a968d9675103e16b6d4b3949955288c0 Mon Sep 17 00:00:00 2001 From: Chris Luke Date: Thu, 2 Jun 2016 11:00:41 -0400 Subject: VPP-91 fix sr tunnel add_del collision check The add_del function was not properly checking if a tunnel already existed; instead it was checking if the given tunnel name existed. If no tunnel name was given it flat out refused to add a tunnel even though that is optional. Cleanup the add/del parameter validation to "do what I expect" it to do: When adding a tunnel: - If a "name" is given, it must not exist. - The "key" is always checked, and must not exist. When deleting a tunnel: - If the "name" is given, and it exists, then use it. - If the "name" is not given, use the "key". - If the "name" and the "key" are given, then both must point to the same thing. Change-Id: I9b48ae0203f9664cf8af0f7dc49bf480ddec10d5 Signed-off-by: Chris Luke (cherry picked from commit e54436005341800f76a584299ef8bf99e8d66227) --- vnet/vnet/sr/sr.c | 143 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 85 insertions(+), 58 deletions(-) diff --git a/vnet/vnet/sr/sr.c b/vnet/vnet/sr/sr.c index 1a20ee1aaf3..9b3973e3670 100644 --- a/vnet/vnet/sr/sr.c +++ b/vnet/vnet/sr/sr.c @@ -743,7 +743,7 @@ int ip6_sr_add_del_tunnel (ip6_sr_add_del_tunnel_args_t * a) ip_lookup_main_t * lm = &im->lookup_main; ip6_sr_tunnel_key_t key; ip6_sr_tunnel_t * t; - uword * p; + uword * p, * n; ip6_sr_header_t * h = 0; u32 header_length; ip6_address_t * addrp, *this_address; @@ -779,69 +779,94 @@ int ip6_sr_add_del_tunnel (ip6_sr_add_del_tunnel_args_t * a) clib_memcpy (key.src.as_u8, a->src_address->as_u8, sizeof (key.src)); clib_memcpy (key.dst.as_u8, a->dst_address->as_u8, sizeof (key.dst)); - /* If the name exists, find the tunnel by name else... */ - if (a->name) - p = hash_get_mem(sm->tunnel_index_by_name, a->name); - else if (p==0) - p = hash_get_mem (sm->tunnel_index_by_key, &key); + /* When adding a tunnel: + * - If a "name" is given, it must not exist. + * - The "key" is always checked, and must not exist. + * When deleting a tunnel: + * - If the "name" is given, and it exists, then use it. + * - If the "name" is not given, use the "key". + * - If the "name" and the "key" are given, then both must point to the same + * thing. + */ - if (p) - { - if (a->is_del) - { - hash_pair_t *hp; - - /* Delete existing tunnel */ - t = pool_elt_at_index (sm->tunnels, p[0]); + /* Lookup the key */ + p = hash_get_mem (sm->tunnel_index_by_key, &key); - ip6_delete_route_no_next_hop (&t->key.dst, t->dst_mask_width, - a->rx_table_id); - vec_free (t->rewrite); - /* Remove tunnel from any policy if associated */ - if (t->policy_index != ~0) - { - pt=pool_elt_at_index (sm->policies, t->policy_index); - for (i=0; i< vec_len (pt->tunnel_indices); i++) - { - if (pt->tunnel_indices[i] == t - sm->tunnels) - { - vec_delete (pt->tunnel_indices, 1, i); - goto found; - } - } - clib_warning ("Tunnel index %d not found in policy_index %d", - t - sm->tunnels, pt - sm->policies); - found: - /* If this is last tunnel in the policy, clean up the policy too */ - if (vec_len (pt->tunnel_indices) == 0) - { - hash_unset_mem (sm->policy_index_by_policy_name, pt->name); - vec_free (pt->name); - pool_put (sm->policies, pt); - } - } + /* If the name is given, look it up */ + if (a->name) + n = hash_get_mem(sm->tunnel_index_by_name, a->name); + else + n = 0; - /* Clean up the tunnel by name */ - if (t->name) - { - hash_unset_mem (sm->tunnel_index_by_name, t->name); - vec_free (t->name); - } - pool_put (sm->tunnels, t); - hp = hash_get_pair (sm->tunnel_index_by_key, &key); - key_copy = (void *)(hp->key); - hash_unset_mem (sm->tunnel_index_by_key, &key); - vec_free (key_copy); - return 0; - } - else /* create; tunnel already exists; complain */ + /* validate key/name parameters */ + if (!a->is_del) /* adding a tunnel */ + { + if (a->name && n) /* name given & exists already */ + return -1; + if (p) /* key exists already */ return -1; } - else + else /* deleting a tunnel */ { - /* delete; tunnel does not exist; complain */ - if (a->is_del) + if (!p) /* key doesn't exist */ + return -2; + if (a->name && !n) /* name given & it doesn't exist */ return -2; + + if (n) /* name given & found */ + { + if (n[0] != p[0]) /* name and key do not point to the same thing */ + return -2; + } + } + + + if (a->is_del) /* delete the tunnel */ + { + hash_pair_t *hp; + + /* Delete existing tunnel */ + t = pool_elt_at_index (sm->tunnels, p[0]); + + ip6_delete_route_no_next_hop (&t->key.dst, t->dst_mask_width, + a->rx_table_id); + vec_free (t->rewrite); + /* Remove tunnel from any policy if associated */ + if (t->policy_index != ~0) + { + pt=pool_elt_at_index (sm->policies, t->policy_index); + for (i=0; i< vec_len (pt->tunnel_indices); i++) + { + if (pt->tunnel_indices[i] == t - sm->tunnels) + { + vec_delete (pt->tunnel_indices, 1, i); + goto found; + } + } + clib_warning ("Tunnel index %d not found in policy_index %d", + t - sm->tunnels, pt - sm->policies); + found: + /* If this is last tunnel in the policy, clean up the policy too */ + if (vec_len (pt->tunnel_indices) == 0) + { + hash_unset_mem (sm->policy_index_by_policy_name, pt->name); + vec_free (pt->name); + pool_put (sm->policies, pt); + } + } + + /* Clean up the tunnel by name */ + if (t->name) + { + hash_unset_mem (sm->tunnel_index_by_name, t->name); + vec_free (t->name); + } + pool_put (sm->tunnels, t); + hp = hash_get_pair (sm->tunnel_index_by_key, &key); + key_copy = (void *)(hp->key); + hash_unset_mem (sm->tunnel_index_by_key, &key); + vec_free (key_copy); + return 0; } /* create a new tunnel */ @@ -1162,7 +1187,9 @@ sr_add_del_tunnel_command_fn (vlib_main_t * vm, VLIB_CLI_COMMAND (sr_tunnel_command, static) = { .path = "sr tunnel", .short_help = - "sr tunnel [del] [name ] src dst [next ] [cleanup] [reroute] [key %s] [policy ", + "sr tunnel [del] [name ] src dst [next ] " + "[clean] [reroute] [key ] [policy ]" + "[rx-fib-id ] [tx-fib-id ]", .function = sr_add_del_tunnel_command_fn, }; -- cgit 1.2.3-korg