gbp: fix contract rule handling

Fix a memory leak when removing old GBP contract rules and make sure a
GBP contract rule exists when matching the corresponding ACL rule.

Type: fix
Fixes: 13a08cc098

Change-Id: Iba67d573e69280ad998488a7a3d3462341c68ea4
Signed-off-by: Benoît Ganne <bganne@cisco.com>

diff --git a/src/plugins/gbp/gbp_api.c b/src/plugins/gbp/gbp_api.c
index 0945547..b347c1d 100644 (file)
--- a/src/plugins/gbp/gbp_api.c
+++ b/src/plugins/gbp/gbp_api.c
@@ -940,6 +940,8 @@ gbp_contract_rules_decode (u8 n_rules,
 
       if (0 != rv)
        {
+         index_t *gui;
+         vec_foreach (gui, guis) gbp_rule_free (*gui);
          vec_free (guis);
          return (rv);
        }

diff --git a/src/plugins/gbp/gbp_contract.c b/src/plugins/gbp/gbp_contract.c
index 452c5a5..e12b331 100644 (file)
--- a/src/plugins/gbp/gbp_contract.c
+++ b/src/plugins/gbp/gbp_contract.c
@@ -73,6 +73,12 @@ gbp_rule_alloc (gbp_rule_action_t action,
   return (gu - gbp_rule_pool);
 }
 
+void
+gbp_rule_free (index_t gui)
+{
+  pool_put_index (gbp_rule_pool, gui);
+}
+
 index_t
 gbp_next_hop_alloc (const ip46_address_t * ip,
                    index_t grd, const mac_address_t * mac, index_t gbd)
@@ -139,6 +145,8 @@ gbp_contract_rules_free (index_t * rules)
        adj_unlock (gnh->gnh_ai[fproto]);
       }
     }
+
+    gbp_rule_free (*gui);
   }
   vec_free (rules);
 }
@@ -159,7 +167,7 @@ format_gbp_next_hop (u8 * s, va_list * args)
   return (s);
 }
 
