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>

7 files changed, 50 insertions, 20 deletions

diff --git a/src/plugins/gbp/gbp_api.c b/src/plugins/gbp/gbp_api.c
index 09455471c5e..b347c1ddea4 100644
--- 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 452c5a5c359..e12b33145ce 100644
--- 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 139de1c372b..509e785dd9e 100644
--- 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 21e4bdba064..127c6d3f059 100644
--- 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 36bb4933c11..6f87f2ec7c4 100644
--- 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 dec30e46336..9f26b9c67ab 100644
--- 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 7bbcffa5b47..8c6ef5c2b94 100644
--- 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,