From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:29
As mentioned in the RFC posted 2 months ago:
https://patchwork.kernel.org/project/netdevbpf/cover/20210824114049.3814660-1-vladimir.oltean@nxp.com/
DSA is transitioning to a driver API where the rtnl_lock is not held
when calling ds->ops->port_fdb_add() and ds->ops->port_fdb_del().
Drivers cannot take that lock privately from those callbacks either.
This change is required so that DSA can wait for switchdev FDB work
items to finish before leaving the bridge. That change will be made in a
future patch series.
A small selftest is provided with the patch set in the hope that
concurrency issues uncovered by this series, but not spotted by me by
code inspection, will be caught.
A status of the existing drivers:
- mv88e6xxx_port_fdb_add() and mv88e6xxx_port_fdb_del() take
mv88e6xxx_reg_lock() so they should be safe.
- qca8k_fdb_add() and qca8k_fdb_del() take mutex_lock(&priv->reg_mutex)
so they should be safe.
- hellcreek_fdb_add() and hellcreek_fdb_add() take mutex_lock(&hellcreek->reg_lock)
so they should be safe.
- ksz9477_port_fdb_add() and ksz9477_port_fdb_del() take mutex_lock(&dev->alu_mutex)
so they should be safe.
- b53_fdb_add() and b53_fdb_del() did not have locking, so I've added a
scheme based on my own judgement there (not tested).
- felix_fdb_add() and felix_fdb_del() did not have locking, I've added
and tested a locking scheme there.
- mt7530_port_fdb_add() and mt7530_port_fdb_del() take
mutex_lock(&priv->reg_mutex), so they should be safe.
- gswip_port_fdb() did not have locking, so I've added a non-expert
locking scheme based on my own judgement (not tested).
- lan9303_alr_add_port() and lan9303_alr_del_port() take
mutex_lock(&chip->alr_mutex) so they should be safe.
- sja1105_fdb_add() and sja1105_fdb_del() did not have locking, I've
added and tested a locking scheme.
Changes in v3:
Unlock arl_mutex only once in b53_fdb_dump().
Changes in v4:
- Use __must_hold in ocelot and b53
- Add missing mutex_init in lantiq_gswip
- Clean up the selftest a bit.
Changes in v5:
- Replace __must_hold with a comment.
- Add a new patch (01/10).
Vladimir Oltean (10):
net: dsa: avoid refcount warnings when ->port_{fdb,mdb}_del returns
error
net: dsa: sja1105: wait for dynamic config command completion on
writes too
net: dsa: sja1105: serialize access to the dynamic config interface
net: mscc: ocelot: serialize access to the MAC table
net: dsa: b53: serialize access to the ARL table
net: dsa: lantiq_gswip: serialize access to the PCE registers
net: dsa: introduce locking for the address lists on CPU and DSA ports
net: dsa: drop rtnl_lock from dsa_slave_switchdev_event_work
selftests: lib: forwarding: allow tests to not require mz and jq
selftests: net: dsa: add a stress test for unlocked FDB operations
MAINTAINERS | 1 +
drivers/net/dsa/b53/b53_common.c | 36 ++++++--
drivers/net/dsa/b53/b53_priv.h | 1 +
drivers/net/dsa/lantiq_gswip.c | 28 +++++-
drivers/net/dsa/sja1105/sja1105.h | 2 +
.../net/dsa/sja1105/sja1105_dynamic_config.c | 91 ++++++++++++++-----
drivers/net/dsa/sja1105/sja1105_main.c | 1 +
drivers/net/ethernet/mscc/ocelot.c | 53 ++++++++---
include/net/dsa.h | 1 +
include/soc/mscc/ocelot.h | 3 +
net/dsa/dsa2.c | 1 +
net/dsa/slave.c | 2 -
net/dsa/switch.c | 80 ++++++++++------
.../drivers/net/dsa/test_bridge_fdb_stress.sh | 47 ++++++++++
tools/testing/selftests/net/forwarding/lib.sh | 10 +-
15 files changed, 280 insertions(+), 77 deletions(-)
create mode 100755 tools/testing/selftests/drivers/net/dsa/test_bridge_fdb_stress.sh
--
2.25.1
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:33
At present, when either of ds->ops->port_fdb_del() or ds->ops->port_mdb_del()
return a non-zero error code, we attempt to save the day and keep the
data structure associated with that switchdev object, as the deletion
procedure did not complete.
However, the way in which we do this is suspicious to the checker in
lib/refcount.c, who thinks it is buggy to increment a refcount that
became zero, and that this is indicative of a use-after-free.
Fixes: 161ca59d39e9 ("net: dsa: reference count the MDB entries at the cross-chip notifier level")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v4->v5: patch is new.
This is a minor fix and slightly unrelated to the series, but my other
patches touch this area and there isn't enough time until the merge
window to wait for another net -> net-next merge. I've annotated it with
a Fixes: tag and sent it to net-next (also made it the first patch), in
the idea that if it could be picked up by AUTOSEL it would be nice, and
if not, the problem it fixes isn't so critical anyway.
net/dsa/switch.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:38
The hardware manual says that software should attempt a new dynamic
config access (be it a a write or a read-back) only while the VALID bit
is cleared. The VALID bit is set by software to 1, and it remains set as
long as the hardware is still processing the request.
Currently the driver only polls for the command completion only for
reads, because that's when we need the actual data read back. Writes
have been more or less "asynchronous", although this has never been an
observable issue.
This change makes sja1105_dynamic_config_write poll the VALID bit as
well, to absolutely ensure that a follow-up access to the static config
finds the VALID bit cleared.
So VALID means "work in progress", while VALIDENT means "entry being
read is valid". On reads we check the VALIDENT bit too, while on writes
that bit is not always defined. So we need to factor it out of the loop,
and make the loop provide back the unpacked command structure, so that
sja1105_dynamic_config_read can check the VALIDENT bit.
The change also attempts to convert the open-coded loop to use the
read_poll_timeout macro, since I know this will come up during review.
It's more code, but hey, it uses read_poll_timeout!
Tested on SJA1105T, SJA1105S, SJA1110A.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v5: none
.../net/dsa/sja1105/sja1105_dynamic_config.c | 81 ++++++++++++++-----
1 file changed, 59 insertions(+), 22 deletions(-)
@@ -1170,6 +1170,56 @@ const struct sja1105_dynamic_table_ops sja1110_dyn_ops[BLK_IDX_MAX_DYN] = {},};+#define SJA1105_DYNAMIC_CONFIG_SLEEP_US 10+#define SJA1105_DYNAMIC_CONFIG_TIMEOUT_US 100000++staticint+sja1105_dynamic_config_poll_valid(structsja1105_private*priv,+structsja1105_dyn_cmd*cmd,+conststructsja1105_dynamic_table_ops*ops)+{+u8packed_buf[SJA1105_MAX_DYN_CMD_SIZE]={};+intrc;++/* We don't _need_ to read the full entry, just the command area which+*isafixedSJA1105_SIZE_DYN_CMD.Butourcmd_packing()APIexpectsa+*bufferthatcontainsthefullentrytoo.Additionally,ourAPI+*doesn'treallyknowhowmanybytesintothebufferdoesthecommand+*areareallybegin.Sojustreadbackthewholeentry.+*/+rc=sja1105_xfer_buf(priv,SPI_READ,ops->addr,packed_buf,+ops->packed_size);+if(rc)+returnrc;++/* Unpack the command structure, and return it to the caller in case it+*needstoperformfurtherchecksonit(VALIDENT).+*/+memset(cmd,0,sizeof(*cmd));+ops->cmd_packing(packed_buf,cmd,UNPACK);++/* Hardware hasn't cleared VALID => still working on it */+returncmd->valid?-EAGAIN:0;+}++/* Poll the dynamic config entry's control area until the hardware has+*clearedtheVALIDbit,whichmeanswehaveconfirmationthatithas+*finishedprocessingthecommand.+*/+staticint+sja1105_dynamic_config_wait_complete(structsja1105_private*priv,+structsja1105_dyn_cmd*cmd,+conststructsja1105_dynamic_table_ops*ops)+{+intrc;++returnread_poll_timeout(sja1105_dynamic_config_poll_valid,+rc,rc!=-EAGAIN,+SJA1105_DYNAMIC_CONFIG_SLEEP_US,+SJA1105_DYNAMIC_CONFIG_TIMEOUT_US,+false,priv,cmd,ops);+}+/* Provides read access to the settings through the dynamic interface*oftheswitch.*@blk_idxisusedaskeytoselectfromthesja1105_dynamic_table_ops.
@@ -1239,28 +1288,12 @@ int sja1105_dynamic_config_read(struct sja1105_private *priv,if(rc<0)returnrc;-/* Loop until we have confirmation that hardware has finished-*processingthecommandandhasclearedtheVALIDfield-*/-do{-memset(packed_buf,0,ops->packed_size);--/* Retrieve the read operation's result */-rc=sja1105_xfer_buf(priv,SPI_READ,ops->addr,packed_buf,-ops->packed_size);-if(rc<0)-returnrc;--cmd=(structsja1105_dyn_cmd){0};-ops->cmd_packing(packed_buf,&cmd,UNPACK);--if(!cmd.valident&&!(ops->access&OP_VALID_ANYWAY))-return-ENOENT;-cpu_relax();-}while(cmd.valid&&--retries);+rc=sja1105_dynamic_config_wait_complete(priv,&cmd,ops);+if(rc<0)+returnrc;-if(cmd.valid)-return-ETIMEDOUT;+if(!cmd.valident&&!(ops->access&OP_VALID_ANYWAY))+return-ENOENT;/* Don't dereference possibly NULL pointer - maybe caller*onlywantedtoseewhethertheentryexistedornot.
@@ -1321,6 +1354,10 @@ int sja1105_dynamic_config_write(struct sja1105_private *priv,if(rc<0)returnrc;+rc=sja1105_dynamic_config_wait_complete(priv,&cmd,ops);+if(rc<0)+returnrc;+cmd=(structsja1105_dyn_cmd){0};ops->cmd_packing(packed_buf,&cmd,UNPACK);if(cmd.errors)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:40
The sja1105 hardware seems as concurrent as can be, but when we create a
background script that adds/removes a rain of FDB entries without the
rtnl_mutex taken, then in parallel we do another operation like run
'bridge fdb show', we can notice these errors popping up:
sja1105 spi2.0: port 2 failed to read back entry for 00:01:02:03:00:40 vid 0: -ENOENT
sja1105 spi2.0: port 2 failed to add 00:01:02:03:00:40 vid 0 to fdb: -2
sja1105 spi2.0: port 2 failed to read back entry for 00:01:02:03:00:46 vid 0: -ENOENT
sja1105 spi2.0: port 2 failed to add 00:01:02:03:00:46 vid 0 to fdb: -2
Luckily what is going on does not require a major rework in the driver.
The sja1105_dynamic_config_read() function sends multiple SPI buffers to
the peripheral until the operation completes. We should not do anything
until the hardware clears the VALID bit.
But since there is no locking (i.e. right now we are implicitly
serialized by the rtnl_mutex, but if we remove that), it might be
possible that the process which performs the dynamic config read is
preempted and another one performs a dynamic config write.
What will happen in that case is that sja1105_dynamic_config_read(),
when it resumes, expects to see VALIDENT set for the entry it reads
back. But it won't.
This can be corrected by introducing a mutex for serializing SPI
accesses to the dynamic config interface which should be atomic with
respect to each other.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v1->v5: none
drivers/net/dsa/sja1105/sja1105.h | 2 ++
drivers/net/dsa/sja1105/sja1105_dynamic_config.c | 12 ++++++++++--
drivers/net/dsa/sja1105/sja1105_main.c | 1 +
3 files changed, 13 insertions(+), 2 deletions(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:42
DSA would like to remove the rtnl_lock from its
SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE handlers, and the felix driver uses
the same MAC table functions as ocelot.
This means that the MAC table functions will no longer be implicitly
serialized with respect to each other by the rtnl_mutex, we need to add
a dedicated lock in ocelot for the non-atomic operations of selecting a
MAC table row, reading/writing what we want and polling for completion.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v3->v4: use __must_hold
v4->v5: revert changes made in v4
drivers/net/ethernet/mscc/ocelot.c | 53 +++++++++++++++++++++++-------
include/soc/mscc/ocelot.h | 3 ++
2 files changed, 44 insertions(+), 12 deletions(-)
@@ -20,11 +20,13 @@ struct ocelot_mact_entry {enummacaccess_entry_typetype;};+/* Caller must hold &ocelot->mact_lock */staticinlineu32ocelot_mact_read_macaccess(structocelot*ocelot){returnocelot_read(ocelot,ANA_TABLES_MACACCESS);}+/* Caller must hold &ocelot->mact_lock */staticinlineintocelot_mact_wait_for_completion(structocelot*ocelot){u32val;
@@ -36,6 +38,7 @@ static inline int ocelot_mact_wait_for_completion(struct ocelot *ocelot)TABLE_UPDATE_SLEEP_US,TABLE_UPDATE_TIMEOUT_US);}+/* Caller must hold &ocelot->mact_lock */staticvoidocelot_mact_select(structocelot*ocelot,constunsignedcharmac[ETH_ALEN],unsignedintvid)
@@ -67,6 +70,7 @@ int ocelot_mact_learn(struct ocelot *ocelot, int port,ANA_TABLES_MACACCESS_ENTRYTYPE(type)|ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_LEARN);unsignedintmc_ports;+interr;/* Set MAC_CPU_COPY if the CPU port is used by a multicast entry */if(type==ENTRYTYPE_MACv4)
@@ -79,18 +83,28 @@ int ocelot_mact_learn(struct ocelot *ocelot, int port,if(mc_ports&BIT(ocelot->num_phys_ports))cmd|=ANA_TABLES_MACACCESS_MAC_CPU_COPY;+mutex_lock(&ocelot->mact_lock);+ocelot_mact_select(ocelot,mac,vid);/* Issue a write command */ocelot_write(ocelot,cmd,ANA_TABLES_MACACCESS);-returnocelot_mact_wait_for_completion(ocelot);+err=ocelot_mact_wait_for_completion(ocelot);++mutex_unlock(&ocelot->mact_lock);++returnerr;}EXPORT_SYMBOL(ocelot_mact_learn);intocelot_mact_forget(structocelot*ocelot,constunsignedcharmac[ETH_ALEN],unsignedintvid){+interr;++mutex_lock(&ocelot->mact_lock);+ocelot_mact_select(ocelot,mac,vid);/* Issue a forget command */
@@ -98,7 +112,11 @@ int ocelot_mact_forget(struct ocelot *ocelot,ANA_TABLES_MACACCESS_MAC_TABLE_CMD(MACACCESS_CMD_FORGET),ANA_TABLES_MACACCESS);-returnocelot_mact_wait_for_completion(ocelot);+err=ocelot_mact_wait_for_completion(ocelot);++mutex_unlock(&ocelot->mact_lock);++returnerr;}EXPORT_SYMBOL(ocelot_mact_forget);
@@ -114,7 +132,9 @@ static void ocelot_mact_init(struct ocelot *ocelot)|ANA_AGENCTRL_LEARN_IGNORE_VLAN,ANA_AGENCTRL);-/* Clear the MAC table */+/* Clear the MAC table. We are not concurrent with anyone, so+*holding&ocelot->mact_lockispointless.+*/ocelot_write(ocelot,MACACCESS_CMD_INIT,ANA_TABLES_MACACCESS);}
@@ -1170,6 +1190,7 @@ int ocelot_port_fdb_do_dump(const unsigned char *addr, u16 vid,}EXPORT_SYMBOL(ocelot_port_fdb_do_dump);+/* Caller must hold &ocelot->mact_lock */staticintocelot_mact_read(structocelot*ocelot,intport,introw,intcol,structocelot_mact_entry*entry){
@@ -1220,33 +1241,40 @@ static int ocelot_mact_read(struct ocelot *ocelot, int port, int row, int col,intocelot_fdb_dump(structocelot*ocelot,intport,dsa_fdb_dump_cb_t*cb,void*data){+interr=0;inti,j;+/* We could take the lock just around ocelot_mact_read, but doing so+*thousandsoftimesinarowseemsratherpointlessandinefficient.+*/+mutex_lock(&ocelot->mact_lock);+/* Loop through all the mac tables entries. */for(i=0;i<ocelot->num_mact_rows;i++){for(j=0;j<4;j++){structocelot_mact_entryentry;boolis_static;-intret;-ret=ocelot_mact_read(ocelot,port,i,j,&entry);+err=ocelot_mact_read(ocelot,port,i,j,&entry);/* If the entry is invalid (wrong port, invalid...),*skipit.*/-if(ret==-EINVAL)+if(err==-EINVAL)continue;-elseif(ret)-returnret;+elseif(err)+break;is_static=(entry.type==ENTRYTYPE_LOCKED);-ret=cb(entry.mac,entry.vid,is_static,data);-if(ret)-returnret;+err=cb(entry.mac,entry.vid,is_static,data);+if(err)+break;}}-return0;+mutex_unlock(&ocelot->mact_lock);++returnerr;}EXPORT_SYMBOL(ocelot_fdb_dump);
@@ -2231,6 +2259,7 @@ int ocelot_init(struct ocelot *ocelot)mutex_init(&ocelot->stats_lock);mutex_init(&ocelot->ptp_lock);+mutex_init(&ocelot->mact_lock);spin_lock_init(&ocelot->ptp_clock_lock);spin_lock_init(&ocelot->ts_id_lock);snprintf(queue_name,sizeof(queue_name),"%s-stats",
@@ -675,6 +675,9 @@ struct ocelot {structdelayed_workstats_work;structworkqueue_struct*stats_queue;+/* Lock for serializing access to the MAC table */+structmutexmact_lock;+structworkqueue_struct*owq;u8ptp:1;
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:45
The b53 driver performs non-atomic transactions to the ARL table when
adding, deleting and reading FDB and MDB entries.
Traditionally these were all serialized by the rtnl_lock(), but now it
is possible that DSA calls ->port_fdb_add and ->port_fdb_del without
holding that lock.
So the driver must have its own serialization logic. Add a mutex and
hold it from all entry points (->port_fdb_{add,del,dump},
->port_mdb_{add,del}).
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v2->v3: unlock arl_mutex centrally in b53_fdb_dump
v3->v4: use __must_hold
v4->v5: revert changes made in v4
drivers/net/dsa/b53/b53_common.c | 36 +++++++++++++++++++++++++-------
drivers/net/dsa/b53/b53_priv.h | 1 +
2 files changed, 30 insertions(+), 7 deletions(-)
@@ -1544,7 +1544,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,}EXPORT_SYMBOL(b53_vlan_del);-/* Address Resolution Logic routines */+/* Address Resolution Logic routines. Caller must hold &dev->arl_mutex. */staticintb53_arl_op_wait(structb53_device*dev){unsignedinttimeout=10;
@@ -1709,6 +1709,7 @@ int b53_fdb_add(struct dsa_switch *ds, int port,constunsignedchar*addr,u16vid){structb53_device*priv=ds->priv;+intret;/* 5325 and 5365 require some more massaging, but could*besupportedeventually
@@ -1716,7 +1717,11 @@ int b53_fdb_add(struct dsa_switch *ds, int port,if(is5325(priv)||is5365(priv))return-EOPNOTSUPP;-returnb53_arl_op(priv,0,port,addr,vid,true);+mutex_lock(&priv->arl_mutex);+ret=b53_arl_op(priv,0,port,addr,vid,true);+mutex_unlock(&priv->arl_mutex);++returnret;}EXPORT_SYMBOL(b53_fdb_add);
@@ -1724,8 +1729,13 @@ int b53_fdb_del(struct dsa_switch *ds, int port,constunsignedchar*addr,u16vid){structb53_device*priv=ds->priv;+intret;-returnb53_arl_op(priv,0,port,addr,vid,false);+mutex_lock(&priv->arl_mutex);+ret=b53_arl_op(priv,0,port,addr,vid,false);+mutex_unlock(&priv->arl_mutex);++returnret;}EXPORT_SYMBOL(b53_fdb_del);
@@ -1782,6 +1792,8 @@ int b53_fdb_dump(struct dsa_switch *ds, int port,intret;u8reg;+mutex_lock(&priv->arl_mutex);+/* Start search operation */reg=ARL_SRCH_STDN;b53_write8(priv,B53_ARLIO_PAGE,B53_ARL_SRCH_CTL,reg);
@@ -1789,18 +1801,18 @@ int b53_fdb_dump(struct dsa_switch *ds, int port,do{ret=b53_arl_search_wait(priv);if(ret)-returnret;+break;b53_arl_search_rd(priv,0,&results[0]);ret=b53_fdb_copy(port,&results[0],cb,data);if(ret)-returnret;+break;if(priv->num_arl_bins>2){b53_arl_search_rd(priv,1,&results[1]);ret=b53_fdb_copy(port,&results[1],cb,data);if(ret)-returnret;+break;if(!results[0].is_valid&&!results[1].is_valid)break;
@@ -1808,6 +1820,8 @@ int b53_fdb_dump(struct dsa_switch *ds, int port,}while(count++<b53_max_arl_entries(priv)/2);+mutex_unlock(&priv->arl_mutex);+return0;}EXPORT_SYMBOL(b53_fdb_dump);
@@ -1816,6 +1830,7 @@ int b53_mdb_add(struct dsa_switch *ds, int port,conststructswitchdev_obj_port_mdb*mdb){structb53_device*priv=ds->priv;+intret;/* 5325 and 5365 require some more massaging, but could*besupportedeventually
@@ -1823,7 +1838,11 @@ int b53_mdb_add(struct dsa_switch *ds, int port,if(is5325(priv)||is5365(priv))return-EOPNOTSUPP;-returnb53_arl_op(priv,0,port,mdb->addr,mdb->vid,true);+mutex_lock(&priv->arl_mutex);+ret=b53_arl_op(priv,0,port,mdb->addr,mdb->vid,true);+mutex_unlock(&priv->arl_mutex);++returnret;}EXPORT_SYMBOL(b53_mdb_add);
@@ -1833,7 +1852,9 @@ int b53_mdb_del(struct dsa_switch *ds, int port,structb53_device*priv=ds->priv;intret;+mutex_lock(&priv->arl_mutex);ret=b53_arl_op(priv,0,port,mdb->addr,mdb->vid,false);+mutex_unlock(&priv->arl_mutex);if(ret)dev_err(ds->dev,"failed to delete MDB entry\n");
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:51
The GSWIP switch accesses various bridging layer tables (VLANs, FDBs,
forwarding rules) indirectly through PCE registers. These hardware
accesses are non-atomic, being comprised of several register reads and
writes.
These accesses are currently serialized by the rtnl_lock, but DSA is
changing its driver API and that lock will no longer be held when
calling ->port_fdb_add() and ->port_fdb_del().
So this driver needs to serialize the access to the PCE registers using
its own locking scheme. This patch adds that.
Note that the driver also uses the gswip_pce_load_microcode() function
to load a static configuration for the packet classification engine into
a table using the same registers. It is currently not protected, but
since that configuration is only done from the dsa_switch_ops :: setup
method, there is no risk of it being concurrent with other operations.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Acked-by: Hauke Mehrtens <hauke@hauke-m.de>
---
v3->v4: call mutex_init
v4->v5: slightly reword the commit message according to Hauke's indications
drivers/net/dsa/lantiq_gswip.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
@@ -523,10 +524,14 @@ static int gswip_pce_table_entry_read(struct gswip_priv *priv,u16addr_mode=tbl->key_mode?GSWIP_PCE_TBL_CTRL_OPMOD_KSRD:GSWIP_PCE_TBL_CTRL_OPMOD_ADRD;+mutex_lock(&priv->pce_table_lock);+err=gswip_switch_r_timeout(priv,GSWIP_PCE_TBL_CTRL,GSWIP_PCE_TBL_CTRL_BAS);-if(err)+if(err){+mutex_unlock(&priv->pce_table_lock);returnerr;+}gswip_switch_w(priv,tbl->index,GSWIP_PCE_TBL_ADDR);gswip_switch_mask(priv,GSWIP_PCE_TBL_CTRL_ADDR_MASK|
@@ -536,8 +541,10 @@ static int gswip_pce_table_entry_read(struct gswip_priv *priv,err=gswip_switch_r_timeout(priv,GSWIP_PCE_TBL_CTRL,GSWIP_PCE_TBL_CTRL_BAS);-if(err)+if(err){+mutex_unlock(&priv->pce_table_lock);returnerr;+}for(i=0;i<ARRAY_SIZE(tbl->key);i++)tbl->key[i]=gswip_switch_r(priv,GSWIP_PCE_TBL_KEY(i));
@@ -553,6 +560,8 @@ static int gswip_pce_table_entry_read(struct gswip_priv *priv,tbl->valid=!!(crtl&GSWIP_PCE_TBL_CTRL_VLD);tbl->gmap=(crtl&GSWIP_PCE_TBL_CTRL_GMAP_MASK)>>7;+mutex_unlock(&priv->pce_table_lock);+return0;}
@@ -565,10 +574,14 @@ static int gswip_pce_table_entry_write(struct gswip_priv *priv,u16addr_mode=tbl->key_mode?GSWIP_PCE_TBL_CTRL_OPMOD_KSWR:GSWIP_PCE_TBL_CTRL_OPMOD_ADWR;+mutex_lock(&priv->pce_table_lock);+err=gswip_switch_r_timeout(priv,GSWIP_PCE_TBL_CTRL,GSWIP_PCE_TBL_CTRL_BAS);-if(err)+if(err){+mutex_unlock(&priv->pce_table_lock);returnerr;+}gswip_switch_w(priv,tbl->index,GSWIP_PCE_TBL_ADDR);gswip_switch_mask(priv,GSWIP_PCE_TBL_CTRL_ADDR_MASK|
@@ -600,8 +613,12 @@ static int gswip_pce_table_entry_write(struct gswip_priv *priv,crtl|=GSWIP_PCE_TBL_CTRL_BAS;gswip_switch_w(priv,crtl,GSWIP_PCE_TBL_CTRL);-returngswip_switch_r_timeout(priv,GSWIP_PCE_TBL_CTRL,-GSWIP_PCE_TBL_CTRL_BAS);+err=gswip_switch_r_timeout(priv,GSWIP_PCE_TBL_CTRL,+GSWIP_PCE_TBL_CTRL_BAS);++mutex_unlock(&priv->pce_table_lock);++returnerr;}/* Add the LAN port into a bridge with the CPU port by
@@ -2106,6 +2123,7 @@ static int gswip_probe(struct platform_device *pdev)priv->ds->priv=priv;priv->ds->ops=priv->hw_info->ops;priv->dev=dev;+mutex_init(&priv->pce_table_lock);version=gswip_switch_r(priv,GSWIP_VERSION);np=dev->of_node;
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:18:52
After talking with Ido Schimmel, it became clear that rtnl_lock is not
actually required for anything that is done inside the
SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE deferred work handlers.
The reason why it was probably added by Arkadi Sharshevsky in commit
c9eb3e0f8701 ("net: dsa: Add support for learning FDB through
notification") was to offer the same locking/serialization guarantees as
.ndo_fdb_{add,del} and avoid reworking any drivers.
DSA has implemented .ndo_fdb_add and .ndo_fdb_del until commit
b117e1e8a86d ("net: dsa: delete dsa_legacy_fdb_add and
dsa_legacy_fdb_del") - that is to say, until fairly recently.
But those methods have been deleted, so now we are free to drop the
rtnl_lock as well.
Note that exposing DSA switch drivers to an unlocked method which was
previously serialized by the rtnl_mutex is a potentially dangerous
affair. Driver writers couldn't ensure that their internal locking
scheme does the right thing even if they wanted.
We could err on the side of paranoia and introduce a switch-wide lock
inside the DSA framework, but that seems way overreaching. Instead, we
could check as many drivers for regressions as we can, fix those first,
then let this change go in once it is assumed to be fairly safe.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v1->v5: none
net/dsa/slave.c | 2 --
1 file changed, 2 deletions(-)
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:19:20
These programs are useful, but not all selftests require them.
Additionally, on embedded boards without package management (things like
buildroot), installing mausezahn or jq is not always as trivial as
downloading a package from the web.
So it is actually a bit annoying to require programs that are not used.
Introduce options that can be set by scripts to not enforce these
dependencies. For compatibility, default to "yes".
Cc: Nikolay Aleksandrov <redacted>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Guillaume Nault <redacted>
Cc: Po-Hsu Lin <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v1->v5: none
tools/testing/selftests/net/forwarding/lib.sh | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
@@ -141,8 +143,12 @@ require_command()fi}-require_commandjq-require_command$MZ+if[["$REQUIRE_JQ"="yes"]];then+require_commandjq+fi+if[["$REQUIRE_MZ"="yes"]];then+require_command$MZ+fiif[[!-vNUM_NETIFS]];thenecho"SKIP: importer does not define \"NUM_NETIFS\""
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:19:24
Now that the rtnl_mutex is going away for dsa_port_{host_,}fdb_{add,del},
no one is serializing access to the address lists that DSA keeps for the
purpose of reference counting on shared ports (CPU and cascade ports).
It can happen for one dsa_switch_do_fdb_del to do list_del on a dp->fdbs
element while another dsa_switch_do_fdb_{add,del} is traversing dp->fdbs.
We need to avoid that.
Currently dp->mdbs is not at risk, because dsa_switch_do_mdb_{add,del}
still runs under the rtnl_mutex. But it would be nice if it would not
depend on that being the case. So let's introduce a mutex per port (the
address lists are per port too) and share it between dp->mdbs and
dp->fdbs.
The place where we put the locking is interesting. It could be tempting
to put a DSA-level lock which still serializes calls to
.port_fdb_{add,del}, but it would still not avoid concurrency with other
driver code paths that are currently under rtnl_mutex (.port_fdb_dump,
.port_fast_age). So it would add a very false sense of security (and
adding a global switch-wide lock in DSA to resynchronize with the
rtnl_lock is also counterproductive and hard).
So the locking is intentionally done only where the dp->fdbs and dp->mdbs
lists are traversed. That means, from a driver perspective, that
.port_fdb_add will be called with the dp->addr_lists_lock mutex held on
the CPU port, but not held on user ports. This is done so that driver
writers are not encouraged to rely on any guarantee offered by
dp->addr_lists_lock.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v1->v4: none
v4->v5: rebase over new patch 01/10.
include/net/dsa.h | 1 +
net/dsa/dsa2.c | 1 +
net/dsa/switch.c | 76 ++++++++++++++++++++++++++++++++---------------
3 files changed, 54 insertions(+), 24 deletions(-)
@@ -287,6 +287,7 @@ struct dsa_port {/* List of MAC addresses that must be forwarded on this port.*TheseareonlyvalidonCPUportsandDSAlinks.*/+structmutexaddr_lists_lock;structlist_headfdbs;structlist_headmdbs;
@@ -215,26 +215,30 @@ static int dsa_port_do_mdb_add(struct dsa_port *dp,structdsa_switch*ds=dp->ds;structdsa_mac_addr*a;intport=dp->index;-interr;+interr=0;/* No need to bother with refcounting for user ports */if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))returnds->ops->port_mdb_add(ds,port,mdb);+mutex_lock(&dp->addr_lists_lock);+a=dsa_mac_addr_find(&dp->mdbs,mdb->addr,mdb->vid);if(a){refcount_inc(&a->refcount);-return0;+gotoout;}a=kzalloc(sizeof(*a),GFP_KERNEL);-if(!a)-return-ENOMEM;+if(!a){+err=-ENOMEM;+gotoout;+}err=ds->ops->port_mdb_add(ds,port,mdb);if(err){kfree(a);-returnerr;+gotoout;}ether_addr_copy(a->addr,mdb->addr);
@@ -242,7 +246,10 @@ static int dsa_port_do_mdb_add(struct dsa_port *dp,refcount_set(&a->refcount,1);list_add_tail(&a->list,&dp->mdbs);-return0;+out:+mutex_unlock(&dp->addr_lists_lock);++returnerr;}staticintdsa_port_do_mdb_del(structdsa_port*dp,
@@ -251,29 +258,36 @@ static int dsa_port_do_mdb_del(struct dsa_port *dp,structdsa_switch*ds=dp->ds;structdsa_mac_addr*a;intport=dp->index;-interr;+interr=0;/* No need to bother with refcounting for user ports */if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))returnds->ops->port_mdb_del(ds,port,mdb);+mutex_lock(&dp->addr_lists_lock);+a=dsa_mac_addr_find(&dp->mdbs,mdb->addr,mdb->vid);-if(!a)-return-ENOENT;+if(!a){+err=-ENOENT;+gotoout;+}if(!refcount_dec_and_test(&a->refcount))-return0;+gotoout;err=ds->ops->port_mdb_del(ds,port,mdb);if(err){refcount_set(&a->refcount,1);-returnerr;+gotoout;}list_del(&a->list);kfree(a);-return0;+out:+mutex_unlock(&dp->addr_lists_lock);++returnerr;}staticintdsa_port_do_fdb_add(structdsa_port*dp,constunsignedchar*addr,
@@ -282,26 +296,30 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,structdsa_switch*ds=dp->ds;structdsa_mac_addr*a;intport=dp->index;-interr;+interr=0;/* No need to bother with refcounting for user ports */if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))returnds->ops->port_fdb_add(ds,port,addr,vid);+mutex_lock(&dp->addr_lists_lock);+a=dsa_mac_addr_find(&dp->fdbs,addr,vid);if(a){refcount_inc(&a->refcount);-return0;+gotoout;}a=kzalloc(sizeof(*a),GFP_KERNEL);-if(!a)-return-ENOMEM;+if(!a){+err=-ENOMEM;+gotoout;+}err=ds->ops->port_fdb_add(ds,port,addr,vid);if(err){kfree(a);-returnerr;+gotoout;}ether_addr_copy(a->addr,addr);
@@ -318,29 +339,36 @@ static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,structdsa_switch*ds=dp->ds;structdsa_mac_addr*a;intport=dp->index;-interr;+interr=0;/* No need to bother with refcounting for user ports */if(!(dsa_port_is_cpu(dp)||dsa_port_is_dsa(dp)))returnds->ops->port_fdb_del(ds,port,addr,vid);+mutex_lock(&dp->addr_lists_lock);+a=dsa_mac_addr_find(&dp->fdbs,addr,vid);-if(!a)-return-ENOENT;+if(!a){+err=-ENOENT;+gotoout;+}if(!refcount_dec_and_test(&a->refcount))-return0;+gotoout;err=ds->ops->port_fdb_del(ds,port,addr,vid);if(err){refcount_set(&a->refcount,1);-returnerr;+gotoout;}list_del(&a->list);kfree(a);-return0;+out:+mutex_unlock(&dp->addr_lists_lock);++returnerr;}staticintdsa_switch_host_fdb_add(structdsa_switch*ds,
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: 2021-10-24 17:19:35
This test is a bit strange in that it is perhaps more manual than
others: it does not transmit a clear OK/FAIL verdict, because user space
does not have synchronous feedback from the kernel. If a hardware access
fails, it is in deferred context.
Nonetheless, on sja1105 I have used it successfully to find and solve a
concurrency issue, so it can be used as a starting point for other
driver maintainers too.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
v3->v4:
- use "wait $pid" instead of "killall bash" :-/
- reorder "ip link del br0" and waiting for the background task on cleanup
v4->v5: none
MAINTAINERS | 1 +
.../drivers/net/dsa/test_bridge_fdb_stress.sh | 47 +++++++++++++++++++
2 files changed, 48 insertions(+)
create mode 100755 tools/testing/selftests/drivers/net/dsa/test_bridge_fdb_stress.sh
@@ -0,0 +1,47 @@+#!/bin/bash+# SPDX-License-Identifier: GPL-2.0++# Bridge FDB entries can be offloaded to DSA switches without holding the+# rtnl_mutex. Traditionally this mutex has conferred drivers implicit+# serialization, which means their code paths are not well tested in the+# presence of concurrency.+# This test creates a background task that stresses the FDB by adding and+# deleting an entry many times in a row without the rtnl_mutex held.+# It then tests the driver resistance to concurrency by calling .ndo_fdb_dump+# (with rtnl_mutex held) from a foreground task.+# Since either the FDB dump or the additions/removals can fail, but the+# additions and removals are performed in deferred as opposed to process+# context, we cannot simply check for user space error codes.++WAIT_TIME=1+NUM_NETIFS=1+REQUIRE_JQ="no"+REQUIRE_MZ="no"+NETIF_CREATE="no"+lib_dir=$(dirname$0)/../../../net/forwarding+source$lib_dir/lib.sh++cleanup(){+echo"Cleaning up"+kill$pid&&wait$pid&>/dev/null+iplinkdelbr0+echo"Please check kernel log for errors"+}+trap'cleanup'EXIT++eth=${NETIFS[p1]}++iplinkdelbr02&>1>/dev/null||:+iplinkaddbr0typebridge&&iplinkset$ethmasterbr0++(while:;do+bridgefdbadd00:01:02:03:04:05dev$ethmasterstatic+bridgefdbdel00:01:02:03:04:05dev$ethmasterstatic+done)&+pid=$!++foriin$(seq150);do+bridgefdbshow>/dev/null+sleep3+echo"$((${i}*2))% complete..."+done
On Sun, Oct 24, 2021 at 08:17:56PM +0300, Vladimir Oltean wrote:
These programs are useful, but not all selftests require them.
Additionally, on embedded boards without package management (things like
buildroot), installing mausezahn or jq is not always as trivial as
downloading a package from the web.
So it is actually a bit annoying to require programs that are not used.
Introduce options that can be set by scripts to not enforce these
dependencies. For compatibility, default to "yes".
Cc: Nikolay Aleksandrov <redacted>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Guillaume Nault <redacted>
Cc: Po-Hsu Lin <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>