Skip to content

Commit 9b1fbd3

Browse files
author
Paolo Abeni
committed
Merge branch 'hsr-fix-lock-warnings'
Hangbin Liu says: ==================== hsr: fix lock warnings hsr_for_each_port is called in many places without holding the RCU read lock, this may trigger warnings on debug kernels like: [ 40.457015] [ T201] WARNING: suspicious RCU usage [ 40.457020] [ T201] 6.17.0-rc2-virtme l1k#1 Not tainted [ 40.457025] [ T201] ----------------------------- [ 40.457029] [ T201] net/hsr/hsr_main.c:137 RCU-list traversed in non-reader section!! [ 40.457036] [ T201] other info that might help us debug this: [ 40.457040] [ T201] rcu_scheduler_active = 2, debug_locks = 1 [ 40.457045] [ T201] 2 locks held by ip/201: [ 40.457050] [ T201] #0: ffffffff93040a40 (&ops->srcu){.+.+}-{0:0}, at: rtnl_link_ops_get+0xf2/0x280 [ 40.457080] [ T201] l1k#1: ffffffff92e7f968 (rtnl_mutex){+.+.}-{4:4}, at: rtnl_newlink+0x5e1/0xb20 [ 40.457102] [ T201] stack backtrace: [ 40.457108] [ T201] CPU: 2 UID: 0 PID: 201 Comm: ip Not tainted 6.17.0-rc2-virtme l1k#1 PREEMPT(full) [ 40.457114] [ T201] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 [ 40.457117] [ T201] Call Trace: [ 40.457120] [ T201] <TASK> [ 40.457126] [ T201] dump_stack_lvl+0x6f/0xb0 [ 40.457136] [ T201] lockdep_rcu_suspicious.cold+0x4f/0xb1 [ 40.457148] [ T201] hsr_port_get_hsr+0xfe/0x140 [ 40.457158] [ T201] hsr_add_port+0x192/0x940 [ 40.457167] [ T201] ? __pfx_hsr_add_port+0x10/0x10 [ 40.457176] [ T201] ? lockdep_init_map_type+0x5c/0x270 [ 40.457189] [ T201] hsr_dev_finalize+0x4bc/0xbf0 [ 40.457204] [ T201] hsr_newlink+0x3c3/0x8f0 [ 40.457212] [ T201] ? __pfx_hsr_newlink+0x10/0x10 [ 40.457222] [ T201] ? rtnl_create_link+0x173/0xe40 [ 40.457233] [ T201] rtnl_newlink_create+0x2cf/0x750 [ 40.457243] [ T201] ? __pfx_rtnl_newlink_create+0x10/0x10 [ 40.457247] [ T201] ? __dev_get_by_name+0x12/0x50 [ 40.457252] [ T201] ? rtnl_dev_get+0xac/0x140 [ 40.457259] [ T201] ? __pfx_rtnl_dev_get+0x10/0x10 [ 40.457285] [ T201] __rtnl_newlink+0x22c/0xa50 [ 40.457305] [ T201] rtnl_newlink+0x637/0xb20 Adding rcu_read_lock() for all hsr_for_each_port() looks confusing. Introduce a new helper, hsr_for_each_port_rtnl(), that assumes the RTNL lock is held. This allows callers in suitable contexts to iterate ports safely without explicit RCU locking. Other code paths that rely on RCU protection continue to use hsr_for_each_port() with rcu_read_lock(). ==================== Link: https://patch.msgid.link/[email protected] Signed-off-by: Paolo Abeni <[email protected]>
2 parents 3a1a66d + 847748f commit 9b1fbd3

File tree

4 files changed

+37
-18
lines changed

4 files changed

+37
-18
lines changed

