Merge branch 'sparx5-vcap-improve-locking'

Steen Hegelund says:

====================
sparx5: Improve locking in the VCAP API

This improves the VCAP cache and the VCAP rule list protection against
access from different sources.

The VCAP Admin lock protects the list of rules for the VCAP instance as
well as the cache used for encoding and decoding rules.

This series provides dedicated functions for accessing rule statistics,
decoding rule content, verifying if a rule exists and getting a rule with
the lock held, as well as ensuring the use of the lock when the list of
rules or the cache is accessed.
====================

Signed-off-by: David S. Miller <davem@davemloft.net>

drivers/net/ethernet/microchip/sparx5/sparx5_tc_flower.c

@@ -35,11 +35,6 @@ struct sparx5_tc_flower_parse_usage {
 	unsigned int used_keys;
 };
 
-struct sparx5_tc_rule_pkt_cnt {
-	u64 cookie;
-	u32 pkts;
-};
-
 /* These protocols have dedicated keysets in IS2 and a TC dissector
  * ETH_P_ARP does not have a TC dissector
  */
@@ -947,44 +942,21 @@ static int sparx5_tc_flower_destroy(struct net_device *ndev,
 	return err;
 }
 
-/* Collect packet counts from all rules with the same cookie */
-static int sparx5_tc_rule_counter_cb(void *arg, struct vcap_rule *rule)
-{
-	struct sparx5_tc_rule_pkt_cnt *rinfo = arg;
-	struct vcap_counter counter;
-	int err = 0;
-
-	if (rule->cookie == rinfo->cookie) {
-		err = vcap_rule_get_counter(rule, &counter);
-		if (err)
-			return err;
-		rinfo->pkts += counter.value;
-		/* Reset the rule counter */
-		counter.value = 0;
-		vcap_rule_set_counter(rule, &counter);
-	}
-	return err;
-}
-
 static int sparx5_tc_flower_stats(struct net_device *ndev,
 				  struct flow_cls_offload *fco,
 				  struct vcap_admin *admin)
 {
 	struct sparx5_port *port = netdev_priv(ndev);
-	struct sparx5_tc_rule_pkt_cnt rinfo = {};
+	struct vcap_counter ctr = {};
 	struct vcap_control *vctrl;
 	ulong lastused = 0;
-	u64 drops = 0;
-	u32 pkts = 0;
 	int err;
 
-	rinfo.cookie = fco->cookie;
 	vctrl = port->sparx5->vcap_ctrl;
-	err = vcap_rule_iter(vctrl, sparx5_tc_rule_counter_cb, &rinfo);
+	err = vcap_get_rule_count_by_cookie(vctrl, &ctr, fco->cookie);
 	if (err)
 		return err;
-	pkts = rinfo.pkts;
-	flow_stats_update(&fco->stats, 0x0, pkts, drops, lastused,
+	flow_stats_update(&fco->stats, 0x0, ctr.value, 0, lastused,
 			  FLOW_ACTION_HW_STATS_IMMEDIATE);
 	return err;
 }

drivers/net/ethernet/microchip/vcap/vcap_api.c

@@ -920,18 +920,35 @@ int vcap_set_rule_set_actionset(struct vcap_rule *rule,
 }
 EXPORT_SYMBOL_GPL(vcap_set_rule_set_actionset);
 
-/* Find a rule with a provided rule id */
-static struct vcap_rule_internal *vcap_lookup_rule(struct vcap_control *vctrl,
-						   u32 id)
+/* Check if a rule with this id exists */
+static bool vcap_rule_exists(struct vcap_control *vctrl, u32 id)
 {
 	struct vcap_rule_internal *ri;
 	struct vcap_admin *admin;
 
 	/* Look for the rule id in all vcaps */
 	list_for_each_entry(admin, &vctrl->list, list)
+		list_for_each_entry(ri, &admin->rules, list)
+			if (ri->data.id == id)
+				return true;
+	return false;
+}
+
+/* Find a rule with a provided rule id return a locked vcap */
+static struct vcap_rule_internal *
+vcap_get_locked_rule(struct vcap_control *vctrl, u32 id)
+{
+	struct vcap_rule_internal *ri;
+	struct vcap_admin *admin;
+
+	/* Look for the rule id in all vcaps */
+	list_for_each_entry(admin, &vctrl->list, list) {
+		mutex_lock(&admin->lock);
 		list_for_each_entry(ri, &admin->rules, list)
 			if (ri->data.id == id)
 				return ri;
+		mutex_unlock(&admin->lock);
+	}
 	return NULL;
 }
 
@@ -940,12 +957,21 @@ int vcap_lookup_rule_by_cookie(struct vcap_control *vctrl, u64 cookie)
 {
 	struct vcap_rule_internal *ri;
 	struct vcap_admin *admin;
+	int id = 0;
 
 	/* Look for the rule id in all vcaps */
-	list_for_each_entry(admin, &vctrl->list, list)
-		list_for_each_entry(ri, &admin->rules, list)
-			if (ri->data.cookie == cookie)
-				return ri->data.id;
+	list_for_each_entry(admin, &vctrl->list, list) {
+		mutex_lock(&admin->lock);
+		list_for_each_entry(ri, &admin->rules, list) {
+			if (ri->data.cookie == cookie) {
+				id = ri->data.id;
+				break;
+			}
+		}
+		mutex_unlock(&admin->lock);
+		if (id)
+			return id;
+	}
 	return -ENOENT;
 }
 EXPORT_SYMBOL_GPL(vcap_lookup_rule_by_cookie);
@@ -1866,7 +1892,7 @@ static u32 vcap_set_rule_id(struct vcap_rule_internal *ri)
 		return ri->data.id;
 
 	for (u32 next_id = 1; next_id < ~0; ++next_id) {
-		if (!vcap_lookup_rule(ri->vctrl, next_id)) {
+		if (!vcap_rule_exists(ri->vctrl, next_id)) {
 			ri->data.id = next_id;
 			break;
 		}
@@ -2102,17 +2128,28 @@ struct vcap_rule *vcap_alloc_rule(struct vcap_control *vctrl,
 	/* Sanity check that this VCAP is supported on this platform */
 	if (vctrl->vcaps[admin->vtype].rows == 0)
 		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&admin->lock);
 	/* Check if a rule with this id already exists */
-	if (vcap_lookup_rule(vctrl, id))
-		return ERR_PTR(-EEXIST);
+	if (vcap_rule_exists(vctrl, id)) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
+
 	/* Check if there is room for the rule in the block(s) of the VCAP */
 	maxsize = vctrl->vcaps[admin->vtype].sw_count; /* worst case rule size */
-	if (vcap_rule_space(admin, maxsize))
-		return ERR_PTR(-ENOSPC);
+	if (vcap_rule_space(admin, maxsize)) {
+		err = -ENOSPC;
+		goto out_unlock;
+	}
+
 	/* Create a container for the rule and return it */
 	ri = kzalloc(sizeof(*ri), GFP_KERNEL);
-	if (!ri)
-		return ERR_PTR(-ENOMEM);
+	if (!ri) {
+		err = -ENOMEM;
+		goto out_unlock;
+	}
+
 	ri->data.vcap_chain_id = vcap_chain_id;
 	ri->data.user = user;
 	ri->data.priority = priority;
@@ -2125,13 +2162,21 @@ struct vcap_rule *vcap_alloc_rule(struct vcap_control *vctrl,
 	ri->ndev = ndev;
 	ri->admin = admin; /* refer to the vcap instance */
 	ri->vctrl = vctrl; /* refer to the client */
-	if (vcap_set_rule_id(ri) == 0)
+
+	if (vcap_set_rule_id(ri) == 0) {
+		err = -EINVAL;
 		goto out_free;
+	}
+
+	mutex_unlock(&admin->lock);
 	return (struct vcap_rule *)ri;
 
 out_free:
 	kfree(ri);
-	return ERR_PTR(-EINVAL);
+out_unlock:
+	mutex_unlock(&admin->lock);
+	return ERR_PTR(err);
+
 }
 EXPORT_SYMBOL_GPL(vcap_alloc_rule);
 
@@ -2156,47 +2201,52 @@ void vcap_free_rule(struct vcap_rule *rule)
 }
 EXPORT_SYMBOL_GPL(vcap_free_rule);
 
-struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id)
+/* Decode a rule from the VCAP cache and return a copy */
+struct vcap_rule *vcap_decode_rule(struct vcap_rule_internal *elem)
 {
-	struct vcap_rule_internal *elem;
 	struct vcap_rule_internal *ri;
 	int err;
 
-	ri = NULL;
+	ri = vcap_dup_rule(elem, elem->state == VCAP_RS_DISABLED);
+	if (IS_ERR(ri))
+		return ERR_PTR(PTR_ERR(ri));
+
+	if (ri->state == VCAP_RS_DISABLED)
+		goto out;
+
+	err = vcap_read_rule(ri);
+	if (err)
+		return ERR_PTR(err);
+
+	err = vcap_decode_keyset(ri);
+	if (err)
+		return ERR_PTR(err);
+
+	err = vcap_decode_actionset(ri);
+	if (err)
+		return ERR_PTR(err);
+
+out:
+	return &ri->data;
+}
+
+struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id)
+{
+	struct vcap_rule_internal *elem;
+	struct vcap_rule *rule;
+	int err;
 
 	err = vcap_api_check(vctrl);
 	if (err)
 		return ERR_PTR(err);
-	elem = vcap_lookup_rule(vctrl, id);
+
+	elem = vcap_get_locked_rule(vctrl, id);
 	if (!elem)
 		return NULL;
-	mutex_lock(&elem->admin->lock);
-	ri = vcap_dup_rule(elem, elem->state == VCAP_RS_DISABLED);
-	if (IS_ERR(ri))
-		goto unlock;
 
-	if (ri->state == VCAP_RS_DISABLED)
-		goto unlock;
-
-	err = vcap_read_rule(ri);
-	if (err) {
-		ri = ERR_PTR(err);
-		goto unlock;
-	}
-	err = vcap_decode_keyset(ri);
-	if (err) {
-		ri = ERR_PTR(err);
-		goto unlock;
-	}
-	err = vcap_decode_actionset(ri);
-	if (err) {
-		ri = ERR_PTR(err);
-		goto unlock;
-	}
-
-unlock:
+	rule = vcap_decode_rule(elem);
 	mutex_unlock(&elem->admin->lock);
-	return (struct vcap_rule *)ri;
+	return rule;
 }
 EXPORT_SYMBOL_GPL(vcap_get_rule);
 
