net: microchip: vcap: fix races on the shared Super VCAP block

The VCAP instances on a chip are not independent, yet they are locked
independently. On sparx5 and lan969x the IS0 and IS2 instances are
backed by the same Super VCAP hardware block and share its cache and
command registers: every access drives the shared VCAP_SUPER_CTRL
register and moves data through the shared cache registers.

Accessing one instance therefore races with accessing another. The
per-instance admin->lock cannot prevent this, as each instance takes a
different lock.

The locking issue is mostly disguised by the fact that the core usage of
the vcap api runs under rtnl. However, the full rule dump in debugfs
decodes rules straight from hardware (a READ command followed by a cache
read) and runs outside rtnl, so it races a concurrent tc-flower rule
write to another Super VCAP instance.

Besides corrupting the dump, the read repopulates the shared cache
between the writers cache fill and its write command, so the writer
commits the wrong data and corrupts the hardware entry.

Introduce vcap_lock() and vcap_unlock() helpers and route every rule
lock site in the VCAP API and its debugfs code through them. Replace the
per-instance admin->lock with a single mutex in struct vcap_control that
serializes access to all instances. The helpers reach it through a new
admin->vctrl back-pointer, and the clients initialise and destroy the
control lock instead of a per-instance one.

No path holds more than one instance lock, so collapsing them onto a
single mutex cannot self-deadlock.

Fixes: 71c9de995260 ("net: microchip: sparx5: Add VCAP locking to protect rules")
Signed-off-by: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
Link: https://patch.msgid.link/20260630-microchip_fix_vcap_locking-v1-1-f60a4596734d@microchip.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
index 72e3b189bac5ac737fcf2f86346edc221de40dac..eb28df80b2818ef22d4fe136848fb9e981397741 100644 (file)
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
@@ -601,7 +601,6 @@ static void lan966x_vcap_admin_free(struct vcap_admin *admin)
        kfree(admin->cache.keystream);
        kfree(admin->cache.maskstream);
        kfree(admin->cache.actionstream);
-       mutex_destroy(&admin->lock);
        kfree(admin);
 }
 
@@ -615,7 +614,7 @@ lan966x_vcap_admin_alloc(struct lan966x *lan966x, struct vcap_control *ctrl,
        if (!admin)
                return ERR_PTR(-ENOMEM);
 
-       mutex_init(&admin->lock);
+       admin->vctrl = ctrl;
        INIT_LIST_HEAD(&admin->list);
        INIT_LIST_HEAD(&admin->rules);
        INIT_LIST_HEAD(&admin->enabled);
@@ -721,6 +720,7 @@ int lan966x_vcap_init(struct lan966x *lan966x)
        ctrl->ops = &lan966x_vcap_ops;
 
        INIT_LIST_HEAD(&ctrl->list);
+       mutex_init(&ctrl->lock);
        for (int i = 0; i < ARRAY_SIZE(lan966x_vcap_inst_cfg); ++i) {
                cfg = &lan966x_vcap_inst_cfg[i];
 
@@ -780,5 +780,6 @@ void lan966x_vcap_deinit(struct lan966x *lan966x)
                lan966x_vcap_admin_free(admin);
        }
 
+       mutex_destroy(&ctrl->lock);
        kfree(ctrl);
 }

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
index 95b93e46a41dce3f3b1e49535dfab0c346b62d1c..cf332de6bf736cb93bdc1742017ead41fd816898 100644 (file)
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_vcap_impl.c
@@ -1930,7 +1930,6 @@ static void sparx5_vcap_admin_free(struct vcap_admin *admin)
 {
        if (!admin)
                return;
-       mutex_destroy(&admin->lock);
        kfree(admin->cache.keystream);
        kfree(admin->cache.maskstream);
        kfree(admin->cache.actionstream);
@@ -1950,7 +1949,7 @@ sparx5_vcap_admin_alloc(struct sparx5 *sparx5, struct vcap_control *ctrl,
        INIT_LIST_HEAD(&admin->list);
        INIT_LIST_HEAD(&admin->rules);
        INIT_LIST_HEAD(&admin->enabled);
-       mutex_init(&admin->lock);
+       admin->vctrl = ctrl;
        admin->vtype = cfg->vtype;
        admin->vinst = cfg->vinst;
        admin->ingress = cfg->ingress;
@@ -2059,6 +2058,7 @@ int sparx5_vcap_init(struct sparx5 *sparx5)
        ctrl->ops = &sparx5_vcap_ops;
 
        INIT_LIST_HEAD(&ctrl->list);
+       mutex_init(&ctrl->lock);
        for (idx = 0; idx < ARRAY_SIZE(sparx5_vcap_inst_cfg); ++idx) {
                cfg = &consts->vcaps_cfg[idx];
                admin = sparx5_vcap_admin_alloc(sparx5, ctrl, cfg);
@@ -2097,5 +2097,6 @@ void sparx5_vcap_deinit(struct sparx5 *sparx5)
                list_del(&admin->list);
                sparx5_vcap_admin_free(admin);
        }
+       mutex_destroy(&ctrl->lock);
        kfree(ctrl);
 }

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 0fdb5e363badc0e134e404bcabc1bd7847ac651f..ff86cde11a32c7513613d8579a9816f430274292 100644 (file)
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -934,6 +934,16 @@ static bool vcap_rule_exists(struct vcap_control *vctrl, u32 id)
        return false;
 }
 
