Skip to content

Commit

Permalink
Merge branch 'hsr-fix-lock-warnings'
Browse files Browse the repository at this point in the history 
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 #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]  #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 #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/20250905091533.377443-1-liuhangbin@gmail.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
  • Loading branch information
Paolo Abeni committed Sep 11, 2025
2 parents 3a1a66d + 847748f commit 9b1fbd3
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 18 deletions.
20 changes: 14 additions & 6 deletions drivers/net/ethernet/ti/icssg/icssg_prueth.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ static void icssg_prueth_hsr_fdb_add_del(struct prueth_emac *emac,

static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)
{
struct net_device *real_dev;
struct net_device *real_dev, *port_dev;
struct prueth_emac *emac;
u8 vlan_id, i;

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

if (is_hsr_master(real_dev)) {
for (i = HSR_PT_SLAVE_A; i < HSR_PT_INTERLINK; i++) {
emac = netdev_priv(hsr_get_port_ndev(real_dev, i));
if (!emac)
port_dev = hsr_get_port_ndev(real_dev, i);
emac = netdev_priv(port_dev);
if (!emac) {
dev_put(port_dev);
return -EINVAL;
}
icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id,
true);
dev_put(port_dev);
}
} else {
emac = netdev_priv(real_dev);
Expand All @@ -679,7 +683,7 @@ static int icssg_prueth_hsr_add_mcast(struct net_device *ndev, const u8 *addr)

static int icssg_prueth_hsr_del_mcast(struct net_device *ndev, const u8 *addr)
{
struct net_device *real_dev;
struct net_device *real_dev, *port_dev;
struct prueth_emac *emac;
u8 vlan_id, i;

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

if (is_hsr_master(real_dev)) {
for (i = HSR_PT_SLAVE_A; i < HSR_PT_INTERLINK; i++) {
emac = netdev_priv(hsr_get_port_ndev(real_dev, i));
if (!emac)
port_dev = hsr_get_port_ndev(real_dev, i);
emac = netdev_priv(port_dev);
if (!emac) {
dev_put(port_dev);
return -EINVAL;
}
icssg_prueth_hsr_fdb_add_del(emac, addr, vlan_id,
false);
dev_put(port_dev);
}
} else {
emac = netdev_priv(real_dev);
Expand Down
28 changes: 18 additions & 10 deletions net/hsr/hsr_device.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ static bool hsr_check_carrier(struct hsr_port *master)

ASSERT_RTNL();

hsr_for_each_port(master->hsr, port) {
hsr_for_each_port_rtnl(master->hsr, port) {
if (port->type != HSR_PT_MASTER && is_slave_up(port->dev)) {
netif_carrier_on(master->dev);
return true;
Expand Down Expand Up @@ -105,7 +105,7 @@ int hsr_get_max_mtu(struct hsr_priv *hsr)
struct hsr_port *port;

mtu_max = ETH_DATA_LEN;
hsr_for_each_port(hsr, port)
hsr_for_each_port_rtnl(hsr, port)
if (port->type != HSR_PT_MASTER)
mtu_max = min(port->dev->mtu, mtu_max);

Expand Down Expand Up @@ -139,7 +139,7 @@ static int hsr_dev_open(struct net_device *dev)

hsr = netdev_priv(dev);

hsr_for_each_port(hsr, port) {
hsr_for_each_port_rtnl(hsr, port) {
if (port->type == HSR_PT_MASTER)
continue;
switch (port->type) {
Expand Down Expand Up @@ -172,7 +172,7 @@ static int hsr_dev_close(struct net_device *dev)
struct hsr_priv *hsr;

hsr = netdev_priv(dev);
hsr_for_each_port(hsr, port) {
hsr_for_each_port_rtnl(hsr, port) {
if (port->type == HSR_PT_MASTER)
continue;
switch (port->type) {
Expand Down Expand Up @@ -205,7 +205,7 @@ static netdev_features_t hsr_features_recompute(struct hsr_priv *hsr,
* may become enabled.
*/
features &= ~NETIF_F_ONE_FOR_ALL;
hsr_for_each_port(hsr, port)
hsr_for_each_port_rtnl(hsr, port)
features = netdev_increment_features(features,
port->dev->features,
mask);
Expand All @@ -226,6 +226,7 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
struct hsr_priv *hsr = netdev_priv(dev);
struct hsr_port *master;

rcu_read_lock();
master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
if (master) {
skb->dev = master->dev;
Expand All @@ -238,6 +239,8 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
dev_core_stats_tx_dropped_inc(dev);
dev_kfree_skb_any(skb);
}
rcu_read_unlock();

return NETDEV_TX_OK;
}

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

hsr = netdev_priv(dev);

hsr_for_each_port(hsr, port) {
hsr_for_each_port_rtnl(hsr, port) {
if (port->type == HSR_PT_MASTER)
continue;
switch (port->type) {
Expand All @@ -506,7 +509,7 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)

hsr = netdev_priv(dev);

hsr_for_each_port(hsr, port) {
hsr_for_each_port_rtnl(hsr, port) {
if (port->type == HSR_PT_MASTER)
continue;
switch (port->type) {
Expand Down Expand Up @@ -534,7 +537,7 @@ static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,

hsr = netdev_priv(dev);

hsr_for_each_port(hsr, port) {
hsr_for_each_port_rtnl(hsr, port) {
if (port->type == HSR_PT_MASTER ||
port->type == HSR_PT_INTERLINK)
continue;
Expand Down Expand Up @@ -580,7 +583,7 @@ static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,

hsr = netdev_priv(dev);

hsr_for_each_port(hsr, port) {
hsr_for_each_port_rtnl(hsr, port) {
switch (port->type) {
case HSR_PT_SLAVE_A:
case HSR_PT_SLAVE_B:
Expand Down Expand Up @@ -672,9 +675,14 @@ struct net_device *hsr_get_port_ndev(struct net_device *ndev,
struct hsr_priv *hsr = netdev_priv(ndev);
struct hsr_port *port;

rcu_read_lock();
hsr_for_each_port(hsr, port)
if (port->type == pt)
if (port->type == pt) {
dev_hold(port->dev);
rcu_read_unlock();
return port->dev;
}
rcu_read_unlock();
return NULL;
}
EXPORT_SYMBOL(hsr_get_port_ndev);
Expand Down
4 changes: 2 additions & 2 deletions net/hsr/hsr_main.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ static bool hsr_slave_empty(struct hsr_priv *hsr)
{
struct hsr_port *port;

hsr_for_each_port(hsr, port)
hsr_for_each_port_rtnl(hsr, port)
if (port->type != HSR_PT_MASTER)
return false;
return true;
Expand Down Expand Up @@ -134,7 +134,7 @@ struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt)
{
struct hsr_port *port;

hsr_for_each_port(hsr, port)
hsr_for_each_port_rtnl(hsr, port)
if (port->type == pt)
return port;
return NULL;
Expand Down
3 changes: 3 additions & 0 deletions net/hsr/hsr_main.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,9 @@ struct hsr_priv {
#define hsr_for_each_port(hsr, port) \
list_for_each_entry_rcu((port), &(hsr)->ports, port_list)

#define hsr_for_each_port_rtnl(hsr, port) \
list_for_each_entry_rcu((port), &(hsr)->ports, port_list, lockdep_rtnl_is_held())

struct hsr_port *hsr_port_get_hsr(struct hsr_priv *hsr, enum hsr_port_type pt);

/* Caller must ensure skb is a valid HSR frame */
Expand Down

0 comments on commit 9b1fbd3

Please sign in to comment.