net: mscc: ocelot: mark traps with a bool instead of keeping them in a list

Since the blamed commit, VCAP filters can appear on more than one list.
If their action is "trap", they are chained on ocelot->traps via
filter->trap_list. This is in addition to their normal placement on the
VCAP block->rules list head.

Therefore, when we free a VCAP filter, we must remove it from all lists
it is a member of, including ocelot->traps.

There are at least 2 bugs which are direct consequences of this design
decision.

First is the incorrect usage of list_empty(), meant to denote whether
"filter" is chained into ocelot->traps via filter->trap_list.
This does not do the correct thing, because list_empty() checks whether
"head->next == head", but in our case, head->next == head->prev == NULL.
So we dereference NULL pointers and die when we call list_del().

Second is the fact that not all places that should remove the filter
from ocelot->traps do so. One example is ocelot_vcap_block_remove_filter(),
which is where we have the main kfree(filter). By keeping freed filters
in ocelot->traps we end up in a use-after-free in
felix_update_trapping_destinations().

Attempting to fix all the buggy patterns is a whack-a-mole game which
makes the driver unmaintainable. Actually this is what the previous
patch version attempted to do:
https://patchwork.kernel.org/project/netdevbpf/patch/20220503115728.834457-3-vladimir.oltean@nxp.com/

but it introduced another set of bugs, because there are other places in
which create VCAP filters, not just ocelot_vcap_filter_create():

- ocelot_trap_add()
- felix_tag_8021q_vlan_add_rx()
- felix_tag_8021q_vlan_add_tx()

Relying on the convention that all those code paths must call
INIT_LIST_HEAD(&filter->trap_list) is not going to scale.

So let's do what should have been done in the first place and keep a
bool in struct ocelot_vcap_filter which denotes whether we are looking
at a trapping rule or not. Iterating now happens over the main VCAP IS2
block->rules. The advantage is that we no longer risk having stale
references to a freed filter, since it is only present in that list.

Fixes: e42bd4ed09aa ("net: mscc: ocelot: keep traps in a list")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 9e28219..faccfb3 100644 (file)
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -403,6 +403,7 @@ static int felix_update_trapping_destinations(struct dsa_switch *ds,
 {
        struct ocelot *ocelot = ds->priv;
        struct felix *felix = ocelot_to_felix(ocelot);
+       struct ocelot_vcap_block *block_vcap_is2;
        struct ocelot_vcap_filter *trap;
        enum ocelot_mask_mode mask_mode;
        unsigned long port_mask;
@@ -422,9 +423,13 @@ static int felix_update_trapping_destinations(struct dsa_switch *ds,
        /* We are sure that "cpu" was found, otherwise
         * dsa_tree_setup_default_cpu() would have failed earlier.
         */
+       block_vcap_is2 = &ocelot->block[VCAP_IS2];
 
        /* Make sure all traps are set up for that destination */
-       list_for_each_entry(trap, &ocelot->traps, trap_list) {
+       list_for_each_entry(trap, &block_vcap_is2->rules, list) {
+               if (!trap->is_trap)
+                       continue;
+
                /* Figure out the current trapping destination */
                if (using_tag_8021q) {
                        /* Redirect to the tag_8021q CPU port. If timestamps

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index ca71b62..20ceac8 100644 (file)
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1622,7 +1622,7 @@ int ocelot_trap_add(struct ocelot *ocelot, int port,
                trap->action.mask_mode = OCELOT_MASK_MODE_PERMIT_DENY;
                trap->action.port_mask = 0;
                trap->take_ts = take_ts;
-               list_add_tail(&trap->trap_list, &ocelot->traps);
+               trap->is_trap = true;
                new = true;
        }
 
@@ -1634,10 +1634,8 @@ int ocelot_trap_add(struct ocelot *ocelot, int port,
                err = ocelot_vcap_filter_replace(ocelot, trap);
        if (err) {
                trap->ingress_port_mask &= ~BIT(port);
-               if (!trap->ingress_port_mask) {
-                       list_del(&trap->trap_list);
+               if (!trap->ingress_port_mask)
                        kfree(trap);
-               }
                return err;
        }
 
@@ -1657,11 +1655,8 @@ int ocelot_trap_del(struct ocelot *ocelot, int port, unsigned long cookie)
                return 0;
 
        trap->ingress_port_mask &= ~BIT(port);
-       if (!trap->ingress_port_mask) {
-               list_del(&trap->trap_list);
-
+       if (!trap->ingress_port_mask)
                return ocelot_vcap_filter_del(ocelot, trap);
-       }
 
        return ocelot_vcap_filter_replace(ocelot, trap);
 }

diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 03b5e59..a9b26b3 100644 (file)
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -295,7 +295,7 @@ static int ocelot_flower_parse_action(struct ocelot *ocelot, int port,
                        filter->action.cpu_copy_ena = true;
                        filter->action.cpu_qu_num = 0;
                        filter->type = OCELOT_VCAP_FILTER_OFFLOAD;
-                       list_add_tail(&filter->trap_list, &ocelot->traps);
+                       filter->is_trap = true;
                        break;
                case FLOW_ACTION_POLICE:
                        if (filter->block_id == PSFP_BLOCK_ID) {
@@ -878,8 +878,6 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port,
 
        ret = ocelot_flower_parse(ocelot, port, ingress, f, filter);
        if (ret) {
-               if (!list_empty(&filter->trap_list))
-                       list_del(&filter->trap_list);
                kfree(filter);
                return ret;
        }

diff --git a/include/soc/mscc/ocelot_vcap.h b/include/soc/mscc/ocelot_vcap.h
index 7b2bf9b..de26c99 100644 (file)
--- a/include/soc/mscc/ocelot_vcap.h
+++ b/include/soc/mscc/ocelot_vcap.h
@@ -681,7 +681,6 @@ struct ocelot_vcap_id {
 
 struct ocelot_vcap_filter {
        struct list_head list;
-       struct list_head trap_list;
 
        enum ocelot_vcap_filter_type type;
        int block_id;
@@ -695,6 +694,7 @@ struct ocelot_vcap_filter {
        struct ocelot_vcap_stats stats;
        /* For VCAP IS1 and IS2 */
        bool take_ts;
+       bool is_trap;
        unsigned long ingress_port_mask;
        /* For VCAP ES0 */
        struct ocelot_vcap_port ingress_port;