@@ -2211,11 +2261,9 @@ int vcap_mod_rule(struct vcap_rule *rule)
 	if (err)
 		return err;
 
-	if (!vcap_lookup_rule(ri->vctrl, ri->data.id))
+	if (!vcap_get_locked_rule(ri->vctrl, ri->data.id))
 		return -ENOENT;
 
-	mutex_lock(&ri->admin->lock);
-
 	vcap_rule_set_state(ri);
 	if (ri->state == VCAP_RS_DISABLED)
 		goto out;
@@ -2232,8 +2280,6 @@ int vcap_mod_rule(struct vcap_rule *rule)
 
 	memset(&ctr, 0, sizeof(ctr));
 	err =  vcap_write_counter(ri, &ctr);
-	if (err)
-		goto out;
 
 out:
 	mutex_unlock(&ri->admin->lock);
@@ -2300,20 +2346,19 @@ int vcap_del_rule(struct vcap_control *vctrl, struct net_device *ndev, u32 id)
 	if (err)
 		return err;
 	/* Look for the rule id in all vcaps */
-	ri = vcap_lookup_rule(vctrl, id);
+	ri = vcap_get_locked_rule(vctrl, id);
 	if (!ri)
-		return -EINVAL;
+		return -ENOENT;
+
 	admin = ri->admin;
 
 	if (ri->addr > admin->last_used_addr)
 		gap = vcap_fill_rule_gap(ri);
 
 	/* Delete the rule from the list of rules and the cache */