+void vcap_lock(struct vcap_admin *admin)
+{
+       mutex_lock(&admin->vctrl->lock);
+}
+
+void vcap_unlock(struct vcap_admin *admin)
+{
+       mutex_unlock(&admin->vctrl->lock);
+}
+
 /* 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)
@@ -943,11 +953,11 @@ vcap_get_locked_rule(struct vcap_control *vctrl, u32 id)
 
        /* Look for the rule id in all vcaps */
        list_for_each_entry(admin, &vctrl->list, list) {
-               mutex_lock(&admin->lock);
+               vcap_lock(admin);
                list_for_each_entry(ri, &admin->rules, list)
                        if (ri->data.id == id)
                                return ri;
-               mutex_unlock(&admin->lock);
+               vcap_unlock(admin);
        }
        return NULL;
 }
@@ -961,14 +971,14 @@ int vcap_lookup_rule_by_cookie(struct vcap_control *vctrl, u64 cookie)
 
        /* Look for the rule id in all vcaps */
        list_for_each_entry(admin, &vctrl->list, list) {
-               mutex_lock(&admin->lock);
+               vcap_lock(admin);
                list_for_each_entry(ri, &admin->rules, list) {
                        if (ri->data.cookie == cookie) {
                                id = ri->data.id;
                                break;
                        }
                }
-               mutex_unlock(&admin->lock);
+               vcap_unlock(admin);
                if (id)
                        return id;
        }
@@ -985,11 +995,11 @@ int vcap_admin_rule_count(struct vcap_admin *admin, int cid)
        int count = 0;
 
        list_for_each_entry(elem, &admin->rules, list) {
-               mutex_lock(&admin->lock);
+               vcap_lock(admin);
                if (elem->data.vcap_chain_id >= min_cid &&
                    elem->data.vcap_chain_id < max_cid)
                        ++count;
-               mutex_unlock(&admin->lock);
+               vcap_unlock(admin);
        }
        return count;
 }
@@ -2266,7 +2276,7 @@ int vcap_add_rule(struct vcap_rule *rule)
        if (ret)
                return ret;
        /* Insert the new rule in the list of vcap rules */
-       mutex_lock(&ri->admin->lock);
+       vcap_lock(ri->admin);
 
        vcap_rule_set_state(ri);
        ret = vcap_insert_rule(ri, &move);
@@ -2302,7 +2312,7 @@ int vcap_add_rule(struct vcap_rule *rule)
                goto out;
        }
 out:
