* [Patch net] net: change addr_list_lock back to static key
@ 2020-06-08 21:53 Cong Wang
2020-06-09 20:00 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Cong Wang @ 2020-06-08 21:53 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo, Dmitry Vyukov
The dynamic key update for addr_list_lock still causes troubles,
for example the following race condition still exists:
CPU 0: CPU 1:
(RCU read lock) (RTNL lock)
dev_mc_seq_show() netdev_update_lockdep_key()
-> lockdep_unregister_key()
-> netif_addr_lock_bh()
because lockdep doesn't provide an API to update it atomically.
Therefore, we have to move it back to static keys and use subclass
for nest locking like before.
In commit 1a33e10e4a95 ("net: partially revert dynamic lockdep key
changes"), I already reverted most parts of commit ab92d68fc22f
("net: core: add generic lockdep keys").
This patch reverts the rest and also part of commit f3b0a18bb6cb
("net: remove unnecessary variables and callback"). After this
patch, addr_list_lock changes back to using static keys and
subclasses to satisfy lockdep. Thanks to dev->lower_level, we do
not have to change back to ->ndo_get_lock_subclass().
And hopefully this reduces some syzbot lockdep noises too.
Reported-by: syzbot+f3a0e80c34b3fc28ac5e@syzkaller.appspotmail.com
Cc: Taehee Yoo <ap420073@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
drivers/net/bonding/bond_main.c | 2 --
drivers/net/bonding/bond_options.c | 2 --
drivers/net/hamradio/bpqether.c | 2 ++
drivers/net/macsec.c | 5 ++++
drivers/net/macvlan.c | 13 ++++++--
drivers/net/vxlan.c | 4 +--
.../net/wireless/intersil/hostap/hostap_hw.c | 3 ++
include/linux/netdevice.h | 12 +++++---
net/8021q/vlan_dev.c | 8 +++--
net/batman-adv/soft-interface.c | 2 ++
net/bridge/br_device.c | 8 +++++
net/core/dev.c | 30 ++++++++++---------
net/core/dev_addr_lists.c | 12 ++++----
net/core/rtnetlink.c | 1 -
net/dsa/master.c | 4 +++
net/netrom/af_netrom.c | 2 ++
net/rose/af_rose.c | 2 ++
17 files changed, 76 insertions(+), 36 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index a25c65d4af71..004919aea5fb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3687,8 +3687,6 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
case BOND_RELEASE_OLD:
case SIOCBONDRELEASE:
res = bond_release(bond_dev, slave_dev);
- if (!res)
- netdev_update_lockdep_key(slave_dev);
break;
case BOND_SETHWADDR_OLD:
case SIOCBONDSETHWADDR:
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 215c10923289..ddb3916d3506 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -1398,8 +1398,6 @@ static int bond_option_slaves_set(struct bonding *bond,
case '-':
slave_dbg(bond->dev, dev, "Releasing interface\n");
ret = bond_release(bond->dev, dev);
- if (!ret)
- netdev_update_lockdep_key(dev);
break;
default:
diff --git a/drivers/net/hamradio/bpqether.c b/drivers/net/hamradio/bpqether.c
index 60dcaf2a04a9..1ad6085994b1 100644
--- a/drivers/net/hamradio/bpqether.c
+++ b/drivers/net/hamradio/bpqether.c
@@ -113,6 +113,7 @@ static LIST_HEAD(bpq_devices);
* off into a separate class since they always nest.
*/
static struct lock_class_key bpq_netdev_xmit_lock_key;
+static struct lock_class_key bpq_netdev_addr_lock_key;
static void bpq_set_lockdep_class_one(struct net_device *dev,
struct netdev_queue *txq,
@@ -123,6 +124,7 @@ static void bpq_set_lockdep_class_one(struct net_device *dev,
static void bpq_set_lockdep_class(struct net_device *dev)
{
+ lockdep_set_class(&dev->addr_list_lock, &bpq_netdev_addr_lock_key);
netdev_for_each_tx_queue(dev, bpq_set_lockdep_class_one, NULL);
}
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 20b53e255f68..e56547bfdac9 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3999,6 +3999,8 @@ static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len)
return 0;
}
+static struct lock_class_key macsec_netdev_addr_lock_key;
+
static int macsec_newlink(struct net *net, struct net_device *dev,
struct nlattr *tb[], struct nlattr *data[],
struct netlink_ext_ack *extack)
@@ -4050,6 +4052,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
return err;
netdev_lockdep_set_classes(dev);
+ lockdep_set_class_and_subclass(&dev->addr_list_lock,
+ &macsec_netdev_addr_lock_key,
+ dev->lower_level);
err = netdev_upper_dev_link(real_dev, dev, extack);
if (err < 0)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 563aed5b3d9f..6a6cc9f75307 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -860,6 +860,8 @@ static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
* "super class" of normal network devices; split their locks off into a
* separate class since they always nest.
*/
+static struct lock_class_key macvlan_netdev_addr_lock_key;
+
#define ALWAYS_ON_OFFLOADS \
(NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \
NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL)
@@ -875,6 +877,14 @@ static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
#define MACVLAN_STATE_MASK \
((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
+static void macvlan_set_lockdep_class(struct net_device *dev)
+{
+ netdev_lockdep_set_classes(dev);
+ lockdep_set_class_and_subclass(&dev->addr_list_lock,
+ &macvlan_netdev_addr_lock_key,
+ dev->lower_level);
+}
+
static int macvlan_init(struct net_device *dev)
{
struct macvlan_dev *vlan = netdev_priv(dev);
@@ -892,8 +902,7 @@ static int macvlan_init(struct net_device *dev)
dev->gso_max_size = lowerdev->gso_max_size;
dev->gso_max_segs = lowerdev->gso_max_segs;
dev->hard_header_len = lowerdev->hard_header_len;
-
- netdev_lockdep_set_classes(dev);
+ macvlan_set_lockdep_class(dev);
vlan->pcpu_stats = netdev_alloc_pcpu_stats(struct vlan_pcpu_stats);
if (!vlan->pcpu_stats)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 5bb448ae6c9c..47424b2da643 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -4245,10 +4245,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
mod_timer(&vxlan->age_timer, jiffies);
netdev_adjacent_change_commit(dst->remote_dev, lowerdev, dev);
- if (lowerdev && lowerdev != dst->remote_dev) {
+ if (lowerdev && lowerdev != dst->remote_dev)
dst->remote_dev = lowerdev;
- netdev_update_lockdep_key(lowerdev);
- }
vxlan_config_apply(dev, &conf, lowerdev, vxlan->net, true);
return 0;
}
diff --git a/drivers/net/wireless/intersil/hostap/hostap_hw.c b/drivers/net/wireless/intersil/hostap/hostap_hw.c
index aadf3dec5bf3..2ab34cf74ecc 100644
--- a/drivers/net/wireless/intersil/hostap/hostap_hw.c
+++ b/drivers/net/wireless/intersil/hostap/hostap_hw.c
@@ -3048,6 +3048,7 @@ static void prism2_clear_set_tim_queue(local_info_t *local)
* This is a natural nesting, which needs a split lock type.
*/
static struct lock_class_key hostap_netdev_xmit_lock_key;
+static struct lock_class_key hostap_netdev_addr_lock_key;
static void prism2_set_lockdep_class_one(struct net_device *dev,
struct netdev_queue *txq,
@@ -3059,6 +3060,8 @@ static void prism2_set_lockdep_class_one(struct net_device *dev,
static void prism2_set_lockdep_class(struct net_device *dev)
{
+ lockdep_set_class(&dev->addr_list_lock,
+ &hostap_netdev_addr_lock_key);
netdev_for_each_tx_queue(dev, prism2_set_lockdep_class_one, NULL);
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1a96e9c4ec36..e2825e27ef89 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1821,8 +1821,6 @@ enum netdev_priv_flags {
* for hardware timestamping
* @sfp_bus: attached &struct sfp_bus structure.
*
- * @addr_list_lock_key: lockdep class annotating
- * net_device->addr_list_lock spinlock
* @qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
* @qdisc_running_key: lockdep class annotating Qdisc->running seqcount
*
@@ -2125,7 +2123,6 @@ struct net_device {
#endif
struct phy_device *phydev;
struct sfp_bus *sfp_bus;
- struct lock_class_key addr_list_lock_key;
struct lock_class_key *qdisc_tx_busylock;
struct lock_class_key *qdisc_running_key;
bool proto_down;
@@ -2217,10 +2214,13 @@ static inline void netdev_for_each_tx_queue(struct net_device *dev,
static struct lock_class_key qdisc_tx_busylock_key; \
static struct lock_class_key qdisc_running_key; \
static struct lock_class_key qdisc_xmit_lock_key; \
+ static struct lock_class_key dev_addr_list_lock_key; \
unsigned int i; \
\
(dev)->qdisc_tx_busylock = &qdisc_tx_busylock_key; \
(dev)->qdisc_running_key = &qdisc_running_key; \
+ lockdep_set_class(&(dev)->addr_list_lock, \
+ &dev_addr_list_lock_key); \
for (i = 0; i < (dev)->num_tx_queues; i++) \
lockdep_set_class(&(dev)->_tx[i]._xmit_lock, \
&qdisc_xmit_lock_key); \
@@ -3253,7 +3253,6 @@ static inline void netif_stop_queue(struct net_device *dev)
}
void netif_tx_stop_all_queues(struct net_device *dev);
-void netdev_update_lockdep_key(struct net_device *dev);
static inline bool netif_tx_queue_stopped(const struct netdev_queue *dev_queue)
{
@@ -4239,6 +4238,11 @@ static inline void netif_addr_lock(struct net_device *dev)
spin_lock(&dev->addr_list_lock);
}
+static inline void netif_addr_lock_nested(struct net_device *dev)
+{
+ spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
+}
+
static inline void netif_addr_lock_bh(struct net_device *dev)
{
spin_lock_bh(&dev->addr_list_lock);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index f00bb57f0f60..c8d6a07e23c5 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -494,6 +494,7 @@ static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
* separate class since they always nest.
*/
static struct lock_class_key vlan_netdev_xmit_lock_key;
+static struct lock_class_key vlan_netdev_addr_lock_key;
static void vlan_dev_set_lockdep_one(struct net_device *dev,
struct netdev_queue *txq,
@@ -502,8 +503,11 @@ static void vlan_dev_set_lockdep_one(struct net_device *dev,
lockdep_set_class(&txq->_xmit_lock, &vlan_netdev_xmit_lock_key);
}
-static void vlan_dev_set_lockdep_class(struct net_device *dev)
+static void vlan_dev_set_lockdep_class(struct net_device *dev, int subclass)
{
+ lockdep_set_class_and_subclass(&dev->addr_list_lock,
+ &vlan_netdev_addr_lock_key,
+ subclass);
netdev_for_each_tx_queue(dev, vlan_dev_set_lockdep_one, NULL);
}
@@ -597,7 +601,7 @@ static int vlan_dev_init(struct net_device *dev)
SET_NETDEV_DEVTYPE(dev, &vlan_type);
- vlan_dev_set_lockdep_class(dev);
+ vlan_dev_set_lockdep_class(dev, dev->lower_level);
vlan->vlan_pcpu_stats = netdev_alloc_pcpu_stats(struct vlan_pcpu_stats);
if (!vlan->vlan_pcpu_stats)
diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
index 0ddd80130ea3..f1f1c86f3419 100644
--- a/net/batman-adv/soft-interface.c
+++ b/net/batman-adv/soft-interface.c
@@ -745,6 +745,7 @@ static int batadv_interface_kill_vid(struct net_device *dev, __be16 proto,
* separate class since they always nest.
*/
static struct lock_class_key batadv_netdev_xmit_lock_key;
+static struct lock_class_key batadv_netdev_addr_lock_key;
/**
* batadv_set_lockdep_class_one() - Set lockdep class for a single tx queue
@@ -765,6 +766,7 @@ static void batadv_set_lockdep_class_one(struct net_device *dev,
*/
static void batadv_set_lockdep_class(struct net_device *dev)
{
+ lockdep_set_class(&dev->addr_list_lock, &batadv_netdev_addr_lock_key);
netdev_for_each_tx_queue(dev, batadv_set_lockdep_class_one, NULL);
}
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8ec1362588af..8c7b78f8bc23 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -105,6 +105,13 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
return NETDEV_TX_OK;
}
+static struct lock_class_key bridge_netdev_addr_lock_key;
+
+static void br_set_lockdep_class(struct net_device *dev)
+{
+ lockdep_set_class(&dev->addr_list_lock, &bridge_netdev_addr_lock_key);
+}
+
static int br_dev_init(struct net_device *dev)
{
struct net_bridge *br = netdev_priv(dev);
@@ -143,6 +150,7 @@ static int br_dev_init(struct net_device *dev)
br_fdb_hash_fini(br);
}
+ br_set_lockdep_class(dev);
return err;
}
diff --git a/net/core/dev.c b/net/core/dev.c
index 061496a1f640..6bc2388141f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -439,6 +439,7 @@ static const char *const netdev_lock_name[] = {
"_xmit_IEEE802154", "_xmit_VOID", "_xmit_NONE"};
static struct lock_class_key netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
+static struct lock_class_key netdev_addr_lock_key[ARRAY_SIZE(netdev_lock_type)];
static inline unsigned short netdev_lock_pos(unsigned short dev_type)
{
@@ -460,11 +461,25 @@ static inline void netdev_set_xmit_lockdep_class(spinlock_t *lock,
lockdep_set_class_and_name(lock, &netdev_xmit_lock_key[i],
netdev_lock_name[i]);
}
+
+static inline void netdev_set_addr_lockdep_class(struct net_device *dev)
+{
+ int i;
+
+ i = netdev_lock_pos(dev->type);
+ lockdep_set_class_and_name(&dev->addr_list_lock,
+ &netdev_addr_lock_key[i],
+ netdev_lock_name[i]);
+}
#else
static inline void netdev_set_xmit_lockdep_class(spinlock_t *lock,
unsigned short dev_type)
{
}
+
+static inline void netdev_set_addr_lockdep_class(struct net_device *dev)
+{
+}
#endif
/*******************************************************************************
@@ -9373,15 +9388,6 @@ void netif_tx_stop_all_queues(struct net_device *dev)
}
EXPORT_SYMBOL(netif_tx_stop_all_queues);
-void netdev_update_lockdep_key(struct net_device *dev)
-{
- lockdep_unregister_key(&dev->addr_list_lock_key);
- lockdep_register_key(&dev->addr_list_lock_key);
-
- lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
-}
-EXPORT_SYMBOL(netdev_update_lockdep_key);
-
/**
* register_netdevice - register a network device
* @dev: device to register
@@ -9420,7 +9426,7 @@ int register_netdevice(struct net_device *dev)
return ret;
spin_lock_init(&dev->addr_list_lock);
- lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
+ netdev_set_addr_lockdep_class(dev);
ret = dev_get_valid_name(net, dev, dev->name);
if (ret < 0)
@@ -9939,8 +9945,6 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
dev_net_set(dev, &init_net);
- lockdep_register_key(&dev->addr_list_lock_key);
-
dev->gso_max_size = GSO_MAX_SIZE;
dev->gso_max_segs = GSO_MAX_SEGS;
dev->upper_level = 1;
@@ -10028,8 +10032,6 @@ void free_netdev(struct net_device *dev)
free_percpu(dev->xdp_bulkq);
dev->xdp_bulkq = NULL;
- lockdep_unregister_key(&dev->addr_list_lock_key);
-
/* Compatibility with error handling in drivers */
if (dev->reg_state == NETREG_UNINITIALIZED) {
netdev_freemem(dev);
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 2f949b5a1eb9..6393ba930097 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -637,7 +637,7 @@ int dev_uc_sync(struct net_device *to, struct net_device *from)
if (to->addr_len != from->addr_len)
return -EINVAL;
- netif_addr_lock(to);
+ netif_addr_lock_nested(to);
err = __hw_addr_sync(&to->uc, &from->uc, to->addr_len);
if (!err)
__dev_set_rx_mode(to);
@@ -667,7 +667,7 @@ int dev_uc_sync_multiple(struct net_device *to, struct net_device *from)
if (to->addr_len != from->addr_len)
return -EINVAL;
- netif_addr_lock(to);
+ netif_addr_lock_nested(to);
err = __hw_addr_sync_multiple(&to->uc, &from->uc, to->addr_len);
if (!err)
__dev_set_rx_mode(to);
@@ -691,7 +691,7 @@ void dev_uc_unsync(struct net_device *to, struct net_device *from)
return;
netif_addr_lock_bh(from);
- netif_addr_lock(to);
+ netif_addr_lock_nested(to);
__hw_addr_unsync(&to->uc, &from->uc, to->addr_len);
__dev_set_rx_mode(to);
netif_addr_unlock(to);
@@ -858,7 +858,7 @@ int dev_mc_sync(struct net_device *to, struct net_device *from)
if (to->addr_len != from->addr_len)
return -EINVAL;
- netif_addr_lock(to);
+ netif_addr_lock_nested(to);
err = __hw_addr_sync(&to->mc, &from->mc, to->addr_len);
if (!err)
__dev_set_rx_mode(to);
@@ -888,7 +888,7 @@ int dev_mc_sync_multiple(struct net_device *to, struct net_device *from)
if (to->addr_len != from->addr_len)
return -EINVAL;
- netif_addr_lock(to);
+ netif_addr_lock_nested(to);
err = __hw_addr_sync_multiple(&to->mc, &from->mc, to->addr_len);
if (!err)
__dev_set_rx_mode(to);
@@ -912,7 +912,7 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
return;
netif_addr_lock_bh(from);
- netif_addr_lock(to);
+ netif_addr_lock_nested(to);
__hw_addr_unsync(&to->mc, &from->mc, to->addr_len);
__dev_set_rx_mode(to);
netif_addr_unlock(to);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2269199c5891..9aedc15736ad 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2462,7 +2462,6 @@ static int do_set_master(struct net_device *dev, int ifindex,
err = ops->ndo_del_slave(upper_dev, dev);
if (err)
return err;
- netdev_update_lockdep_key(dev);
} else {
return -EOPNOTSUPP;
}
diff --git a/net/dsa/master.c b/net/dsa/master.c
index a621367c6e8c..480a61460c23 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -327,6 +327,8 @@ static void dsa_master_reset_mtu(struct net_device *dev)
rtnl_unlock();
}
+static struct lock_class_key dsa_master_addr_list_lock_key;
+
int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
{
int ret;
@@ -345,6 +347,8 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
wmb();
dev->dsa_ptr = cpu_dp;
+ lockdep_set_class(&dev->addr_list_lock,
+ &dsa_master_addr_list_lock_key);
ret = dsa_master_ethtool_setup(dev);
if (ret)
return ret;
diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index eccc7d366e17..f90ef6934b8f 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -70,6 +70,7 @@ static const struct proto_ops nr_proto_ops;
* separate class since they always nest.
*/
static struct lock_class_key nr_netdev_xmit_lock_key;
+static struct lock_class_key nr_netdev_addr_lock_key;
static void nr_set_lockdep_one(struct net_device *dev,
struct netdev_queue *txq,
@@ -80,6 +81,7 @@ static void nr_set_lockdep_one(struct net_device *dev,
static void nr_set_lockdep_key(struct net_device *dev)
{
+ lockdep_set_class(&dev->addr_list_lock, &nr_netdev_addr_lock_key);
netdev_for_each_tx_queue(dev, nr_set_lockdep_one, NULL);
}
diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index e7a872207b46..ce85656ac9c1 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -71,6 +71,7 @@ ax25_address rose_callsign;
* separate class since they always nest.
*/
static struct lock_class_key rose_netdev_xmit_lock_key;
+static struct lock_class_key rose_netdev_addr_lock_key;
static void rose_set_lockdep_one(struct net_device *dev,
struct netdev_queue *txq,
@@ -81,6 +82,7 @@ static void rose_set_lockdep_one(struct net_device *dev,
static void rose_set_lockdep_key(struct net_device *dev)
{
+ lockdep_set_class(&dev->addr_list_lock, &rose_netdev_addr_lock_key);
netdev_for_each_tx_queue(dev, rose_set_lockdep_one, NULL);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-08 21:53 [Patch net] net: change addr_list_lock back to static key Cong Wang
@ 2020-06-09 20:00 ` David Miller
2020-06-10 14:48 ` Taehee Yoo
2020-06-18 19:40 ` Vladimir Oltean
2 siblings, 0 replies; 17+ messages in thread
From: David Miller @ 2020-06-09 20:00 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev, syzbot+f3a0e80c34b3fc28ac5e, ap420073, dvyukov
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 8 Jun 2020 14:53:01 -0700
> The dynamic key update for addr_list_lock still causes troubles,
> for example the following race condition still exists:
>
> CPU 0: CPU 1:
> (RCU read lock) (RTNL lock)
> dev_mc_seq_show() netdev_update_lockdep_key()
> -> lockdep_unregister_key()
> -> netif_addr_lock_bh()
>
> because lockdep doesn't provide an API to update it atomically.
> Therefore, we have to move it back to static keys and use subclass
> for nest locking like before.
>
> In commit 1a33e10e4a95 ("net: partially revert dynamic lockdep key
> changes"), I already reverted most parts of commit ab92d68fc22f
> ("net: core: add generic lockdep keys").
>
> This patch reverts the rest and also part of commit f3b0a18bb6cb
> ("net: remove unnecessary variables and callback"). After this
> patch, addr_list_lock changes back to using static keys and
> subclasses to satisfy lockdep. Thanks to dev->lower_level, we do
> not have to change back to ->ndo_get_lock_subclass().
>
> And hopefully this reduces some syzbot lockdep noises too.
>
> Reported-by: syzbot+f3a0e80c34b3fc28ac5e@syzkaller.appspotmail.com
> Cc: Taehee Yoo <ap420073@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Ok, let's see how this goes.
Applied, thanks Cong.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-08 21:53 [Patch net] net: change addr_list_lock back to static key Cong Wang
2020-06-09 20:00 ` David Miller
@ 2020-06-10 14:48 ` Taehee Yoo
2020-06-10 23:21 ` Cong Wang
2020-06-18 19:40 ` Vladimir Oltean
2 siblings, 1 reply; 17+ messages in thread
From: Taehee Yoo @ 2020-06-10 14:48 UTC (permalink / raw)
To: Cong Wang; +Cc: Netdev, syzbot+f3a0e80c34b3fc28ac5e, Dmitry Vyukov
On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
Hi Cong,
Thank you for this work!
> The dynamic key update for addr_list_lock still causes troubles,
> for example the following race condition still exists:
>
> CPU 0: CPU 1:
> (RCU read lock) (RTNL lock)
> dev_mc_seq_show() netdev_update_lockdep_key()
> -> lockdep_unregister_key()
> -> netif_addr_lock_bh()
>
> because lockdep doesn't provide an API to update it atomically.
> Therefore, we have to move it back to static keys and use subclass
> for nest locking like before.
>
I'm sorry for the late reply.
I agree that using subclass mechanism to avoid too many lockdep keys.
But the subclass mechanism is also not updated its subclass key
automatically. So, if upper/lower relationship is changed,
interface would have incorrect subclass key.
It eventually results in lockdep warning.
And, I think this patch doesn't contain bonding and team module part.
So, an additional patch is needed.
Test commands:
ip link add bond0 type bond
ip link add vlan0 link bond0 type vlan id 1
ip link add bond1 type bond
ip link add vlan1 link bond1 type vlan id 2
ip link set bond0 up
ip link set vlan0 down
ip link set vlan0 master bond1
ip link set bond0 up
ip link set bond1 up
ip link set vlan0 up
ip link set vlan1 up
modprobe -rv 8021q
vlan0 vlan1 <-- subclass 1
| |
bond0 bond1 <-- subclass 0
After master operation
vlan1 <-- subclass 1
|
bond1 <-- subclass 0
|
vlan0 <-- subclass 1
|
bond0 <-- subclass 0
vlan1 subclass should be 3 but it is still 1 because of a lack of
subclass update routine.
Splat looks like:
[ 126.733736][ T1318] ============================================
[ 126.734882][ T1318] WARNING: possible recursive locking detected
[ 126.735993][ T1318] 5.7.0+ #577 Not tainted
[ 126.736814][ T1318] --------------------------------------------
[ 126.737979][ T1318] modprobe/1318 is trying to acquire lock:
[ 126.738922][ T1318] ffff88805f53c280
(&vlan_netdev_addr_lock_key/1){+...}-{2:2}, at:
dev_uc_sync_multiple+0xca/0x160
[ 126.740637][ T1318]
[ 126.740637][ T1318] but task is already holding lock:
[ 126.741834][ T1318] ffff888060ed1280
(&vlan_netdev_addr_lock_key/1){+...}-{2:2}, at:
dev_mc_unsync+0x98/0x170
[ 126.743442][ T1318]
[ 126.743442][ T1318] other info that might help us debug this:
[ 126.744585][ T1318] Possible unsafe locking scenario:
[ 126.744585][ T1318]
[ 126.747081][ T1318] CPU0
[ 126.747556][ T1318] ----
[ 126.748035][ T1318] lock(&vlan_netdev_addr_lock_key/1);
[ 126.748728][ T1318] lock(&vlan_netdev_addr_lock_key/1);
[ 126.749416][ T1318]
[ 126.749416][ T1318] *** DEADLOCK ***
[ 126.749416][ T1318]
[ 126.750449][ T1318] May be due to missing lock nesting notation
[ 126.750449][ T1318]
[ 126.751530][ T1318] 5 locks held by modprobe/1318:
[ 126.752152][ T1318] #0: ffffffffb6d013b8
(pernet_ops_rwsem){+.+.}-{3:3}, at: rtnl_link_unregister+0x88/0x220
[ 126.753427][ T1318] #1: ffffffffb6d0da30 (rtnl_mutex){+.+.}-{3:3},
at: rtnl_link_unregister+0xd6/0x220
[ 126.754609][ T1318] #2: ffff888060ed1280
(&vlan_netdev_addr_lock_key/1){+...}-{2:2}, at:
dev_mc_unsync+0x98/0x170
[ 126.756185][ T1318] #3: ffff88805c69a280
(&dev_addr_list_lock_key/2){+...}-{2:2}, at: dev_mc_unsync+0xdc/0x170
[ 126.758001][ T1318] #4: ffffffffb692b580
(rcu_read_lock){....}-{1:2}, at: bond_set_rx_mode+0x5/0x3b0 [bonding]
[ 126.759688][ T1318]
[ ... ]
> In commit 1a33e10e4a95 ("net: partially revert dynamic lockdep key
> changes"), I already reverted most parts of commit ab92d68fc22f
> ("net: core: add generic lockdep keys").
>
> This patch reverts the rest and also part of commit f3b0a18bb6cb
> ("net: remove unnecessary variables and callback"). After this
> patch, addr_list_lock changes back to using static keys and
> subclasses to satisfy lockdep. Thanks to dev->lower_level, we do
> not have to change back to ->ndo_get_lock_subclass().
>
> And hopefully this reduces some syzbot lockdep noises too.
>
> Reported-by: syzbot+f3a0e80c34b3fc28ac5e@syzkaller.appspotmail.com
> Cc: Taehee Yoo <ap420073@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> drivers/net/bonding/bond_main.c | 2 --
> drivers/net/bonding/bond_options.c | 2 --
> drivers/net/hamradio/bpqether.c | 2 ++
> drivers/net/macsec.c | 5 ++++
> drivers/net/macvlan.c | 13 ++++++--
> drivers/net/vxlan.c | 4 +--
> .../net/wireless/intersil/hostap/hostap_hw.c | 3 ++
> include/linux/netdevice.h | 12 +++++---
> net/8021q/vlan_dev.c | 8 +++--
> net/batman-adv/soft-interface.c | 2 ++
> net/bridge/br_device.c | 8 +++++
> net/core/dev.c | 30 ++++++++++---------
> net/core/dev_addr_lists.c | 12 ++++----
> net/core/rtnetlink.c | 1 -
> net/dsa/master.c | 4 +++
> net/netrom/af_netrom.c | 2 ++
> net/rose/af_rose.c | 2 ++
> 17 files changed, 76 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index a25c65d4af71..004919aea5fb 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3687,8 +3687,6 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
> case BOND_RELEASE_OLD:
> case SIOCBONDRELEASE:
> res = bond_release(bond_dev, slave_dev);
> - if (!res)
> - netdev_update_lockdep_key(slave_dev);
> break;
> case BOND_SETHWADDR_OLD:
> case SIOCBONDSETHWADDR:
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 215c10923289..ddb3916d3506 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -1398,8 +1398,6 @@ static int bond_option_slaves_set(struct bonding *bond,
> case '-':
> slave_dbg(bond->dev, dev, "Releasing interface\n");
> ret = bond_release(bond->dev, dev);
> - if (!ret)
> - netdev_update_lockdep_key(dev);
> break;
>
> default:
> diff --git a/drivers/net/hamradio/bpqether.c b/drivers/net/hamradio/bpqether.c
> index 60dcaf2a04a9..1ad6085994b1 100644
> --- a/drivers/net/hamradio/bpqether.c
> +++ b/drivers/net/hamradio/bpqether.c
> @@ -113,6 +113,7 @@ static LIST_HEAD(bpq_devices);
> * off into a separate class since they always nest.
> */
> static struct lock_class_key bpq_netdev_xmit_lock_key;
> +static struct lock_class_key bpq_netdev_addr_lock_key;
>
> static void bpq_set_lockdep_class_one(struct net_device *dev,
> struct netdev_queue *txq,
> @@ -123,6 +124,7 @@ static void bpq_set_lockdep_class_one(struct net_device *dev,
>
> static void bpq_set_lockdep_class(struct net_device *dev)
> {
> + lockdep_set_class(&dev->addr_list_lock, &bpq_netdev_addr_lock_key);
> netdev_for_each_tx_queue(dev, bpq_set_lockdep_class_one, NULL);
> }
>
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 20b53e255f68..e56547bfdac9 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -3999,6 +3999,8 @@ static int macsec_add_dev(struct net_device *dev, sci_t sci, u8 icv_len)
> return 0;
> }
>
> +static struct lock_class_key macsec_netdev_addr_lock_key;
> +
> static int macsec_newlink(struct net *net, struct net_device *dev,
> struct nlattr *tb[], struct nlattr *data[],
> struct netlink_ext_ack *extack)
> @@ -4050,6 +4052,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
> return err;
>
> netdev_lockdep_set_classes(dev);
> + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> + &macsec_netdev_addr_lock_key,
> + dev->lower_level);
>
> err = netdev_upper_dev_link(real_dev, dev, extack);
> if (err < 0)
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 563aed5b3d9f..6a6cc9f75307 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -860,6 +860,8 @@ static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> * "super class" of normal network devices; split their locks off into a
> * separate class since they always nest.
> */
> +static struct lock_class_key macvlan_netdev_addr_lock_key;
> +
> #define ALWAYS_ON_OFFLOADS \
> (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \
> NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL)
> @@ -875,6 +877,14 @@ static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> #define MACVLAN_STATE_MASK \
> ((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
>
> +static void macvlan_set_lockdep_class(struct net_device *dev)
> +{
> + netdev_lockdep_set_classes(dev);
> + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> + &macvlan_netdev_addr_lock_key,
> + dev->lower_level);
> +}
> +
> static int macvlan_init(struct net_device *dev)
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
> @@ -892,8 +902,7 @@ static int macvlan_init(struct net_device *dev)
> dev->gso_max_size = lowerdev->gso_max_size;
> dev->gso_max_segs = lowerdev->gso_max_segs;
> dev->hard_header_len = lowerdev->hard_header_len;
> -
> - netdev_lockdep_set_classes(dev);
> + macvlan_set_lockdep_class(dev);
>
> vlan->pcpu_stats = netdev_alloc_pcpu_stats(struct vlan_pcpu_stats);
> if (!vlan->pcpu_stats)
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 5bb448ae6c9c..47424b2da643 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -4245,10 +4245,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> mod_timer(&vxlan->age_timer, jiffies);
>
> netdev_adjacent_change_commit(dst->remote_dev, lowerdev, dev);
> - if (lowerdev && lowerdev != dst->remote_dev) {
> + if (lowerdev && lowerdev != dst->remote_dev)
> dst->remote_dev = lowerdev;
> - netdev_update_lockdep_key(lowerdev);
> - }
> vxlan_config_apply(dev, &conf, lowerdev, vxlan->net, true);
> return 0;
> }
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_hw.c b/drivers/net/wireless/intersil/hostap/hostap_hw.c
> index aadf3dec5bf3..2ab34cf74ecc 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_hw.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_hw.c
> @@ -3048,6 +3048,7 @@ static void prism2_clear_set_tim_queue(local_info_t *local)
> * This is a natural nesting, which needs a split lock type.
> */
> static struct lock_class_key hostap_netdev_xmit_lock_key;
> +static struct lock_class_key hostap_netdev_addr_lock_key;
>
> static void prism2_set_lockdep_class_one(struct net_device *dev,
> struct netdev_queue *txq,
> @@ -3059,6 +3060,8 @@ static void prism2_set_lockdep_class_one(struct net_device *dev,
>
> static void prism2_set_lockdep_class(struct net_device *dev)
> {
> + lockdep_set_class(&dev->addr_list_lock,
> + &hostap_netdev_addr_lock_key);
> netdev_for_each_tx_queue(dev, prism2_set_lockdep_class_one, NULL);
> }
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 1a96e9c4ec36..e2825e27ef89 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1821,8 +1821,6 @@ enum netdev_priv_flags {
> * for hardware timestamping
> * @sfp_bus: attached &struct sfp_bus structure.
> *
> - * @addr_list_lock_key: lockdep class annotating
> - * net_device->addr_list_lock spinlock
> * @qdisc_tx_busylock: lockdep class annotating Qdisc->busylock spinlock
> * @qdisc_running_key: lockdep class annotating Qdisc->running seqcount
> *
> @@ -2125,7 +2123,6 @@ struct net_device {
> #endif
> struct phy_device *phydev;
> struct sfp_bus *sfp_bus;
> - struct lock_class_key addr_list_lock_key;
> struct lock_class_key *qdisc_tx_busylock;
> struct lock_class_key *qdisc_running_key;
> bool proto_down;
> @@ -2217,10 +2214,13 @@ static inline void netdev_for_each_tx_queue(struct net_device *dev,
> static struct lock_class_key qdisc_tx_busylock_key; \
> static struct lock_class_key qdisc_running_key; \
> static struct lock_class_key qdisc_xmit_lock_key; \
> + static struct lock_class_key dev_addr_list_lock_key; \
> unsigned int i; \
> \
> (dev)->qdisc_tx_busylock = &qdisc_tx_busylock_key; \
> (dev)->qdisc_running_key = &qdisc_running_key; \
> + lockdep_set_class(&(dev)->addr_list_lock, \
> + &dev_addr_list_lock_key); \
> for (i = 0; i < (dev)->num_tx_queues; i++) \
> lockdep_set_class(&(dev)->_tx[i]._xmit_lock, \
> &qdisc_xmit_lock_key); \
> @@ -3253,7 +3253,6 @@ static inline void netif_stop_queue(struct net_device *dev)
> }
>
> void netif_tx_stop_all_queues(struct net_device *dev);
> -void netdev_update_lockdep_key(struct net_device *dev);
>
> static inline bool netif_tx_queue_stopped(const struct netdev_queue *dev_queue)
> {
> @@ -4239,6 +4238,11 @@ static inline void netif_addr_lock(struct net_device *dev)
> spin_lock(&dev->addr_list_lock);
> }
>
> +static inline void netif_addr_lock_nested(struct net_device *dev)
> +{
> + spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
> +}
> +
> static inline void netif_addr_lock_bh(struct net_device *dev)
> {
> spin_lock_bh(&dev->addr_list_lock);
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index f00bb57f0f60..c8d6a07e23c5 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -494,6 +494,7 @@ static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
> * separate class since they always nest.
> */
> static struct lock_class_key vlan_netdev_xmit_lock_key;
> +static struct lock_class_key vlan_netdev_addr_lock_key;
>
> static void vlan_dev_set_lockdep_one(struct net_device *dev,
> struct netdev_queue *txq,
> @@ -502,8 +503,11 @@ static void vlan_dev_set_lockdep_one(struct net_device *dev,
> lockdep_set_class(&txq->_xmit_lock, &vlan_netdev_xmit_lock_key);
> }
>
> -static void vlan_dev_set_lockdep_class(struct net_device *dev)
> +static void vlan_dev_set_lockdep_class(struct net_device *dev, int subclass)
> {
> + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> + &vlan_netdev_addr_lock_key,
> + subclass);
> netdev_for_each_tx_queue(dev, vlan_dev_set_lockdep_one, NULL);
> }
>
> @@ -597,7 +601,7 @@ static int vlan_dev_init(struct net_device *dev)
>
> SET_NETDEV_DEVTYPE(dev, &vlan_type);
>
> - vlan_dev_set_lockdep_class(dev);
> + vlan_dev_set_lockdep_class(dev, dev->lower_level);
>
> vlan->vlan_pcpu_stats = netdev_alloc_pcpu_stats(struct vlan_pcpu_stats);
> if (!vlan->vlan_pcpu_stats)
> diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
> index 0ddd80130ea3..f1f1c86f3419 100644
> --- a/net/batman-adv/soft-interface.c
> +++ b/net/batman-adv/soft-interface.c
> @@ -745,6 +745,7 @@ static int batadv_interface_kill_vid(struct net_device *dev, __be16 proto,
> * separate class since they always nest.
> */
> static struct lock_class_key batadv_netdev_xmit_lock_key;
> +static struct lock_class_key batadv_netdev_addr_lock_key;
>
> /**
> * batadv_set_lockdep_class_one() - Set lockdep class for a single tx queue
> @@ -765,6 +766,7 @@ static void batadv_set_lockdep_class_one(struct net_device *dev,
> */
> static void batadv_set_lockdep_class(struct net_device *dev)
> {
> + lockdep_set_class(&dev->addr_list_lock, &batadv_netdev_addr_lock_key);
> netdev_for_each_tx_queue(dev, batadv_set_lockdep_class_one, NULL);
> }
>
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 8ec1362588af..8c7b78f8bc23 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -105,6 +105,13 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> return NETDEV_TX_OK;
> }
>
> +static struct lock_class_key bridge_netdev_addr_lock_key;
> +
> +static void br_set_lockdep_class(struct net_device *dev)
> +{
> + lockdep_set_class(&dev->addr_list_lock, &bridge_netdev_addr_lock_key);
> +}
> +
> static int br_dev_init(struct net_device *dev)
> {
> struct net_bridge *br = netdev_priv(dev);
> @@ -143,6 +150,7 @@ static int br_dev_init(struct net_device *dev)
> br_fdb_hash_fini(br);
> }
>
> + br_set_lockdep_class(dev);
> return err;
> }
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 061496a1f640..6bc2388141f6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -439,6 +439,7 @@ static const char *const netdev_lock_name[] = {
> "_xmit_IEEE802154", "_xmit_VOID", "_xmit_NONE"};
>
> static struct lock_class_key netdev_xmit_lock_key[ARRAY_SIZE(netdev_lock_type)];
> +static struct lock_class_key netdev_addr_lock_key[ARRAY_SIZE(netdev_lock_type)];
>
> static inline unsigned short netdev_lock_pos(unsigned short dev_type)
> {
> @@ -460,11 +461,25 @@ static inline void netdev_set_xmit_lockdep_class(spinlock_t *lock,
> lockdep_set_class_and_name(lock, &netdev_xmit_lock_key[i],
> netdev_lock_name[i]);
> }
> +
> +static inline void netdev_set_addr_lockdep_class(struct net_device *dev)
> +{
> + int i;
> +
> + i = netdev_lock_pos(dev->type);
> + lockdep_set_class_and_name(&dev->addr_list_lock,
> + &netdev_addr_lock_key[i],
> + netdev_lock_name[i]);
> +}
> #else
> static inline void netdev_set_xmit_lockdep_class(spinlock_t *lock,
> unsigned short dev_type)
> {
> }
> +
> +static inline void netdev_set_addr_lockdep_class(struct net_device *dev)
> +{
> +}
> #endif
>
> /*******************************************************************************
> @@ -9373,15 +9388,6 @@ void netif_tx_stop_all_queues(struct net_device *dev)
> }
> EXPORT_SYMBOL(netif_tx_stop_all_queues);
>
> -void netdev_update_lockdep_key(struct net_device *dev)
> -{
> - lockdep_unregister_key(&dev->addr_list_lock_key);
> - lockdep_register_key(&dev->addr_list_lock_key);
> -
> - lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
> -}
> -EXPORT_SYMBOL(netdev_update_lockdep_key);
> -
> /**
> * register_netdevice - register a network device
> * @dev: device to register
> @@ -9420,7 +9426,7 @@ int register_netdevice(struct net_device *dev)
> return ret;
>
> spin_lock_init(&dev->addr_list_lock);
> - lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
> + netdev_set_addr_lockdep_class(dev);
>
> ret = dev_get_valid_name(net, dev, dev->name);
> if (ret < 0)
> @@ -9939,8 +9945,6 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>
> dev_net_set(dev, &init_net);
>
> - lockdep_register_key(&dev->addr_list_lock_key);
> -
> dev->gso_max_size = GSO_MAX_SIZE;
> dev->gso_max_segs = GSO_MAX_SEGS;
> dev->upper_level = 1;
> @@ -10028,8 +10032,6 @@ void free_netdev(struct net_device *dev)
> free_percpu(dev->xdp_bulkq);
> dev->xdp_bulkq = NULL;
>
> - lockdep_unregister_key(&dev->addr_list_lock_key);
> -
> /* Compatibility with error handling in drivers */
> if (dev->reg_state == NETREG_UNINITIALIZED) {
> netdev_freemem(dev);
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index 2f949b5a1eb9..6393ba930097 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -637,7 +637,7 @@ int dev_uc_sync(struct net_device *to, struct net_device *from)
> if (to->addr_len != from->addr_len)
> return -EINVAL;
>
> - netif_addr_lock(to);
> + netif_addr_lock_nested(to);
> err = __hw_addr_sync(&to->uc, &from->uc, to->addr_len);
> if (!err)
> __dev_set_rx_mode(to);
> @@ -667,7 +667,7 @@ int dev_uc_sync_multiple(struct net_device *to, struct net_device *from)
> if (to->addr_len != from->addr_len)
> return -EINVAL;
>
> - netif_addr_lock(to);
> + netif_addr_lock_nested(to);
> err = __hw_addr_sync_multiple(&to->uc, &from->uc, to->addr_len);
> if (!err)
> __dev_set_rx_mode(to);
> @@ -691,7 +691,7 @@ void dev_uc_unsync(struct net_device *to, struct net_device *from)
> return;
>
> netif_addr_lock_bh(from);
> - netif_addr_lock(to);
> + netif_addr_lock_nested(to);
> __hw_addr_unsync(&to->uc, &from->uc, to->addr_len);
> __dev_set_rx_mode(to);
> netif_addr_unlock(to);
> @@ -858,7 +858,7 @@ int dev_mc_sync(struct net_device *to, struct net_device *from)
> if (to->addr_len != from->addr_len)
> return -EINVAL;
>
> - netif_addr_lock(to);
> + netif_addr_lock_nested(to);
> err = __hw_addr_sync(&to->mc, &from->mc, to->addr_len);
> if (!err)
> __dev_set_rx_mode(to);
> @@ -888,7 +888,7 @@ int dev_mc_sync_multiple(struct net_device *to, struct net_device *from)
> if (to->addr_len != from->addr_len)
> return -EINVAL;
>
> - netif_addr_lock(to);
> + netif_addr_lock_nested(to);
> err = __hw_addr_sync_multiple(&to->mc, &from->mc, to->addr_len);
> if (!err)
> __dev_set_rx_mode(to);
> @@ -912,7 +912,7 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from)
> return;
>
> netif_addr_lock_bh(from);
> - netif_addr_lock(to);
> + netif_addr_lock_nested(to);
> __hw_addr_unsync(&to->mc, &from->mc, to->addr_len);
> __dev_set_rx_mode(to);
> netif_addr_unlock(to);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 2269199c5891..9aedc15736ad 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2462,7 +2462,6 @@ static int do_set_master(struct net_device *dev, int ifindex,
> err = ops->ndo_del_slave(upper_dev, dev);
> if (err)
> return err;
> - netdev_update_lockdep_key(dev);
> } else {
> return -EOPNOTSUPP;
> }
> diff --git a/net/dsa/master.c b/net/dsa/master.c
> index a621367c6e8c..480a61460c23 100644
> --- a/net/dsa/master.c
> +++ b/net/dsa/master.c
> @@ -327,6 +327,8 @@ static void dsa_master_reset_mtu(struct net_device *dev)
> rtnl_unlock();
> }
>
> +static struct lock_class_key dsa_master_addr_list_lock_key;
> +
> int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
> {
> int ret;
> @@ -345,6 +347,8 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
> wmb();
>
> dev->dsa_ptr = cpu_dp;
> + lockdep_set_class(&dev->addr_list_lock,
> + &dsa_master_addr_list_lock_key);
> ret = dsa_master_ethtool_setup(dev);
> if (ret)
> return ret;
> diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
> index eccc7d366e17..f90ef6934b8f 100644
> --- a/net/netrom/af_netrom.c
> +++ b/net/netrom/af_netrom.c
> @@ -70,6 +70,7 @@ static const struct proto_ops nr_proto_ops;
> * separate class since they always nest.
> */
> static struct lock_class_key nr_netdev_xmit_lock_key;
> +static struct lock_class_key nr_netdev_addr_lock_key;
>
> static void nr_set_lockdep_one(struct net_device *dev,
> struct netdev_queue *txq,
> @@ -80,6 +81,7 @@ static void nr_set_lockdep_one(struct net_device *dev,
>
> static void nr_set_lockdep_key(struct net_device *dev)
> {
> + lockdep_set_class(&dev->addr_list_lock, &nr_netdev_addr_lock_key);
> netdev_for_each_tx_queue(dev, nr_set_lockdep_one, NULL);
> }
>
> diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
> index e7a872207b46..ce85656ac9c1 100644
> --- a/net/rose/af_rose.c
> +++ b/net/rose/af_rose.c
> @@ -71,6 +71,7 @@ ax25_address rose_callsign;
> * separate class since they always nest.
> */
> static struct lock_class_key rose_netdev_xmit_lock_key;
> +static struct lock_class_key rose_netdev_addr_lock_key;
>
> static void rose_set_lockdep_one(struct net_device *dev,
> struct netdev_queue *txq,
> @@ -81,6 +82,7 @@ static void rose_set_lockdep_one(struct net_device *dev,
>
> static void rose_set_lockdep_key(struct net_device *dev)
> {
> + lockdep_set_class(&dev->addr_list_lock, &rose_netdev_addr_lock_key);
> netdev_for_each_tx_queue(dev, rose_set_lockdep_one, NULL);
> }
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-10 14:48 ` Taehee Yoo
@ 2020-06-10 23:21 ` Cong Wang
2020-06-13 16:03 ` Taehee Yoo
0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2020-06-10 23:21 UTC (permalink / raw)
To: Taehee Yoo; +Cc: Netdev, syzbot+f3a0e80c34b3fc28ac5e, Dmitry Vyukov
On Wed, Jun 10, 2020 at 7:48 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
>
> Hi Cong,
> Thank you for this work!
>
> > The dynamic key update for addr_list_lock still causes troubles,
> > for example the following race condition still exists:
> >
> > CPU 0: CPU 1:
> > (RCU read lock) (RTNL lock)
> > dev_mc_seq_show() netdev_update_lockdep_key()
> > -> lockdep_unregister_key()
> > -> netif_addr_lock_bh()
> >
> > because lockdep doesn't provide an API to update it atomically.
> > Therefore, we have to move it back to static keys and use subclass
> > for nest locking like before.
> >
>
> I'm sorry for the late reply.
> I agree that using subclass mechanism to avoid too many lockdep keys.
Avoiding too many lockdep keys is not the real goal of my patch,
its main purpose is to fix a race condition shown above. Just FYI.
> But the subclass mechanism is also not updated its subclass key
> automatically. So, if upper/lower relationship is changed,
> interface would have incorrect subclass key.
> It eventually results in lockdep warning.
So dev->lower_level is not updated accordingly? I just blindly trust
dev->lower_level, as you use it in other places too.
> And, I think this patch doesn't contain bonding and team module part.
> So, an additional patch is needed.
Hmm, dev->lower_level is generic, so is addr_list_lock.
Again, I just assume you already update dev->lower_level each time
the topology changes. I added some printk() to verify it too for my
simple bond over bond case. So, I can't immediately see what is
wrong with dev->lower_level here. Do you mind to be more specific?
Or I misunderstand your point?
Thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-10 23:21 ` Cong Wang
@ 2020-06-13 16:03 ` Taehee Yoo
2020-06-15 21:33 ` Cong Wang
0 siblings, 1 reply; 17+ messages in thread
From: Taehee Yoo @ 2020-06-13 16:03 UTC (permalink / raw)
To: Cong Wang; +Cc: Netdev, syzbot+f3a0e80c34b3fc28ac5e, Dmitry Vyukov
On Thu, 11 Jun 2020 at 08:21, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
Hi Cong :)
> On Wed, Jun 10, 2020 at 7:48 AM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> >
> > Hi Cong,
> > Thank you for this work!
> >
> > > The dynamic key update for addr_list_lock still causes troubles,
> > > for example the following race condition still exists:
> > >
> > > CPU 0: CPU 1:
> > > (RCU read lock) (RTNL lock)
> > > dev_mc_seq_show() netdev_update_lockdep_key()
> > > -> lockdep_unregister_key()
> > > -> netif_addr_lock_bh()
> > >
> > > because lockdep doesn't provide an API to update it atomically.
> > > Therefore, we have to move it back to static keys and use subclass
> > > for nest locking like before.
> > >
> >
> > I'm sorry for the late reply.
> > I agree that using subclass mechanism to avoid too many lockdep keys.
>
> Avoiding too many lockdep keys is not the real goal of my patch,
> its main purpose is to fix a race condition shown above. Just FYI.
>
Thank you for notifying me.
>
> > But the subclass mechanism is also not updated its subclass key
> > automatically. So, if upper/lower relationship is changed,
> > interface would have incorrect subclass key.
> > It eventually results in lockdep warning.
>
> So dev->lower_level is not updated accordingly? I just blindly trust
> dev->lower_level, as you use it in other places too.
>
> > And, I think this patch doesn't contain bonding and team module part.
> > So, an additional patch is needed.
>
> Hmm, dev->lower_level is generic, so is addr_list_lock.
>
> Again, I just assume you already update dev->lower_level each time
> the topology changes. I added some printk() to verify it too for my
> simple bond over bond case. So, I can't immediately see what is
> wrong with dev->lower_level here. Do you mind to be more specific?
> Or I misunderstand your point?
>
> > > + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> > > + &vlan_netdev_addr_lock_key,
> > > + subclass);
In this patch, lockdep_set_class_and_subclass() is used.
As far as I know, this function initializes lockdep key and subclass
value with a given variable.
A dev->lower_level variable is used as a subclass value in this patch.
When dev->lower_level value is changed, the subclass value of this
lockdep key is not changed automatically.
If this value has to be changed, additional function is needed.
>>> netif_addr_lock_bh(from);
In this function, internally spin_lock_bh() is used and this function
might use an 'initialized subclass value' not a current dev->lower_level.
At this point, I think the lockdep splat might occur.
+static inline void netif_addr_lock_nested(struct net_device *dev)
+{
+ spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
+}
In this patch, you used netif_addr_lock_nested() too.
These two subclass values could be different.
But I'm not sure whether using spin_lock_nested with two different
subclass values are the right way or not.
If I misunderstood the lockdep and this logic, please let me know!
Thanks :)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-13 16:03 ` Taehee Yoo
@ 2020-06-15 21:33 ` Cong Wang
2020-06-16 15:03 ` Taehee Yoo
0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2020-06-15 21:33 UTC (permalink / raw)
To: Taehee Yoo; +Cc: Netdev, syzbot+f3a0e80c34b3fc28ac5e, Dmitry Vyukov
On Sat, Jun 13, 2020 at 9:03 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Thu, 11 Jun 2020 at 08:21, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
>
> Hi Cong :)
>
> > On Wed, Jun 10, 2020 at 7:48 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > >
> > > On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> > > > + &vlan_netdev_addr_lock_key,
> > > > + subclass);
>
> In this patch, lockdep_set_class_and_subclass() is used.
> As far as I know, this function initializes lockdep key and subclass
> value with a given variable.
> A dev->lower_level variable is used as a subclass value in this patch.
> When dev->lower_level value is changed, the subclass value of this
> lockdep key is not changed automatically.
> If this value has to be changed, additional function is needed.
Hmm, but we pass a dynamic subclass to spin_lock_nested().
So I guess I should just remove all the
lockdep_set_class_and_subclass() and leave subclass to 0?
>
> >>> netif_addr_lock_bh(from);
> In this function, internally spin_lock_bh() is used and this function
> might use an 'initialized subclass value' not a current dev->lower_level.
> At this point, I think the lockdep splat might occur.
>
> +static inline void netif_addr_lock_nested(struct net_device *dev)
> +{
> + spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
> +}
> In this patch, you used netif_addr_lock_nested() too.
> These two subclass values could be different.
> But I'm not sure whether using spin_lock_nested with two different
> subclass values are the right way or not.
Yeah, as long as dev->lower_level is different, it should be different
subclass. I assume dev->lower_level is automatically adjusted
whenever the topology changes, like the vlan over bond case above.
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-15 21:33 ` Cong Wang
@ 2020-06-16 15:03 ` Taehee Yoo
2020-06-17 17:42 ` Cong Wang
0 siblings, 1 reply; 17+ messages in thread
From: Taehee Yoo @ 2020-06-16 15:03 UTC (permalink / raw)
To: Cong Wang; +Cc: Netdev, syzbot+f3a0e80c34b3fc28ac5e, Dmitry Vyukov
On Tue, 16 Jun 2020 at 06:33, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
Hi Cong,
> On Sat, Jun 13, 2020 at 9:03 AM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > On Thu, 11 Jun 2020 at 08:21, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> >
> > Hi Cong :)
> >
> > > On Wed, Jun 10, 2020 at 7:48 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > >
> > > > On Tue, 9 Jun 2020 at 06:53, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > + lockdep_set_class_and_subclass(&dev->addr_list_lock,
> > > > > + &vlan_netdev_addr_lock_key,
> > > > > + subclass);
> >
> > In this patch, lockdep_set_class_and_subclass() is used.
> > As far as I know, this function initializes lockdep key and subclass
> > value with a given variable.
> > A dev->lower_level variable is used as a subclass value in this patch.
> > When dev->lower_level value is changed, the subclass value of this
> > lockdep key is not changed automatically.
> > If this value has to be changed, additional function is needed.
>
> Hmm, but we pass a dynamic subclass to spin_lock_nested().
>
> So I guess I should just remove all the
> lockdep_set_class_and_subclass() and leave subclass to 0?
>
I agree with that.
And, do you have any plan to replace netif_addr_lock_bh() with
netif_addr_lock_nested()?
(Of course, it needs BH handling code)
I'm not sure but I think it would be needed.
> >
> > >>> netif_addr_lock_bh(from);
> > In this function, internally spin_lock_bh() is used and this function
> > might use an 'initialized subclass value' not a current dev->lower_level.
> > At this point, I think the lockdep splat might occur.
> >
> > +static inline void netif_addr_lock_nested(struct net_device *dev)
> > +{
> > + spin_lock_nested(&dev->addr_list_lock, dev->lower_level);
> > +}
> > In this patch, you used netif_addr_lock_nested() too.
> > These two subclass values could be different.
> > But I'm not sure whether using spin_lock_nested with two different
> > subclass values are the right way or not.
>
> Yeah, as long as dev->lower_level is different, it should be different
> subclass. I assume dev->lower_level is automatically adjusted
> whenever the topology changes, like the vlan over bond case above.
>
Yes, you're right.
Thanks!
Taehee Yoo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-16 15:03 ` Taehee Yoo
@ 2020-06-17 17:42 ` Cong Wang
0 siblings, 0 replies; 17+ messages in thread
From: Cong Wang @ 2020-06-17 17:42 UTC (permalink / raw)
To: Taehee Yoo; +Cc: Netdev, syzbot+f3a0e80c34b3fc28ac5e, Dmitry Vyukov
On Tue, Jun 16, 2020 at 8:03 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> I agree with that.
> And, do you have any plan to replace netif_addr_lock_bh() with
> netif_addr_lock_nested()?
> (Of course, it needs BH handling code)
> I'm not sure but I think it would be needed.
Yeah, I agree it's needed. I have a patch now and will send it
out after some testing.
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-08 21:53 [Patch net] net: change addr_list_lock back to static key Cong Wang
2020-06-09 20:00 ` David Miller
2020-06-10 14:48 ` Taehee Yoo
@ 2020-06-18 19:40 ` Vladimir Oltean
2020-06-18 19:56 ` Cong Wang
2 siblings, 1 reply; 17+ messages in thread
From: Vladimir Oltean @ 2020-06-18 19:40 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo, Dmitry Vyukov
Hi Cong, Taehee,
On Tue, 9 Jun 2020 at 00:54, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> The dynamic key update for addr_list_lock still causes troubles,
> for example the following race condition still exists:
>
> CPU 0: CPU 1:
> (RCU read lock) (RTNL lock)
> dev_mc_seq_show() netdev_update_lockdep_key()
> -> lockdep_unregister_key()
> -> netif_addr_lock_bh()
>
> because lockdep doesn't provide an API to update it atomically.
> Therefore, we have to move it back to static keys and use subclass
> for nest locking like before.
>
> In commit 1a33e10e4a95 ("net: partially revert dynamic lockdep key
> changes"), I already reverted most parts of commit ab92d68fc22f
> ("net: core: add generic lockdep keys").
>
> This patch reverts the rest and also part of commit f3b0a18bb6cb
> ("net: remove unnecessary variables and callback"). After this
> patch, addr_list_lock changes back to using static keys and
> subclasses to satisfy lockdep. Thanks to dev->lower_level, we do
> not have to change back to ->ndo_get_lock_subclass().
>
> And hopefully this reduces some syzbot lockdep noises too.
>
> Reported-by: syzbot+f3a0e80c34b3fc28ac5e@syzkaller.appspotmail.com
> Cc: Taehee Yoo <ap420073@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> drivers/net/bonding/bond_main.c | 2 --
> drivers/net/bonding/bond_options.c | 2 --
> drivers/net/hamradio/bpqether.c | 2 ++
> drivers/net/macsec.c | 5 ++++
> drivers/net/macvlan.c | 13 ++++++--
> drivers/net/vxlan.c | 4 +--
> .../net/wireless/intersil/hostap/hostap_hw.c | 3 ++
> include/linux/netdevice.h | 12 +++++---
> net/8021q/vlan_dev.c | 8 +++--
> net/batman-adv/soft-interface.c | 2 ++
> net/bridge/br_device.c | 8 +++++
> net/core/dev.c | 30 ++++++++++---------
> net/core/dev_addr_lists.c | 12 ++++----
> net/core/rtnetlink.c | 1 -
> net/dsa/master.c | 4 +++
> net/netrom/af_netrom.c | 2 ++
> net/rose/af_rose.c | 2 ++
> 17 files changed, 76 insertions(+), 36 deletions(-)
>
It's me with the stacked DSA devices again:
[ 11.424642] ============================================
[ 11.429967] WARNING: possible recursive locking detected
[ 11.435295] 5.8.0-rc1-00133-g923e4b5032dd-dirty #208 Not tainted
[ 11.441319] --------------------------------------------
[ 11.446646] dhcpcd/323 is trying to acquire lock:
[ 11.451362] ffff000066dd4268
(&dsa_master_addr_list_lock_key/1){+...}-{2:2}, at:
dev_mc_sync+0x44/0x90
[ 11.460713]
[ 11.460713] but task is already holding lock:
[ 11.466561] ffff00006608c268
(&dsa_master_addr_list_lock_key/1){+...}-{2:2}, at:
dev_mc_sync+0x44/0x90
[ 11.475905]
[ 11.475905] other info that might help us debug this:
[ 11.482450] Possible unsafe locking scenario:
[ 11.482450]
[ 11.488386] CPU0
[ 11.490833] ----
[ 11.493280] lock(&dsa_master_addr_list_lock_key/1);
[ 11.498347] lock(&dsa_master_addr_list_lock_key/1);
[ 11.503413]
[ 11.503413] *** DEADLOCK ***
[ 11.503413]
[ 11.509349] May be due to missing lock nesting notation
[ 11.509349]
[ 11.516158] 3 locks held by dhcpcd/323:
[ 11.520001] #0: ffffdbd1381dda18 (rtnl_mutex){+.+.}-{3:3}, at:
rtnl_lock+0x24/0x30
[ 11.527688] #1: ffff00006614b268 (_xmit_ETHER){+...}-{2:2}, at:
dev_set_rx_mode+0x28/0x48
[ 11.535987] #2: ffff00006608c268
(&dsa_master_addr_list_lock_key/1){+...}-{2:2}, at:
dev_mc_sync+0x44/0x90
[ 11.545766]
[ 11.545766] stack backtrace:
[ 11.564098] Call trace:
[ 11.566549] dump_backtrace+0x0/0x1e0
[ 11.570220] show_stack+0x20/0x30
[ 11.573544] dump_stack+0xec/0x158
[ 11.576955] __lock_acquire+0xca0/0x2398
[ 11.580886] lock_acquire+0xe8/0x440
[ 11.584469] _raw_spin_lock_nested+0x64/0x90
[ 11.588749] dev_mc_sync+0x44/0x90
[ 11.592159] dsa_slave_set_rx_mode+0x34/0x50
[ 11.596438] __dev_set_rx_mode+0x60/0xa0
[ 11.600369] dev_mc_sync+0x84/0x90
[ 11.603778] dsa_slave_set_rx_mode+0x34/0x50
[ 11.608057] __dev_set_rx_mode+0x60/0xa0
[ 11.611989] dev_set_rx_mode+0x30/0x48
[ 11.615745] __dev_open+0x10c/0x180
[ 11.619240] __dev_change_flags+0x170/0x1c8
[ 11.623432] dev_change_flags+0x2c/0x70
[ 11.627279] devinet_ioctl+0x774/0x878
[ 11.631036] inet_ioctl+0x348/0x3b0
[ 11.634532] sock_do_ioctl+0x50/0x310
[ 11.638202] sock_ioctl+0x1f8/0x580
[ 11.641698] ksys_ioctl+0xb0/0xf0
[ 11.645019] __arm64_sys_ioctl+0x28/0x38
[ 11.648951] el0_svc_common.constprop.0+0x7c/0x180
[ 11.653753] do_el0_svc+0x2c/0x98
[ 11.657075] el0_sync_handler+0x9c/0x1b8
[ 11.661005] el0_sync+0x158/0x180
Could you please share some suggestions?
Thanks,
-Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 19:40 ` Vladimir Oltean
@ 2020-06-18 19:56 ` Cong Wang
2020-06-18 20:06 ` Cong Wang
0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2020-06-18 19:56 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo, Dmitry Vyukov
On Thu, Jun 18, 2020 at 12:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> It's me with the stacked DSA devices again:
It looks like DSA never uses netdev API to link master
device with slave devices? If so, their dev->lower_level
are always 1, therefore triggers this warning.
I think it should call one of these netdev_upper_dev_link()
API's when creating a slave device.
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 19:56 ` Cong Wang
@ 2020-06-18 20:06 ` Cong Wang
2020-06-18 20:33 ` Vladimir Oltean
0 siblings, 1 reply; 17+ messages in thread
From: Cong Wang @ 2020-06-18 20:06 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo, Dmitry Vyukov
On Thu, Jun 18, 2020 at 12:56 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Jun 18, 2020 at 12:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > It's me with the stacked DSA devices again:
>
> It looks like DSA never uses netdev API to link master
> device with slave devices? If so, their dev->lower_level
> are always 1, therefore triggers this warning.
>
> I think it should call one of these netdev_upper_dev_link()
> API's when creating a slave device.
>
I don't know whether DSA is too special to use the API, but
something like this should work:
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 4c7f086a047b..f7a2a281e7f0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1807,6 +1807,11 @@ int dsa_slave_create(struct dsa_port *port)
ret, slave_dev->name);
goto out_phy;
}
+ ret = netdev_upper_dev_link(slave_dev, master, NULL);
+ if (ret) {
+ unregister_netdevice(slave_dev);
+ goto out_phy;
+ }
return 0;
@@ -1832,6 +1837,7 @@ void dsa_slave_destroy(struct net_device *slave_dev)
netif_carrier_off(slave_dev);
rtnl_lock();
phylink_disconnect_phy(dp->pl);
+ netdev_upper_dev_unlink(slave_dev, dp->master);
rtnl_unlock();
dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 20:06 ` Cong Wang
@ 2020-06-18 20:33 ` Vladimir Oltean
2020-06-18 20:59 ` Cong Wang
2020-06-18 22:29 ` Andrew Lunn
0 siblings, 2 replies; 17+ messages in thread
From: Vladimir Oltean @ 2020-06-18 20:33 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo, Dmitry Vyukov
On Thu, 18 Jun 2020 at 23:06, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Jun 18, 2020 at 12:56 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Thu, Jun 18, 2020 at 12:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > >
> > > It's me with the stacked DSA devices again:
> >
> > It looks like DSA never uses netdev API to link master
> > device with slave devices? If so, their dev->lower_level
> > are always 1, therefore triggers this warning.
> >
> > I think it should call one of these netdev_upper_dev_link()
> > API's when creating a slave device.
> >
>
> I don't know whether DSA is too special to use the API, but
> something like this should work:
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 4c7f086a047b..f7a2a281e7f0 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1807,6 +1807,11 @@ int dsa_slave_create(struct dsa_port *port)
> ret, slave_dev->name);
> goto out_phy;
> }
> + ret = netdev_upper_dev_link(slave_dev, master, NULL);
> + if (ret) {
> + unregister_netdevice(slave_dev);
> + goto out_phy;
> + }
>
> return 0;
>
> @@ -1832,6 +1837,7 @@ void dsa_slave_destroy(struct net_device *slave_dev)
> netif_carrier_off(slave_dev);
> rtnl_lock();
> phylink_disconnect_phy(dp->pl);
> + netdev_upper_dev_unlink(slave_dev, dp->master);
> rtnl_unlock();
>
> dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
Thanks. This is a good approximation of what needed to be done:
- netdev_upper_dev_link needs to be under rtnl,
- "dp->master" should be "dsa_slave_to_master(slave_dev)" since it's
actually a union if you look at struct dsa_port).
- And, most importantly, I think the hierarchy should be reversed: a
(virtual) DSA switch port net device (slave) should be an upper of the
(real) DSA master (the host port). Think of it like this: a DSA switch
is a sort of port multiplier for a host port, based on a frame header.
But, it works!
Do you mind if I submit your modified patch to "net"? What would be an
adequate Fixes: tag?
Cheers,
-Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 20:33 ` Vladimir Oltean
@ 2020-06-18 20:59 ` Cong Wang
2020-06-18 22:29 ` Andrew Lunn
1 sibling, 0 replies; 17+ messages in thread
From: Cong Wang @ 2020-06-18 20:59 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo, Dmitry Vyukov
On Thu, Jun 18, 2020 at 1:33 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Thu, 18 Jun 2020 at 23:06, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Thu, Jun 18, 2020 at 12:56 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 12:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > It's me with the stacked DSA devices again:
> > >
> > > It looks like DSA never uses netdev API to link master
> > > device with slave devices? If so, their dev->lower_level
> > > are always 1, therefore triggers this warning.
> > >
> > > I think it should call one of these netdev_upper_dev_link()
> > > API's when creating a slave device.
> > >
> >
> > I don't know whether DSA is too special to use the API, but
> > something like this should work:
> >
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > index 4c7f086a047b..f7a2a281e7f0 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -1807,6 +1807,11 @@ int dsa_slave_create(struct dsa_port *port)
> > ret, slave_dev->name);
> > goto out_phy;
> > }
> > + ret = netdev_upper_dev_link(slave_dev, master, NULL);
> > + if (ret) {
> > + unregister_netdevice(slave_dev);
> > + goto out_phy;
> > + }
> >
> > return 0;
> >
> > @@ -1832,6 +1837,7 @@ void dsa_slave_destroy(struct net_device *slave_dev)
> > netif_carrier_off(slave_dev);
> > rtnl_lock();
> > phylink_disconnect_phy(dp->pl);
> > + netdev_upper_dev_unlink(slave_dev, dp->master);
> > rtnl_unlock();
> >
> > dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
>
> Thanks. This is a good approximation of what needed to be done:
> - netdev_upper_dev_link needs to be under rtnl,
> - "dp->master" should be "dsa_slave_to_master(slave_dev)" since it's
> actually a union if you look at struct dsa_port).
> - And, most importantly, I think the hierarchy should be reversed: a
> (virtual) DSA switch port net device (slave) should be an upper of the
> (real) DSA master (the host port). Think of it like this: a DSA switch
> is a sort of port multiplier for a host port, based on a frame header.
> But, it works!
Please feel free to make any changes you need and submit it
by yourself, as you know DSA better than me and I do not even
have a DSA testing environment.
>
> Do you mind if I submit your modified patch to "net"? What would be an
> adequate Fixes: tag?
If it is merely to fix the lockdep warning, my commit 845e0ebb4408d447
is the right one to blame.
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 20:33 ` Vladimir Oltean
2020-06-18 20:59 ` Cong Wang
@ 2020-06-18 22:29 ` Andrew Lunn
2020-06-18 22:32 ` Vladimir Oltean
1 sibling, 1 reply; 17+ messages in thread
From: Andrew Lunn @ 2020-06-18 22:29 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Cong Wang, netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo,
Dmitry Vyukov
On Thu, Jun 18, 2020 at 11:33:44PM +0300, Vladimir Oltean wrote:
> On Thu, 18 Jun 2020 at 23:06, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Thu, Jun 18, 2020 at 12:56 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 12:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > >
> > > > It's me with the stacked DSA devices again:
> > >
> > > It looks like DSA never uses netdev API to link master
> > > device with slave devices? If so, their dev->lower_level
> > > are always 1, therefore triggers this warning.
> > >
> > > I think it should call one of these netdev_upper_dev_link()
> > > API's when creating a slave device.
> > >
> >
> > I don't know whether DSA is too special to use the API, but
> > something like this should work:
> >
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > index 4c7f086a047b..f7a2a281e7f0 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -1807,6 +1807,11 @@ int dsa_slave_create(struct dsa_port *port)
> > ret, slave_dev->name);
> > goto out_phy;
> > }
> > + ret = netdev_upper_dev_link(slave_dev, master, NULL);
> > + if (ret) {
> > + unregister_netdevice(slave_dev);
> > + goto out_phy;
> > + }
> >
> > return 0;
> >
> > @@ -1832,6 +1837,7 @@ void dsa_slave_destroy(struct net_device *slave_dev)
> > netif_carrier_off(slave_dev);
> > rtnl_lock();
> > phylink_disconnect_phy(dp->pl);
> > + netdev_upper_dev_unlink(slave_dev, dp->master);
> > rtnl_unlock();
> >
> > dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
>
> Thanks. This is a good approximation of what needed to be done:
> - netdev_upper_dev_link needs to be under rtnl,
> - "dp->master" should be "dsa_slave_to_master(slave_dev)" since it's
> actually a union if you look at struct dsa_port).
> - And, most importantly, I think the hierarchy should be reversed: a
> (virtual) DSA switch port net device (slave) should be an upper of the
> (real) DSA master (the host port). Think of it like this: a DSA switch
> is a sort of port multiplier for a host port, based on a frame header.
> But, it works!
Hi Vladimir
So you are suggesting this?
> > + ret = netdev_upper_dev_link(master, slave_dev, NULL);
Andrew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 22:29 ` Andrew Lunn
@ 2020-06-18 22:32 ` Vladimir Oltean
2020-06-18 22:46 ` Andrew Lunn
0 siblings, 1 reply; 17+ messages in thread
From: Vladimir Oltean @ 2020-06-18 22:32 UTC (permalink / raw)
To: Andrew Lunn
Cc: Cong Wang, netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo,
Dmitry Vyukov
On Fri, 19 Jun 2020 at 01:30, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, Jun 18, 2020 at 11:33:44PM +0300, Vladimir Oltean wrote:
> > On Thu, 18 Jun 2020 at 23:06, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Thu, Jun 18, 2020 at 12:56 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Thu, Jun 18, 2020 at 12:40 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> > > > >
> > > > > It's me with the stacked DSA devices again:
> > > >
> > > > It looks like DSA never uses netdev API to link master
> > > > device with slave devices? If so, their dev->lower_level
> > > > are always 1, therefore triggers this warning.
> > > >
> > > > I think it should call one of these netdev_upper_dev_link()
> > > > API's when creating a slave device.
> > > >
> > >
> > > I don't know whether DSA is too special to use the API, but
> > > something like this should work:
> > >
> > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > > index 4c7f086a047b..f7a2a281e7f0 100644
> > > --- a/net/dsa/slave.c
> > > +++ b/net/dsa/slave.c
> > > @@ -1807,6 +1807,11 @@ int dsa_slave_create(struct dsa_port *port)
> > > ret, slave_dev->name);
> > > goto out_phy;
> > > }
> > > + ret = netdev_upper_dev_link(slave_dev, master, NULL);
> > > + if (ret) {
> > > + unregister_netdevice(slave_dev);
> > > + goto out_phy;
> > > + }
> > >
> > > return 0;
> > >
> > > @@ -1832,6 +1837,7 @@ void dsa_slave_destroy(struct net_device *slave_dev)
> > > netif_carrier_off(slave_dev);
> > > rtnl_lock();
> > > phylink_disconnect_phy(dp->pl);
> > > + netdev_upper_dev_unlink(slave_dev, dp->master);
> > > rtnl_unlock();
> > >
> > > dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
> >
> > Thanks. This is a good approximation of what needed to be done:
> > - netdev_upper_dev_link needs to be under rtnl,
> > - "dp->master" should be "dsa_slave_to_master(slave_dev)" since it's
> > actually a union if you look at struct dsa_port).
>
> > - And, most importantly, I think the hierarchy should be reversed: a
> > (virtual) DSA switch port net device (slave) should be an upper of the
> > (real) DSA master (the host port). Think of it like this: a DSA switch
> > is a sort of port multiplier for a host port, based on a frame header.
> > But, it works!
>
> Hi Vladimir
>
> So you are suggesting this?
>
> > > + ret = netdev_upper_dev_link(master, slave_dev, NULL);
>
> Andrew
Yes, basically this:
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 4c7f086a047b..6aff8cfc9cf1 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1807,6 +1807,13 @@ int dsa_slave_create(struct dsa_port *port)
ret, slave_dev->name);
goto out_phy;
}
+ rtnl_lock();
+ ret = netdev_upper_dev_link(master, slave_dev, NULL);
+ rtnl_unlock();
+ if (ret) {
+ unregister_netdevice(slave_dev);
+ goto out_phy;
+ }
return 0;
@@ -1826,12 +1833,14 @@ int dsa_slave_create(struct dsa_port *port)
void dsa_slave_destroy(struct net_device *slave_dev)
{
+ struct net_device *master = dsa_slave_to_master(slave_dev);
struct dsa_port *dp = dsa_slave_to_port(slave_dev);
struct dsa_slave_priv *p = netdev_priv(slave_dev);
netif_carrier_off(slave_dev);
rtnl_lock();
phylink_disconnect_phy(dp->pl);
+ netdev_upper_dev_unlink(master, slave_dev);
rtnl_unlock();
dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
Do you see a problem with it?
Thanks,
-Vladimir
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 22:32 ` Vladimir Oltean
@ 2020-06-18 22:46 ` Andrew Lunn
2020-06-18 22:53 ` Vladimir Oltean
0 siblings, 1 reply; 17+ messages in thread
From: Andrew Lunn @ 2020-06-18 22:46 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Cong Wang, netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo,
Dmitry Vyukov
> > Hi Vladimir
> >
> > So you are suggesting this?
> >
> > > > + ret = netdev_upper_dev_link(master, slave_dev, NULL);
> >
> > Andrew
>
> Yes, basically this:
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 4c7f086a047b..6aff8cfc9cf1 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1807,6 +1807,13 @@ int dsa_slave_create(struct dsa_port *port)
> ret, slave_dev->name);
> goto out_phy;
> }
> + rtnl_lock();
> + ret = netdev_upper_dev_link(master, slave_dev, NULL);
> + rtnl_unlock();
> + if (ret) {
> + unregister_netdevice(slave_dev);
> + goto out_phy;
> + }
>
> return 0;
>
> @@ -1826,12 +1833,14 @@ int dsa_slave_create(struct dsa_port *port)
>
> void dsa_slave_destroy(struct net_device *slave_dev)
> {
> + struct net_device *master = dsa_slave_to_master(slave_dev);
> struct dsa_port *dp = dsa_slave_to_port(slave_dev);
> struct dsa_slave_priv *p = netdev_priv(slave_dev);
>
> netif_carrier_off(slave_dev);
> rtnl_lock();
> phylink_disconnect_phy(dp->pl);
> + netdev_upper_dev_unlink(master, slave_dev);
> rtnl_unlock();
>
> dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
>
> Do you see a problem with it?
I was initially not sure you could do this. But it looks like you can
have N : M relationships between uppers and lowers. I suppose it does
make sense. You can have multiple VLAN uppers to one base device. You
can have multiple lowers to one bond device, etc.
I wonder what 'side effects' there are for declaring this linkage. It
is not something i've looked into before, since we never used it. So i
don't see a problem with this, other than i don't know what problems
we might run into :-)
Andrew
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Patch net] net: change addr_list_lock back to static key
2020-06-18 22:46 ` Andrew Lunn
@ 2020-06-18 22:53 ` Vladimir Oltean
0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Oltean @ 2020-06-18 22:53 UTC (permalink / raw)
To: Andrew Lunn
Cc: Cong Wang, netdev, syzbot+f3a0e80c34b3fc28ac5e, Taehee Yoo,
Dmitry Vyukov
On Fri, 19 Jun 2020 at 01:46, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > Hi Vladimir
> > >
> > > So you are suggesting this?
> > >
> > > > > + ret = netdev_upper_dev_link(master, slave_dev, NULL);
> > >
> > > Andrew
> >
> > Yes, basically this:
> >
> > diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> > index 4c7f086a047b..6aff8cfc9cf1 100644
> > --- a/net/dsa/slave.c
> > +++ b/net/dsa/slave.c
> > @@ -1807,6 +1807,13 @@ int dsa_slave_create(struct dsa_port *port)
> > ret, slave_dev->name);
> > goto out_phy;
> > }
> > + rtnl_lock();
> > + ret = netdev_upper_dev_link(master, slave_dev, NULL);
> > + rtnl_unlock();
> > + if (ret) {
> > + unregister_netdevice(slave_dev);
> > + goto out_phy;
> > + }
> >
> > return 0;
> >
> > @@ -1826,12 +1833,14 @@ int dsa_slave_create(struct dsa_port *port)
> >
> > void dsa_slave_destroy(struct net_device *slave_dev)
> > {
> > + struct net_device *master = dsa_slave_to_master(slave_dev);
> > struct dsa_port *dp = dsa_slave_to_port(slave_dev);
> > struct dsa_slave_priv *p = netdev_priv(slave_dev);
> >
> > netif_carrier_off(slave_dev);
> > rtnl_lock();
> > phylink_disconnect_phy(dp->pl);
> > + netdev_upper_dev_unlink(master, slave_dev);
> > rtnl_unlock();
> >
> > dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
> >
> > Do you see a problem with it?
>
> I was initially not sure you could do this. But it looks like you can
> have N : M relationships between uppers and lowers. I suppose it does
> make sense. You can have multiple VLAN uppers to one base device. You
> can have multiple lowers to one bond device, etc.
>
> I wonder what 'side effects' there are for declaring this linkage. It
> is not something i've looked into before, since we never used it. So i
> don't see a problem with this, other than i don't know what problems
> we might run into :-)
>
> Andrew
>
It was surprising to me as well, since I was used to the bridge model
(a port can have only one bridge master). But it looks like, that is
the difference between netdev_upper_dev_link and
netdev_master_upper_dev_link. This uses the former, and the bridge
layer uses the latter.
So I guess it is ok.
Thanks,
-Vladimir
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-06-18 22:53 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-08 21:53 [Patch net] net: change addr_list_lock back to static key Cong Wang
2020-06-09 20:00 ` David Miller
2020-06-10 14:48 ` Taehee Yoo
2020-06-10 23:21 ` Cong Wang
2020-06-13 16:03 ` Taehee Yoo
2020-06-15 21:33 ` Cong Wang
2020-06-16 15:03 ` Taehee Yoo
2020-06-17 17:42 ` Cong Wang
2020-06-18 19:40 ` Vladimir Oltean
2020-06-18 19:56 ` Cong Wang
2020-06-18 20:06 ` Cong Wang
2020-06-18 20:33 ` Vladimir Oltean
2020-06-18 20:59 ` Cong Wang
2020-06-18 22:29 ` Andrew Lunn
2020-06-18 22:32 ` Vladimir Oltean
2020-06-18 22:46 ` Andrew Lunn
2020-06-18 22:53 ` Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.