-	mutex_lock(&admin->lock);
 	list_del(&ri->list);
 	vctrl->ops->init(ndev, admin, admin->last_used_addr, ri->size + gap);
 	vcap_free_rule(&ri->data);
-	mutex_unlock(&admin->lock);
 
 	/* Update the last used address, set to default when no rules */
 	if (list_empty(&admin->rules)) {
@@ -2323,7 +2368,9 @@ int vcap_del_rule(struct vcap_control *vctrl, struct net_device *ndev, u32 id)
 				       list);
 		admin->last_used_addr = elem->addr;
 	}
-	return 0;
+
+	mutex_unlock(&admin->lock);
+	return err;
 }
 EXPORT_SYMBOL_GPL(vcap_del_rule);
 
@@ -2989,31 +3036,6 @@ void vcap_rule_set_counter_id(struct vcap_rule *rule, u32 counter_id)
 }
 EXPORT_SYMBOL_GPL(vcap_rule_set_counter_id);
 
-/* Provide all rules via a callback interface */
-int vcap_rule_iter(struct vcap_control *vctrl,
-		   int (*callback)(void *, struct vcap_rule *), void *arg)
-{
-	struct vcap_rule_internal *ri;
-	struct vcap_admin *admin;
-	int ret;
-
-	ret = vcap_api_check(vctrl);
-	if (ret)
-		return ret;
-
-	/* Iterate all rules in each VCAP instance */
-	list_for_each_entry(admin, &vctrl->list, list) {
-		list_for_each_entry(ri, &admin->rules, list) {
-			ret = callback(arg, &ri->data);
-			if (ret)
-				return ret;
-		}
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(vcap_rule_iter);
-
 int vcap_rule_set_counter(struct vcap_rule *rule, struct vcap_counter *ctr)
 {
 	struct vcap_rule_internal *ri = to_intrule(rule);
@@ -3026,7 +3048,12 @@ int vcap_rule_set_counter(struct vcap_rule *rule, struct vcap_counter *ctr)
 		pr_err("%s:%d: counter is missing\n", __func__, __LINE__);
 		return -EINVAL;
 	}
-	return vcap_write_counter(ri, ctr);
+
+	mutex_lock(&ri->admin->lock);
+	err = vcap_write_counter(ri, ctr);
+	mutex_unlock(&ri->admin->lock);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(vcap_rule_set_counter);
 
@@ -3042,7 +3069,12 @@ int vcap_rule_get_counter(struct vcap_rule *rule, struct vcap_counter *ctr)
 		pr_err("%s:%d: counter is missing\n", __func__, __LINE__);
 		return -EINVAL;
 	}
-	return vcap_read_counter(ri, ctr);
+
+	mutex_lock(&ri->admin->lock);
+	err = vcap_read_counter(ri, ctr);
+	mutex_unlock(&ri->admin->lock);
+
+	return err;
 }
 EXPORT_SYMBOL_GPL(vcap_rule_get_counter);
 