‎drivers/net/ethernet/ti/icssg/icssg_prueth.c‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -654,7 +654,7 @@ static void icssg_prueth_hsr_fdb_add_del(struct prueth_emac *emac,
654654

655655
static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
656656
{
657-
struct net_device *real_dev;
657+
struct net_device *real_dev, *port_dev;
658658
struct prueth_emac *emac;
659659
u8 vlan_id, i;
660660

@@ -663,11 +663,15 @@ static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
663663

664664
if (is_hsr_master(real_dev)) {
665665
for (i = HSR_PT_SLAVE_A; i < HSR_PT_INTERLINK; i++) {
666-
emac = netdev_priv(hsr_get_port_ndev(real_dev, i));
667-
if (!emac)
666+
port_dev = hsr_get_port_ndev(real_dev, i);
667+
emac = netdev_priv(port_dev);
668+
if (!emac) {
669+
dev_put(port_dev);
668670
return -EINVAL;
671+
}
669672
icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id,
670673
true);
674+
dev_put(port_dev);
671675
}
672676
} else {
673677
emac = netdev_priv(real_dev);
@@ -679,7 +683,7 @@ static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
679683

680684
static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
681685
{
682-
struct net_device *real_dev;
686+
struct net_device *real_dev, *port_dev;
683687
struct prueth_emac *emac;
684688
u8 vlan_id, i;
685689

@@ -688,11 +692,15 @@ static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
688692

689693
if (is_hsr_master(real_dev)) {
690694
for (i = HSR_PT_SLAVE_A; i < HSR_PT_INTERLINK; i++) {
691-
emac = netdev_priv(hsr_get_port_ndev(real_dev, i));
692-
if (!emac)
695+
port_dev = hsr_get_port_ndev(real_dev, i);
696+
emac = netdev_priv(port_dev);
697+
if (!emac) {
698+
dev_put(port_dev);
693699
return -EINVAL;
700+
}
694701
icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id,
695702
false);
703+
dev_put(port_dev);
696704
}
697705
} else {
698706
emac = netdev_priv(real_dev);

‎net/hsr/hsr_device.c‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ static bool hsr_check_carrier(struct hsr_port *master)
4949

5050
ASSERT_RTNL();
5151

52-
hsr_for_each_port(master->hsr, port) {
52+
hsr_for_each_port_rtnl(master->hsr, port) {
5353
if (port->type != HSR_PT_MASTER && is_slave_up(port->dev)) {
5454
netif_carrier_on(master->dev);
5555
return true;
@@ -105,7 +105,7 @@ int hsr_get_max_mtu(struct hsr_priv *hsr)
105105
struct hsr_port *port;
106106

107107
mtu_max = ETH_DATA_LEN;
108-
hsr_for_each_port(hsr, port)
108+
hsr_for_each_port_rtnl(hsr, port)
109109
if (port->type != HSR_PT_MASTER)
110110
mtu_max = min(port->dev->mtu, mtu_max);
111111

@@ -139,7 +139,7 @@ static int hsr_dev_open(struct net_device *dev)
139139

140140
hsr = netdev_priv(dev);
141141

142-
hsr_for_each_port(hsr, port) {
142+
hsr_for_each_port_rtnl(hsr, port) {
143143
if (port->type == HSR_PT_MASTER)
144144
continue;
145145
switch (port->type) {
@@ -172,7 +172,7 @@ static int hsr_dev_close(struct net_device *dev)
172172
struct hsr_priv *hsr;
173173

174174
hsr = netdev_priv(dev);
175-
hsr_for_each_port(hsr, port) {
175+
hsr_for_each_port_rtnl(hsr, port) {
176176
if (port->type == HSR_PT_MASTER)
177177
continue;
178178
switch (port->type) {
@@ -205,7 +205,7 @@ static netdev_features_t hsr_features_recompute(struct hsr_priv *hsr,
205205
* may become enabled.
206206
*/
207207
features &= ~NETIF_F_ONE_FOR_ALL;
208-
hsr_for_each_port(hsr, port)
208+
hsr_for_each_port_rtnl(hsr, port)
209209
features = netdev_increment_features(features,
210210
port->dev->features,
211211
mask);
@@ -226,6 +226,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
226226
struct hsr_priv *hsr = netdev_priv(dev);
227227
struct hsr_port *master;
228228

229+
rcu_read_lock();
229230
master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
230231
if (master) {
231232
skb->dev = master->dev;
@@ -238,6 +239,8 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
238239
dev_core_stats_tx_dropped_inc(dev);
239240
dev_kfree_skb_any(skb);
240241
}
242+
rcu_read_unlock();
243+
241244
return NETDEV_TX_OK;
242245
}
243246

@@ -484,7 +487,7 @@ static void hsr_set_rx_mode(struct net_device *dev)
484487

485488
hsr = netdev_priv(dev);
486489

487-
hsr_for_each_port(hsr, port) {
490+
hsr_for_each_port_rtnl(hsr, port) {
488491
if (port->type == HSR_PT_MASTER)
489492
continue;
490493
switch (port->type) {
@@ -506,7 +509,7 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
506509

507510
hsr = netdev_priv(dev);
508511

509-
hsr_for_each_port(hsr, port) {
512+
hsr_for_each_port_rtnl(hsr, port) {
510513
if (port->type == HSR_PT_MASTER)
511514
continue;
512515
switch (port->type) {
@@ -534,7 +537,7 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
534537

535538
hsr = netdev_priv(dev);
536539

537-
hsr_for_each_port(hsr, port) {
540+
hsr_for_each_port_rtnl(hsr, port) {
538541
if (port->type == HSR_PT_MASTER ||
539542
port->type == HSR_PT_INTERLINK)
540543
continue;
@@ -580,7 +583,7 @@ static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
580583

581584
hsr = netdev_priv(dev);
582585

583-
hsr_for_each_port(hsr, port) {
586+
hsr_for_each_port_rtnl(hsr, port) {
584587
switch (port->type) {
585588
case HSR_PT_SLAVE_A:
586589
case HSR_PT_SLAVE_B:
@@ -672,9 +675,14 @@ struct net_device *hsr_get_port_ndev(struct net_device *ndev,
672675
struct hsr_priv *hsr = netdev_priv(ndev);
673676
struct hsr_port *port;
674677

678+
rcu_read_lock();
675679
hsr_for_each_port(hsr, port)
676-
if (port->type == pt)
680+
if (port->type == pt) {
681+
dev_hold(port->dev);
682+
rcu_read_unlock();
677683
return port->dev;
684+
}
685+
rcu_read_unlock();
678686
return NULL;
679687
}
680688
EXPORT_SYMBOL(hsr_get_port_ndev);

‎net/hsr/hsr_main.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ static bool hsr_slave_empty(struct hsr_priv *hsr)
2222
{
2323
struct hsr_port *port;
2424

25-
hsr_for_each_port(hsr, port)
25+
hsr_for_each_port_rtnl(hsr, port)
2626
if (port->type != HSR_PT_MASTER)
2727
return false;
2828
return true;
@@ -134,7 +134,7 @@ struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt)
134134
{
135135
struct hsr_port *port;
136136

137-
hsr_for_each_port(hsr, port)
137+
hsr_for_each_port_rtnl(hsr, port)
138138
if (port->type == pt)
139139
return port;
140140
return NULL;

‎net/hsr/hsr_main.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,9 @@ struct hsr_priv {
224224
#define hsr_for_each_port(hsr, port) \
225225
list_for_each_entry_rcu((port), &(hsr)->ports, port_list)
226226

227+
#define hsr_for_each_port_rtnl(hsr, port) \
228+
list_for_each_entry_rcu((port), &(hsr)->ports, port_list, lockdep_rtnl_is_held())
229+
227230
struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt);
228231

229232
/* Caller must ensure skb is a valid HSR frame */

0 commit comments

Comments
 (0)