Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

net: dsa: fix db type confusion in host fdb/mdb add/del

We have the following code paths:

Host FDB (unicast RX filtering):

dsa_port_standalone_host_fdb_add() dsa_port_bridge_host_fdb_add()
| |
+--------------+ +------------+
| |
v v
dsa_port_host_fdb_add()

dsa_port_standalone_host_fdb_del() dsa_port_bridge_host_fdb_del()
| |
+--------------+ +------------+
| |
v v
dsa_port_host_fdb_del()

Host MDB (multicast RX filtering):

dsa_port_standalone_host_mdb_add() dsa_port_bridge_host_mdb_add()
| |
+--------------+ +------------+
| |
v v
dsa_port_host_mdb_add()

dsa_port_standalone_host_mdb_del() dsa_port_bridge_host_mdb_del()
| |
+--------------+ +------------+
| |
v v
dsa_port_host_mdb_del()

The logic added by commit 5e8a1e03aa4d ("net: dsa: install secondary
unicast and multicast addresses as host FDB/MDB") zeroes out
db.bridge.num if the switch doesn't support ds->fdb_isolation
(the majority doesn't). This is done for a reason explained in commit
c26933639b54 ("net: dsa: request drivers to perform FDB isolation").

Taking a single code path as example - dsa_port_host_fdb_add() - the
others are similar - the problem is that this function handles:
- DSA_DB_PORT databases, when called from
dsa_port_standalone_host_fdb_add()
- DSA_DB_BRIDGE databases, when called from
dsa_port_bridge_host_fdb_add()

So, if dsa_port_host_fdb_add() were to make any change on the
"bridge.num" attribute of the database, this would only be correct for a
DSA_DB_BRIDGE, and a type confusion for a DSA_DB_PORT bridge.

However, this bug is without consequences, for 2 reasons:

- dsa_port_standalone_host_fdb_add() is only called from code which is
(in)directly guarded by dsa_switch_supports_uc_filtering(ds), and that
function only returns true if ds->fdb_isolation is set. So, the code
only executed for DSA_DB_BRIDGE databases.

- Even if the code was not dead for DSA_DB_PORT, we have the following
memory layout:

struct dsa_bridge {
struct net_device *dev;
unsigned int num;
bool tx_fwd_offload;
refcount_t refcount;
};

struct dsa_db {
enum dsa_db_type type;

union {
const struct dsa_port *dp; // DSA_DB_PORT
struct dsa_lag lag;
struct dsa_bridge bridge; // DSA_DB_BRIDGE
};
};

So, the zeroization of dsa_db :: bridge :: num on a dsa_db structure of
type DSA_DB_PORT would access memory which is unused, because we only
use dsa_db :: dp for DSA_DB_PORT, and this is mapped at the same address
with dsa_db :: dev for DSA_DB_BRIDGE, thanks to the union definition.

It is correct to fix up dsa_db :: bridge :: num only from code paths
that come from the bridge / switchdev, so move these there.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Link: https://lore.kernel.org/r/20230329133819.697642-1-vladimir.oltean@nxp.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

authored by

Vladimir Oltean and committed by
Jakub Kicinski
eb1ab765 9a865a98

+12 -12
+12 -12
net/dsa/port.c
··· 1028 1028 .db = db, 1029 1029 }; 1030 1030 1031 - if (!dp->ds->fdb_isolation) 1032 - info.db.bridge.num = 0; 1033 - 1034 1031 return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_ADD, &info); 1035 1032 } 1036 1033 ··· 1051 1054 .bridge = *dp->bridge, 1052 1055 }; 1053 1056 int err; 1057 + 1058 + if (!dp->ds->fdb_isolation) 1059 + db.bridge.num = 0; 1054 1060 1055 1061 /* Avoid a call to __dev_set_promiscuity() on the master, which 1056 1062 * requires rtnl_lock(), since we can't guarantee that is held here, ··· 1079 1079 .db = db, 1080 1080 }; 1081 1081 1082 - if (!dp->ds->fdb_isolation) 1083 - info.db.bridge.num = 0; 1084 - 1085 1082 return dsa_port_notify(dp, DSA_NOTIFIER_HOST_FDB_DEL, &info); 1086 1083 } 1087 1084 ··· 1102 1105 .bridge = *dp->bridge, 1103 1106 }; 1104 1107 int err; 1108 + 1109 + if (!dp->ds->fdb_isolation) 1110 + db.bridge.num = 0; 1105 1111 1106 1112 if (master->priv_flags & IFF_UNICAST_FLT) { 1107 1113 err = dev_uc_del(master, addr); ··· 1210 1210 .db = db, 1211 1211 }; 1212 1212 1213 - if (!dp->ds->fdb_isolation) 1214 - info.db.bridge.num = 0; 1215 - 1216 1213 return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_ADD, &info); 1217 1214 } 1218 1215 ··· 1234 1237 }; 1235 1238 int err; 1236 1239 1240 + if (!dp->ds->fdb_isolation) 1241 + db.bridge.num = 0; 1242 + 1237 1243 err = dev_mc_add(master, mdb->addr); 1238 1244 if (err) 1239 1245 return err; ··· 1253 1253 .mdb = mdb, 1254 1254 .db = db, 1255 1255 }; 1256 - 1257 - if (!dp->ds->fdb_isolation) 1258 - info.db.bridge.num = 0; 1259 1256 1260 1257 return dsa_port_notify(dp, DSA_NOTIFIER_HOST_MDB_DEL, &info); 1261 1258 } ··· 1277 1280 .bridge = *dp->bridge, 1278 1281 }; 1279 1282 int err; 1283 + 1284 + if (!dp->ds->fdb_isolation) 1285 + db.bridge.num = 0; 1280 1286 1281 1287 err = dev_mc_del(master, mdb->addr); 1282 1288 if (err)