-       mutex_unlock(&ri->admin->lock);
+       vcap_unlock(ri->admin);
        return ret;
 }
 EXPORT_SYMBOL_GPL(vcap_add_rule);
@@ -2330,7 +2340,7 @@ struct vcap_rule *vcap_alloc_rule(struct vcap_control *vctrl,
        if (vctrl->vcaps[admin->vtype].rows == 0)
                return ERR_PTR(-EINVAL);
 
-       mutex_lock(&admin->lock);
+       vcap_lock(admin);
        /* Check if a rule with this id already exists */
        if (vcap_rule_exists(vctrl, id)) {
                err = -EINVAL;
@@ -2369,13 +2379,13 @@ struct vcap_rule *vcap_alloc_rule(struct vcap_control *vctrl,
                goto out_free;
        }
 
-       mutex_unlock(&admin->lock);
+       vcap_unlock(admin);
        return (struct vcap_rule *)ri;
 
 out_free:
        kfree(ri);
 out_unlock:
-       mutex_unlock(&admin->lock);
+       vcap_unlock(admin);
        return ERR_PTR(err);
 
 }
@@ -2446,7 +2456,7 @@ struct vcap_rule *vcap_get_rule(struct vcap_control *vctrl, u32 id)
                return ERR_PTR(-ENOENT);
 
        rule = vcap_decode_rule(elem);
-       mutex_unlock(&elem->admin->lock);
+       vcap_unlock(elem->admin);
        return rule;
 }
 EXPORT_SYMBOL_GPL(vcap_get_rule);
@@ -2483,7 +2493,7 @@ int vcap_mod_rule(struct vcap_rule *rule)
        err =  vcap_write_counter(ri, &ctr);
 
 out:
-       mutex_unlock(&ri->admin->lock);
+       vcap_unlock(ri->admin);
        return err;
 }
 EXPORT_SYMBOL_GPL(vcap_mod_rule);
@@ -2570,7 +2580,7 @@ int vcap_del_rule(struct vcap_control *vctrl, struct net_device *ndev, u32 id)
                admin->last_used_addr = elem->addr;
        }
 
-       mutex_unlock(&admin->lock);
+       vcap_unlock(admin);
        return err;
 }
 EXPORT_SYMBOL_GPL(vcap_del_rule);
@@ -2585,7 +2595,7 @@ int vcap_del_rules(struct vcap_control *vctrl, struct vcap_admin *admin)
        if (ret)
                return ret;
 
-       mutex_lock(&admin->lock);
+       vcap_lock(admin);
        list_for_each_entry_safe(ri, next_ri, &admin->rules, list) {
                vctrl->ops->init(ri->ndev, admin, ri->addr, ri->size);
                list_del(&ri->list);
@@ -2598,7 +2608,7 @@ int vcap_del_rules(struct vcap_control *vctrl, struct vcap_admin *admin)
                list_del(&eport->list);
                kfree(eport);
        }
-       mutex_unlock(&admin->lock);
+       vcap_unlock(admin);
 
        return 0;
 }