-static u8 *
+u8 *
 format_gbp_rule_action (u8 * s, va_list * args)
 {
   gbp_rule_action_t action = va_arg (*args, gbp_rule_action_t);

diff --git a/src/plugins/gbp/gbp_contract.h b/src/plugins/gbp/gbp_contract.h
index 139de1c..509e785 100644 (file)
--- a/src/plugins/gbp/gbp_contract.h
+++ b/src/plugins/gbp/gbp_contract.h
@@ -28,7 +28,8 @@
   _(DROP_CONTRACT,      "drop-contract")                   \
   _(DROP_ETHER_TYPE,    "drop-ether-type")                 \
   _(DROP_NO_CONTRACT,   "drop-no-contract")                \
-  _(DROP_NO_DCLASS,     "drop-no-dclass")
+  _(DROP_NO_DCLASS,     "drop-no-dclass")                  \
+  _(DROP_NO_RULE,       "drop-no-rule")
 
 typedef enum
 {
@@ -173,6 +174,7 @@ extern int gbp_contract_delete (gbp_scope_t scope, sclass_t sclass,
 
 extern index_t gbp_rule_alloc (gbp_rule_action_t action,
                               gbp_hash_mode_t hash_mode, index_t * nhs);
+extern void gbp_rule_free (index_t gui);
 extern index_t gbp_next_hop_alloc (const ip46_address_t * ip,
                                   index_t grd,
                                   const mac_address_t * mac, index_t gbd);
@@ -180,6 +182,7 @@ extern index_t gbp_next_hop_alloc (const ip46_address_t * ip,
 typedef int (*gbp_contract_cb_t) (gbp_contract_t * gbpe, void *ctx);
 extern void gbp_contract_walk (gbp_contract_cb_t bgpe, void *ctx);
 
+extern u8 *format_gbp_rule_action (u8 * s, va_list * args);
 extern u8 *format_gbp_contract (u8 * s, va_list * args);
 
 /**
@@ -230,13 +233,14 @@ static_always_inline gbp_rule_action_t
 gbp_contract_apply (vlib_main_t * vm, gbp_main_t * gm,
                    gbp_contract_key_t * key, vlib_buffer_t * b,
                    gbp_rule_t ** rule, u32 * intra, u32 * sclass1,
+                   u32 * acl_match, u32 * rule_match,
                    gbp_contract_error_t * err,
                    gbp_contract_apply_type_t type)
 {
   fa_5tuple_opaque_t fa_5tuple;
   const gbp_contract_t *contract;
   index_t contract_index;
-  u32 acl_pos, acl_match, rule_match, trace_bitmap;
+  u32 acl_pos, trace_bitmap;
   u16 etype;
   u8 ip6, action;
 
@@ -315,12 +319,18 @@ gbp_contract_apply (vlib_main_t * vm, gbp_main_t * gm,
                                 &fa_5tuple);
   acl_plugin_match_5tuple_inline (gm->acl_plugin.p_acl_main,
                                  contract->gc_lc_index, &fa_5tuple, ip6,
-                                 &action, &acl_pos, &acl_match, &rule_match,
+                                 &action, &acl_pos, acl_match, rule_match,
                                  &trace_bitmap);
   if (action <= 0)
     goto contract_deny;
 
-  *rule = gbp_rule_get (contract->gc_rules[rule_match]);
+  if (PREDICT_FALSE (*rule_match >= vec_len (contract->gc_rules)))
+    {
+      *err = GBP_CONTRACT_ERROR_DROP_NO_RULE;
+      goto contract_deny;
+    }
+
+  *rule = gbp_rule_get (contract->gc_rules[*rule_match]);
   switch ((*rule)->gu_action)
     {
     case GBP_RULE_PERMIT:

diff --git a/src/plugins/gbp/gbp_policy.c b/src/plugins/gbp/gbp_policy.c
index 21e4bdb..127c6d3 100644 (file)
--- a/src/plugins/gbp/gbp_policy.c
+++ b/src/plugins/gbp/gbp_policy.c
@@ -28,9 +28,11 @@ format_gbp_policy_trace (u8 * s, va_list * args)
   gbp_policy_trace_t *t = va_arg (*args, gbp_policy_trace_t *);
 
   s =
-    format (s, "scope:%d sclass:%d, dclass:%d, allowed:%d flags:%U", t->scope,
-           t->sclass, t->dclass, t->allowed, format_vxlan_gbp_header_gpflags,
-           t->flags);
+    format (s,
+           "scope:%d sclass:%d, dclass:%d, action:%U flags:%U acl: %d rule: %d",
+           t->scope, t->sclass, t->dclass, format_gbp_rule_action, t->action,
+           format_vxlan_gbp_header_gpflags, t->flags, t->acl_match,
+           t->rule_match);
 
   return s;
 }

diff --git a/src/plugins/gbp/gbp_policy.h b/src/plugins/gbp/gbp_policy.h
index 36bb493..6f87f2e 100644 (file)
--- a/src/plugins/gbp/gbp_policy.h
+++ b/src/plugins/gbp/gbp_policy.h
@@ -27,15 +27,17 @@ typedef struct gbp_policy_trace_t_
   gbp_scope_t scope;
   sclass_t sclass;
   sclass_t dclass;
-  u32 allowed;
+  gbp_rule_action_t action;
   u32 flags;
+  u32 acl_match;
+  u32 rule_match;
 } gbp_policy_trace_t;
 
 /* packet trace format function */
 u8 * format_gbp_policy_trace (u8 * s, va_list * args);
 
 static_always_inline void
-gbp_policy_trace(vlib_main_t * vm, vlib_node_runtime_t * node, vlib_buffer_t *b, const gbp_contract_key_t *key, u8 allowed)
+gbp_policy_trace(vlib_main_t * vm, vlib_node_runtime_t * node, vlib_buffer_t *b, const gbp_contract_key_t *key, gbp_rule_action_t action, u32 acl_match, u32 rule_match)
 {
   gbp_policy_trace_t *t;
 
@@ -46,8 +48,10 @@ gbp_policy_trace(vlib_main_t * vm, vlib_node_runtime_t * node, vlib_buffer_t *b,
   t->sclass = key->gck_src;
   t->dclass = key->gck_dst;
   t->scope = key->gck_scope;
-  t->allowed = allowed;
+  t->action = action;
   t->flags = vnet_buffer2 (b)->gbp.flags;
+  t->acl_match = acl_match;
+  t->rule_match = rule_match;
 }
 
 #endif /* __GBP_POLICY_H__ */

diff --git a/src/plugins/gbp/gbp_policy_dpo.c b/src/plugins/gbp/gbp_policy_dpo.c
index dec30e4..9f26b9c 100644 (file)
--- a/src/plugins/gbp/gbp_policy_dpo.c
+++ b/src/plugins/gbp/gbp_policy_dpo.c
@@ -268,13 +268,14 @@ gbp_policy_dpo_inline (vlib_main_t * vm,
 
       while (n_left_from > 0 && n_left_to_next > 0)
        {
+         gbp_rule_action_t action0 = GBP_RULE_DENY;
+         u32 acl_match = ~0, rule_match = ~0;
          const gbp_policy_dpo_t *gpd0;
-         gbp_rule_action_t action0;
          gbp_contract_error_t err0;
-         u32 bi0, next0;
          gbp_contract_key_t key0;
          vlib_buffer_t *b0;
          gbp_rule_t *rule0;
+         u32 bi0, next0;
 
          bi0 = from[0];
          to_next[0] = bi0;
@@ -325,7 +326,8 @@ gbp_policy_dpo_inline (vlib_main_t * vm,
 
          action0 =
            gbp_contract_apply (vm, gm, &key0, b0, &rule0, &n_allow_intra,
-                               &n_allow_sclass_1, &err0,
+                               &n_allow_sclass_1, &acl_match, &rule_match,
+                               &err0,
                                is_ip6 ? GBP_CONTRACT_APPLY_IP6 :
                                GBP_CONTRACT_APPLY_IP4);
          switch (action0)
@@ -345,7 +347,8 @@ gbp_policy_dpo_inline (vlib_main_t * vm,
            }
 
        trace:
-         gbp_policy_trace (vm, node, b0, &key0, (next0 != GBP_POLICY_DROP));
+         gbp_policy_trace (vm, node, b0, &key0, action0, acl_match,
+                           rule_match);
 
          vlib_validate_buffer_enqueue_x1 (vm, node, next_index, to_next,
                                           n_left_to_next, bi0, next0);

diff --git a/src/plugins/gbp/gbp_policy_node.c b/src/plugins/gbp/gbp_policy_node.c
index 7bbcffa..8c6ef5c 100644 (file)
--- a/src/plugins/gbp/gbp_policy_node.c
+++ b/src/plugins/gbp/gbp_policy_node.c
@@ -116,10 +116,11 @@ gbp_policy_inline (vlib_main_t * vm,
 
       while (n_left_from > 0 && n_left_to_next > 0)
        {
+         gbp_rule_action_t action0 = GBP_RULE_DENY;
          const ethernet_header_t *h0;
          const gbp_endpoint_t *ge0;
-         gbp_rule_action_t action0;
          gbp_contract_error_t err0;
+         u32 acl_match = ~0, rule_match = ~0;
          gbp_policy_next_t next0;
          gbp_contract_key_t key0;
          u32 bi0, sw_if_index0;
@@ -220,8 +221,8 @@ gbp_policy_inline (vlib_main_t * vm,
 
          action0 =
            gbp_contract_apply (vm, gm, &key0, b0, &rule0, &n_allow_intra,
-                               &n_allow_sclass_1, &err0,
-                               GBP_CONTRACT_APPLY_L2);
+                               &n_allow_sclass_1, &acl_match, &rule_match,
+                               &err0, GBP_CONTRACT_APPLY_L2);
          switch (action0)
            {
            case GBP_RULE_PERMIT:
@@ -239,8 +240,8 @@ gbp_policy_inline (vlib_main_t * vm,
            }
 
        trace:
-         gbp_policy_trace (vm, node, b0, &key0,
-                           (next0 != GBP_POLICY_NEXT_DROP));
+         gbp_policy_trace (vm, node, b0, &key0, action0, acl_match,
+                           rule_match);
 
          /* verify speculative enqueue, maybe switch current next frame */
          vlib_validate_buffer_enqueue_x1 (vm, node, next_index,