@@ -3105,6 +3137,48 @@ int vcap_rule_get_keysets(struct vcap_rule_internal *ri,
 	return -EINVAL;
 }
 
+/* Collect packet counts from all rules with the same cookie */
+int vcap_get_rule_count_by_cookie(struct vcap_control *vctrl,
+				  struct vcap_counter *ctr, u64 cookie)
+{
+	struct vcap_rule_internal *ri;
+	struct vcap_counter temp = {};
+	struct vcap_admin *admin;
+	int err;
+
+	err = vcap_api_check(vctrl);
+	if (err)
+		return err;
+
+	/* Iterate all rules in each VCAP instance */
+	list_for_each_entry(admin, &vctrl->list, list) {
+		mutex_lock(&admin->lock);
+		list_for_each_entry(ri, &admin->rules, list) {
+			if (ri->data.cookie != cookie)
+				continue;
+
+			err = vcap_read_counter(ri, &temp);
+			if (err)
+				goto unlock;
+			ctr->value += temp.value;
+
+			/* Reset the rule counter */
+			temp.value = 0;
+			temp.sticky = 0;
+			err = vcap_write_counter(ri, &temp);
+			if (err)
+				goto unlock;
+		}
+		mutex_unlock(&admin->lock);
+	}
+	return err;
+
+unlock:
+	mutex_unlock(&admin->lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(vcap_get_rule_count_by_cookie);
+
 static int vcap_rule_mod_key(struct vcap_rule *rule,
 			     enum vcap_key_field key,
 			     enum vcap_field_type ftype,

drivers/net/ethernet/microchip/vcap/vcap_api_client.h

@@ -202,6 +202,8 @@ int vcap_rule_add_action_u32(struct vcap_rule *rule,
 			     enum vcap_action_field action, u32 value);
 
 /* VCAP rule counter operations */
+int vcap_get_rule_count_by_cookie(struct vcap_control *vctrl,
+				  struct vcap_counter *ctr, u64 cookie);
 int vcap_rule_set_counter(struct vcap_rule *rule, struct vcap_counter *ctr);
 int vcap_rule_get_counter(struct vcap_rule *rule, struct vcap_counter *ctr);
 

drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c

@@ -295,7 +295,7 @@ static int vcap_show_admin(struct vcap_control *vctrl,
 
 	vcap_show_admin_info(vctrl, admin, out);
 	list_for_each_entry(elem, &admin->rules, list) {
-		vrule = vcap_get_rule(vctrl, elem->data.id);
+		vrule = vcap_decode_rule(elem);
 		if (IS_ERR_OR_NULL(vrule)) {
 			ret = PTR_ERR(vrule);
 			break;
@@ -404,8 +404,12 @@ static int vcap_debugfs_show(struct seq_file *m, void *unused)
 		.prf = (void *)seq_printf,
 		.dst = m,
 	};
+	int ret;
 
-	return vcap_show_admin(info->vctrl, info->admin, &out);
+	mutex_lock(&info->admin->lock);
+	ret = vcap_show_admin(info->vctrl, info->admin, &out);
+	mutex_unlock(&info->admin->lock);
+	return ret;
 }
 DEFINE_SHOW_ATTRIBUTE(vcap_debugfs);
 
@@ -417,8 +421,12 @@ static int vcap_raw_debugfs_show(struct seq_file *m, void *unused)
 		.prf = (void *)seq_printf,
 		.dst = m,
 	};
+	int ret;
 
-	return vcap_show_admin_raw(info->vctrl, info->admin, &out);
+	mutex_lock(&info->admin->lock);
+	ret = vcap_show_admin_raw(info->vctrl, info->admin, &out);
+	mutex_unlock(&info->admin->lock);
+	return ret;
 }
 DEFINE_SHOW_ATTRIBUTE(vcap_raw_debugfs);
 

drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c

@@ -246,6 +246,7 @@ static void vcap_test_api_init(struct vcap_admin *admin)
 	INIT_LIST_HEAD(&admin->list);
 	INIT_LIST_HEAD(&admin->rules);
 	INIT_LIST_HEAD(&admin->enabled);
+	mutex_init(&admin->lock);
 	list_add_tail(&admin->list, &test_vctrl.list);
 	memset(test_updateaddr, 0, sizeof(test_updateaddr));
 	test_updateaddridx = 0;

drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c

@@ -236,6 +236,7 @@ static void vcap_test_api_init(struct vcap_admin *admin)
 	INIT_LIST_HEAD(&admin->list);
 	INIT_LIST_HEAD(&admin->rules);
 	INIT_LIST_HEAD(&admin->enabled);
+	mutex_init(&admin->lock);
 	list_add_tail(&admin->list, &test_vctrl.list);
 	memset(test_updateaddr, 0, sizeof(test_updateaddr));
 	test_updateaddridx = 0;

drivers/net/ethernet/microchip/vcap/vcap_api_private.h

@@ -118,4 +118,7 @@ int vcap_find_keystream_keysets(struct vcap_control *vctrl, enum vcap_type vt,
 /* Get the keysets that matches the rule key type/mask */
 int vcap_rule_get_keysets(struct vcap_rule_internal *ri,
 			  struct vcap_keyset_list *matches);
+/* Decode a rule from the VCAP cache and return a copy */
+struct vcap_rule *vcap_decode_rule(struct vcap_rule_internal *elem);
+
 #endif /* __VCAP_API_PRIVATE__ */