@@ -3016,7 +3026,7 @@ static int vcap_enable_rules(struct vcap_control *vctrl,
                        continue;
 
                /* Found the admin, now find the offloadable rules */
-               mutex_lock(&admin->lock);
+               vcap_lock(admin);
                list_for_each_entry(ri, &admin->rules, list) {
                        /* Is the rule in the lookup defined by the chain */
                        if (!(ri->data.vcap_chain_id >= chain &&
@@ -3034,7 +3044,7 @@ static int vcap_enable_rules(struct vcap_control *vctrl,
                        if (err)
                                break;
                }
-               mutex_unlock(&admin->lock);
+               vcap_unlock(admin);
                if (err)
                        break;
        }
@@ -3074,7 +3084,7 @@ static int vcap_disable_rules(struct vcap_control *vctrl,
                        continue;
 
                /* Found the admin, now find the rules on the chain */
-               mutex_lock(&admin->lock);
+               vcap_lock(admin);
                list_for_each_entry(ri, &admin->rules, list) {
                        if (ri->data.vcap_chain_id != chain)
                                continue;
@@ -3089,7 +3099,7 @@ static int vcap_disable_rules(struct vcap_control *vctrl,
                        if (err)
                                break;
                }
-               mutex_unlock(&admin->lock);
+               vcap_unlock(admin);
                if (err)
                        break;
        }
@@ -3133,9 +3143,9 @@ static int vcap_enable(struct vcap_control *vctrl, struct net_device *ndev,
        eport->cookie = cookie;
        eport->src_cid = src_cid;
        eport->dst_cid = dst_cid;
-       mutex_lock(&admin->lock);
+       vcap_lock(admin);
        list_add_tail(&eport->list, &admin->enabled);
-       mutex_unlock(&admin->lock);
+       vcap_unlock(admin);
 
        if (vcap_path_exist(vctrl, ndev, src_cid)) {
                /* Enable chained lookups */
@@ -3185,9 +3195,9 @@ static int vcap_disable(struct vcap_control *vctrl, struct net_device *ndev,
                dst_cid = vcap_get_next_chain(vctrl, ndev, dst_cid);
        }
 
-       mutex_lock(&found->lock);
+       vcap_lock(found);
        list_del(&eport->list);
-       mutex_unlock(&found->lock);
+       vcap_unlock(found);
        kfree(eport);
        return 0;
 }
@@ -3270,9 +3280,9 @@ int vcap_rule_set_counter(struct vcap_rule *rule, struct vcap_counter *ctr)
                return -EINVAL;
        }
 
-       mutex_lock(&ri->admin->lock);
+       vcap_lock(ri->admin);
        err = vcap_write_counter(ri, ctr);
-       mutex_unlock(&ri->admin->lock);
+       vcap_unlock(ri->admin);
 
        return err;
 }
@@ -3291,9 +3301,9 @@ int vcap_rule_get_counter(struct vcap_rule *rule, struct vcap_counter *ctr)
                return -EINVAL;
        }
 
-       mutex_lock(&ri->admin->lock);
+       vcap_lock(ri->admin);
        err = vcap_read_counter(ri, ctr);
-       mutex_unlock(&ri->admin->lock);
+       vcap_unlock(ri->admin);
 
        return err;
 }
@@ -3395,7 +3405,7 @@ int vcap_get_rule_count_by_cookie(struct vcap_control *vctrl,
 
        /* Iterate all rules in each VCAP instance */
        list_for_each_entry(admin, &vctrl->list, list) {
-               mutex_lock(&admin->lock);
+               vcap_lock(admin);
                list_for_each_entry(ri, &admin->rules, list) {
                        if (ri->data.cookie != cookie)
                                continue;
@@ -3412,12 +3422,12 @@ int vcap_get_rule_count_by_cookie(struct vcap_control *vctrl,
                        if (err)
                                goto unlock;
                }
-               mutex_unlock(&admin->lock);
+               vcap_unlock(admin);
        }
        return err;
 
 unlock:
-       mutex_unlock(&admin->lock);
+       vcap_unlock(admin);
        return err;
 }
 EXPORT_SYMBOL_GPL(vcap_get_rule_count_by_cookie);

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.h b/drivers/net/ethernet/microchip/vcap/vcap_api.h
index 6069ad95c27e93b60ae406da9834dba4767ac419..05b4b02e59ef5fd616c05ddfbd7d6e808ea1560c 100644 (file)
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.h
@@ -164,7 +164,7 @@ struct vcap_admin {
        struct list_head list; /* for insertion in vcap_control */
        struct list_head rules; /* list of rules */
        struct list_head enabled; /* list of enabled ports */
-       struct mutex lock; /* control access to rules */
+       struct vcap_control *vctrl; /* the control instance owning this vcap */
        enum vcap_type vtype;  /* type of vcap */
        int vinst; /* instance number within the same type */
        int first_cid; /* first chain id in this vcap */
@@ -275,6 +275,7 @@ struct vcap_control {
        const struct vcap_info *vcaps; /* client supplied vcap models */
        const struct vcap_statistics *stats; /* client supplied vcap stats */
        struct list_head list; /* list of vcap instances */
+       struct mutex lock; /* serialize access to all vcap instances */
 };
 
 #endif /* __VCAP_API__ */

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
index 59bfbda29bb3dab1f45098134ffc5ee662c01ce0..e0c65c7ab23e1e18089006d2c6093320006570de 100644 (file)
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs.c
@@ -410,9 +410,9 @@ static int vcap_debugfs_show(struct seq_file *m, void *unused)
        };
        int ret;
 
-       mutex_lock(&info->admin->lock);
+       vcap_lock(info->admin);
        ret = vcap_show_admin(info->vctrl, info->admin, &out);
-       mutex_unlock(&info->admin->lock);
+       vcap_unlock(info->admin);
        return ret;
 }
 DEFINE_SHOW_ATTRIBUTE(vcap_debugfs);
@@ -427,9 +427,9 @@ static int vcap_raw_debugfs_show(struct seq_file *m, void *unused)
        };
        int ret;
 
