Skip to content

Commit 338a3a4

Browse files
vladimirolteandavem330
authored andcommitted
net: dsa: introduce locking for the address lists on CPU and DSA ports
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 <[email protected]> Reviewed-by: Florian Fainelli <[email protected]> Signed-off-by: David S. Miller <[email protected]>
1 parent cf231b4 commit 338a3a4

File tree

3 files changed

+54
-24
lines changed

3 files changed

+54
-24
lines changed

include/net/dsa.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@ struct dsa_port {
287287
/* List of MAC addresses that must be forwarded on this port.
288288
* These are only valid on CPU ports and DSA links.
289289
*/
290+
struct mutex addr_lists_lock;
290291
struct list_head fdbs;
291292
struct list_head mdbs;
292293

net/dsa/dsa2.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,7 @@ static int dsa_port_setup(struct dsa_port *dp)
433433
if (dp->setup)
434434
return 0;
435435

436+
mutex_init(&dp->addr_lists_lock);
436437
INIT_LIST_HEAD(&dp->fdbs);
437438
INIT_LIST_HEAD(&dp->mdbs);
438439

net/dsa/switch.c

Lines changed: 52 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -215,34 +215,41 @@ static int dsa_port_do_mdb_add(struct dsa_port *dp,
215215
struct dsa_switch *ds = dp->ds;
216216
struct dsa_mac_addr *a;
217217
int port = dp->index;
218-
int err;
218+
int err = 0;
219219

220220
/* No need to bother with refcounting for user ports */
221221
if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
222222
return ds->ops->port_mdb_add(ds, port, mdb);
223223

224+
mutex_lock(&dp->addr_lists_lock);
225+
224226
a = dsa_mac_addr_find(&dp->mdbs, mdb->addr, mdb->vid);
225227
if (a) {
226228
refcount_inc(&a->refcount);
227-
return 0;
229+
goto out;
228230
}
229231

230232
a = kzalloc(sizeof(*a), GFP_KERNEL);
231-
if (!a)
232-
return -ENOMEM;
233+
if (!a) {
234+
err = -ENOMEM;
235+
goto out;
236+
}
233237

234238
err = ds->ops->port_mdb_add(ds, port, mdb);
235239
if (err) {
236240
kfree(a);
237-
return err;
241+
goto out;
238242
}
239243

240244
ether_addr_copy(a->addr, mdb->addr);
241245
a->vid = mdb->vid;
242246
refcount_set(&a->refcount, 1);
243247
list_add_tail(&a->list, &dp->mdbs);
244248

245-
return 0;
249+
out:
250+
mutex_unlock(&dp->addr_lists_lock);
251+
252+
return err;
246253
}
247254

248255
static int dsa_port_do_mdb_del(struct dsa_port *dp,
@@ -251,29 +258,36 @@ static int dsa_port_do_mdb_del(struct dsa_port *dp,
251258
struct dsa_switch *ds = dp->ds;
252259
struct dsa_mac_addr *a;
253260
int port = dp->index;
254-
int err;
261+
int err = 0;
255262

256263
/* No need to bother with refcounting for user ports */
257264
if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
258265
return ds->ops->port_mdb_del(ds, port, mdb);
259266

267+
mutex_lock(&dp->addr_lists_lock);
268+
260269
a = dsa_mac_addr_find(&dp->mdbs, mdb->addr, mdb->vid);
261-
if (!a)
262-
return -ENOENT;
270+
if (!a) {
271+
err = -ENOENT;
272+
goto out;
273+
}
263274

264275
if (!refcount_dec_and_test(&a->refcount))
265-
return 0;
276+
goto out;
266277

267278
err = ds->ops->port_mdb_del(ds, port, mdb);
268279
if (err) {
269280
refcount_set(&a->refcount, 1);
270-
return err;
281+
goto out;
271282
}
272283

273284
list_del(&a->list);
274285
kfree(a);
275286

276-
return 0;
287+
out:
288+
mutex_unlock(&dp->addr_lists_lock);
289+
290+
return err;
277291
}
278292

279293
static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
@@ -282,34 +296,41 @@ static int dsa_port_do_fdb_add(struct dsa_port *dp, const unsigned char *addr,
282296
struct dsa_switch *ds = dp->ds;
283297
struct dsa_mac_addr *a;
284298
int port = dp->index;
285-
int err;
299+
int err = 0;
286300

287301
/* No need to bother with refcounting for user ports */
288302
if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
289303
return ds->ops->port_fdb_add(ds, port, addr, vid);
290304

305+
mutex_lock(&dp->addr_lists_lock);
306+
291307
a = dsa_mac_addr_find(&dp->fdbs, addr, vid);
292308
if (a) {
293309
refcount_inc(&a->refcount);
294-
return 0;
310+
goto out;
295311
}
296312

297313
a = kzalloc(sizeof(*a), GFP_KERNEL);
298-
if (!a)
299-
return -ENOMEM;
314+
if (!a) {
315+
err = -ENOMEM;
316+
goto out;
317+
}
300318

301319
err = ds->ops->port_fdb_add(ds, port, addr, vid);
302320
if (err) {
303321
kfree(a);
304-
return err;
322+
goto out;
305323
}
306324

307325
ether_addr_copy(a->addr, addr);
308326
a->vid = vid;
309327
refcount_set(&a->refcount, 1);
310328
list_add_tail(&a->list, &dp->fdbs);
311329

312-
return 0;
330+
out:
331+
mutex_unlock(&dp->addr_lists_lock);
332+
333+
return err;
313334
}
314335

315336
static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,
@@ -318,29 +339,36 @@ static int dsa_port_do_fdb_del(struct dsa_port *dp, const unsigned char *addr,
318339
struct dsa_switch *ds = dp->ds;
319340
struct dsa_mac_addr *a;
320341
int port = dp->index;
321-
int err;
342+
int err = 0;
322343

323344
/* No need to bother with refcounting for user ports */
324345
if (!(dsa_port_is_cpu(dp) || dsa_port_is_dsa(dp)))
325346
return ds->ops->port_fdb_del(ds, port, addr, vid);
326347

348+
mutex_lock(&dp->addr_lists_lock);
349+
327350
a = dsa_mac_addr_find(&dp->fdbs, addr, vid);
328-
if (!a)
329-
return -ENOENT;
351+
if (!a) {
352+
err = -ENOENT;
353+
goto out;
354+
}
330355

331356
if (!refcount_dec_and_test(&a->refcount))
332-
return 0;
357+
goto out;
333358

334359
err = ds->ops->port_fdb_del(ds, port, addr, vid);
335360
if (err) {
336361
refcount_set(&a->refcount, 1);
337-
return err;
362+
goto out;
338363
}
339364

340365
list_del(&a->list);
341366
kfree(a);
342367

343-
return 0;
368+
out:
369+
mutex_unlock(&dp->addr_lists_lock);
370+
371+
return err;
344372
}
345373

346374
static int dsa_switch_host_fdb_add(struct dsa_switch *ds,

0 commit comments

Comments
 (0)