-       mutex_lock(&info->admin->lock);
+       vcap_lock(info->admin);
        ret = vcap_show_admin_raw(info->vctrl, info->admin, &out);
-       mutex_unlock(&info->admin->lock);
+       vcap_unlock(info->admin);
        return ret;
 }
 DEFINE_SHOW_ATTRIBUTE(vcap_raw_debugfs);

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
index 9c9d380421255d62dfb2d7a272c5cff6fb2f92ac..ac2a3b8c4f3217efeef3caba5140c200cf7df4bc 100644 (file)
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_debugfs_kunit.c
@@ -243,10 +243,11 @@ static void vcap_test_api_init(struct vcap_admin *admin)
 {
        /* Initialize the shared objects */
        INIT_LIST_HEAD(&test_vctrl.list);
+       mutex_init(&test_vctrl.lock);
        INIT_LIST_HEAD(&admin->list);
        INIT_LIST_HEAD(&admin->rules);
        INIT_LIST_HEAD(&admin->enabled);
-       mutex_init(&admin->lock);
+       admin->vctrl = &test_vctrl;
        list_add_tail(&admin->list, &test_vctrl.list);
        memset(test_updateaddr, 0, sizeof(test_updateaddr));
        test_updateaddridx = 0;

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
index ce26ccbdccdf93e079c7e857227c61454e7efcdf..83de384d3e3bd48b84ed59f2508db323ca12aa58 100644 (file)
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_kunit.c
@@ -233,10 +233,11 @@ static void vcap_test_api_init(struct vcap_admin *admin)
 {
        /* Initialize the shared objects */
        INIT_LIST_HEAD(&test_vctrl.list);
+       mutex_init(&test_vctrl.lock);
        INIT_LIST_HEAD(&admin->list);
        INIT_LIST_HEAD(&admin->rules);
        INIT_LIST_HEAD(&admin->enabled);
-       mutex_init(&admin->lock);
+       admin->vctrl = &test_vctrl;
        list_add_tail(&admin->list, &test_vctrl.list);
        memset(test_updateaddr, 0, sizeof(test_updateaddr));
        test_updateaddridx = 0;

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api_private.h b/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
index 844bdf6b5f4502edee09890d21db9d82dc8caeb0..b4057fbe3d188c66e4179ddb6a9929d6ad9af4e9 100644 (file)
--- a/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api_private.h
@@ -50,6 +50,9 @@ struct vcap_stream_iter {
 
 /* Check that the control has a valid set of callbacks */
 int vcap_api_check(struct vcap_control *ctrl);
+/* Serialize access to the vcap instances of a control */
+void vcap_lock(struct vcap_admin *admin);
+void vcap_unlock(struct vcap_admin *admin);
 /* Erase the VCAP cache area used or encoding and decoding */
 void vcap_erase_cache(struct vcap_rule_internal *ri);