netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4]  Fix lockdep issues with stacked devices
@ 2014-05-16 21:04 Vlad Yasevich
  2014-05-16 21:04 ` [PATCH v3 1/4] net: Find the nesting level of a given device by type Vlad Yasevich
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Vlad Yasevich @ 2014-05-16 21:04 UTC (permalink / raw)
  To: netdev; +Cc: dingtianhong, kaber, vfalico, jiri, Vlad Yasevich

Recent commit dc8eaaa006350d24030502a4521542e74b5cb39f
    vlan: Fix lockdep warning when vlan dev handle notification

attempted to solve lockdep issues with vlans where multiple
vlans were stacked.  However, the code does not work correctly
when the vlan stack is interspersed with other devices in between
the vlans.  Additionally, similar lockdep issues show up with other
devices.

This series provides a generic way to solve these issue for any
devices that can be stacked.  It also addresses the concern for
vlan and macvlan devices.  I am not sure whether it makes sense
to do so for other types like team, vxlan, and bond.

Thanks
-vlad

Since v2:
  - Remove rcu variants from patch1, since that function is called
    only under rtnl.
  - Fix whitespace problems reported by checkpatch

Since v1:
  - Fixed up a goofed-up rebase.
    * is_vlan_dev() should be bool and that change belongs in patch3.
    * patch4 should not have any vlan changes in it.

Vlad Yasevich (4):
  net: Find the nesting level of a given device by type.
  net: Allow for more then a single subclass for netif_addr_lock
  vlan: Fix lockdep warning with stacked vlan devices.
  macvlan: Fix lockdep warnings with stacked macvlan devices

 drivers/net/macvlan.c      | 12 +++++++++--
 include/linux/if_macvlan.h |  1 +
 include/linux/if_vlan.h    |  3 ++-
 include/linux/netdevice.h  | 18 +++++++++++++++-
 net/8021q/vlan.c           |  1 +
 net/8021q/vlan_dev.c       | 52 ++++++++--------------------------------------
 net/core/dev.c             | 49 ++++++++++++++++++++++++++++++++++++++++++-
 7 files changed, 88 insertions(+), 48 deletions(-)

-- 
1.9.0

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH v3 1/4] net: Find the nesting level of a given device by type.
  2014-05-16 21:04 [PATCH v3 0/4] Fix lockdep issues with stacked devices Vlad Yasevich
@ 2014-05-16 21:04 ` Vlad Yasevich
  2014-05-17  2:16   ` David Miller
  2014-05-16 21:04 ` [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock Vlad Yasevich
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Vlad Yasevich @ 2014-05-16 21:04 UTC (permalink / raw)
  To: netdev; +Cc: dingtianhong, kaber, vfalico, jiri, Vlad Yasevich

Multiple devices in the kernel can be stacked/nested and they
need to know their nesting level for the purposes of lockdep.
This patch provides a generic function that determines a nesting
level of a particular device by its type (ex: vlan, macvlan, etc).
We only care about nesting of the same type of devices.

For example:
  eth0 <- vlan0.10 <- macvlan0 <- vlan1.20

The nesting level of vlan1.20 would be 1, since there is another vlan
in the stack under it.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/netdevice.h | 10 ++++++++++
 net/core/dev.c            | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 20e99ef..fb912e8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3077,6 +3077,14 @@ void *netdev_lower_get_next_private_rcu(struct net_device *dev,
 	     priv; \
 	     priv = netdev_lower_get_next_private_rcu(dev, &(iter)))
 
+void *netdev_lower_get_next(struct net_device *dev,
+				struct list_head **iter);
+#define netdev_for_each_lower_dev(dev, ldev, iter) \
+	for (iter = &(dev)->adj_list.lower, \
+	     ldev = netdev_lower_get_next(dev, &(iter)); \
+	     ldev; \
+	     ldev = netdev_lower_get_next(dev, &(iter)))
+
 void *netdev_adjacent_get_private(struct list_head *adj_list);
 void *netdev_lower_get_first_private_rcu(struct net_device *dev);
 struct net_device *netdev_master_upper_dev_get(struct net_device *dev);
@@ -3092,6 +3100,8 @@ void netdev_upper_dev_unlink(struct net_device *dev,
 void netdev_adjacent_rename_links(struct net_device *dev, char *oldname);
 void *netdev_lower_dev_get_private(struct net_device *dev,
 				   struct net_device *lower_dev);
+int dev_get_nest_level(struct net_device *dev,
+		       bool (*type_check)(struct net_device *dev));
 int skb_checksum_help(struct sk_buff *skb);
 struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
 				  netdev_features_t features, bool tx_path);
diff --git a/net/core/dev.c b/net/core/dev.c
index 6da649b..a9bfef6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4622,6 +4622,32 @@ void *netdev_lower_get_next_private_rcu(struct net_device *dev,
 EXPORT_SYMBOL(netdev_lower_get_next_private_rcu);
 
 /**
+ * netdev_lower_get_next - Get the next device from the lower neighbour
+ *                         list
+ * @dev: device
+ * @iter: list_head ** of the current position
+ *
+ * Gets the next netdev_adjacent from the dev's lower neighbour
+ * list, starting from iter position. The caller must hold RTNL lock or
+ * its own locking that guarantees that the neighbour lower
+ * list will remain unchainged.
+ */
+void *netdev_lower_get_next(struct net_device *dev, struct list_head **iter)
+{
+	struct netdev_adjacent *lower;
+
+	lower = list_entry((*iter)->next, struct netdev_adjacent, list);
+
+	if (&lower->list == &dev->adj_list.lower)
+		return NULL;
+
+	*iter = &lower->list;
+
+	return lower->dev;
+}
+EXPORT_SYMBOL(netdev_lower_get_next);
+
+/**
  * netdev_lower_get_first_private_rcu - Get the first ->private from the
  *				       lower neighbour list, RCU
  *				       variant
@@ -5071,6 +5097,30 @@ void *netdev_lower_dev_get_private(struct net_device *dev,
 }
 EXPORT_SYMBOL(netdev_lower_dev_get_private);
 
+
+int dev_get_nest_level(struct net_device *dev,
+		       bool (*type_check)(struct net_device *dev))
+{
+	struct net_device *lower = NULL;
+	struct list_head *iter;
+	int max_nest = -1;
+	int nest;
+
+	ASSERT_RTNL();
+
+	netdev_for_each_lower_dev(dev, lower, iter) {
+		nest = dev_get_nest_level(lower, type_check);
+		if (max_nest < nest)
+			max_nest = nest;
+	}
+
+	if (type_check(dev))
+		max_nest++;
+
+	return max_nest;
+}
+EXPORT_SYMBOL(dev_get_nest_level);
+
 static void dev_change_rx_flags(struct net_device *dev, int flags)
 {
 	const struct net_device_ops *ops = dev->netdev_ops;
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock
  2014-05-16 21:04 [PATCH v3 0/4] Fix lockdep issues with stacked devices Vlad Yasevich
  2014-05-16 21:04 ` [PATCH v3 1/4] net: Find the nesting level of a given device by type Vlad Yasevich
@ 2014-05-16 21:04 ` Vlad Yasevich
  2014-06-04 21:42   ` Keller, Jacob E
  2014-05-16 21:04 ` [PATCH v3 3/4] vlan: Fix lockdep warning with stacked vlan devices Vlad Yasevich
  2014-05-16 21:04 ` [PATCH v3 4/4] macvlan: Fix lockdep warnings with stacked macvlan devices Vlad Yasevich
  3 siblings, 1 reply; 11+ messages in thread
From: Vlad Yasevich @ 2014-05-16 21:04 UTC (permalink / raw)
  To: netdev; +Cc: dingtianhong, kaber, vfalico, jiri, Vlad Yasevich

Currently netif_addr_lock_nested assumes that there can be only
a single nesting level between 2 devices.  However, if we
have multiple devices of the same type stacked, this fails.
For example:
 eth0 <-- vlan0.10 <-- vlan0.10.20

A more complicated configuration may stack more then one type of
device in different order.
Ex:
  eth0 <-- vlan0.10 <-- macvlan0 <-- vlan1.10.20 <-- macvlan1

This patch adds an ndo_* function that allows each stackable
device to report its nesting level.  If the device doesn't
provide this function default subclass of 1 is used.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/netdevice.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index fb912e8..9d4b1f1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1144,6 +1144,7 @@ struct net_device_ops {
 	netdev_tx_t		(*ndo_dfwd_start_xmit) (struct sk_buff *skb,
 							struct net_device *dev,
 							void *priv);
+	int			(*ndo_get_lock_subclass)(struct net_device *dev);
 };
 
 /**
@@ -2950,7 +2951,12 @@ static inline void netif_addr_lock(struct net_device *dev)
 
 static inline void netif_addr_lock_nested(struct net_device *dev)
 {
-	spin_lock_nested(&dev->addr_list_lock, SINGLE_DEPTH_NESTING);
+	int subclass = SINGLE_DEPTH_NESTING;
+
+	if (dev->netdev_ops->ndo_get_lock_subclass)
+		subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
+
+	spin_lock_nested(&dev->addr_list_lock, subclass);
 }
 
 static inline void netif_addr_lock_bh(struct net_device *dev)
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v3 3/4] vlan: Fix lockdep warning with stacked vlan devices.
  2014-05-16 21:04 [PATCH v3 0/4] Fix lockdep issues with stacked devices Vlad Yasevich
  2014-05-16 21:04 ` [PATCH v3 1/4] net: Find the nesting level of a given device by type Vlad Yasevich
  2014-05-16 21:04 ` [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock Vlad Yasevich
@ 2014-05-16 21:04 ` Vlad Yasevich
  2014-05-16 21:04 ` [PATCH v3 4/4] macvlan: Fix lockdep warnings with stacked macvlan devices Vlad Yasevich
  3 siblings, 0 replies; 11+ messages in thread
From: Vlad Yasevich @ 2014-05-16 21:04 UTC (permalink / raw)
  To: netdev; +Cc: dingtianhong, kaber, vfalico, jiri, Vlad Yasevich

This reverts commit dc8eaaa006350d24030502a4521542e74b5cb39f.
	vlan: Fix lockdep warning when vlan dev handle notification

Instead we use the new new API to find the lock subclass of
our vlan device.  This way we can support configurations where
vlans are interspersed with other devices:
  bond -> vlan -> macvlan -> vlan

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 include/linux/if_vlan.h |  3 ++-
 net/8021q/vlan.c        |  1 +
 net/8021q/vlan_dev.c    | 52 +++++++++----------------------------------------
 net/core/dev.c          |  1 -
 4 files changed, 12 insertions(+), 45 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 13bbbde..724bde8 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -73,7 +73,7 @@ static inline struct vlan_ethhdr *vlan_eth_hdr(const struct sk_buff *skb)
 /* found in socket.c */
 extern void vlan_ioctl_set(int (*hook)(struct net *, void __user *));
 
-static inline int is_vlan_dev(struct net_device *dev)
+static inline bool is_vlan_dev(struct net_device *dev)
 {
         return dev->priv_flags & IFF_802_1Q_VLAN;
 }
@@ -159,6 +159,7 @@ struct vlan_dev_priv {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	struct netpoll				*netpoll;
 #endif
+	unsigned int				nest_level;
 };
 
 static inline struct vlan_dev_priv *vlan_dev_priv(const struct net_device *dev)
diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 175273f..44ebd5c 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -169,6 +169,7 @@ int register_vlan_dev(struct net_device *dev)
 	if (err < 0)
 		goto out_uninit_mvrp;
 
+	vlan->nest_level = dev_get_nest_level(real_dev, is_vlan_dev) + 1;
 	err = register_netdevice(dev);
 	if (err < 0)
 		goto out_uninit_mvrp;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 733ec28..019efb7 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -493,48 +493,10 @@ static void vlan_dev_change_rx_flags(struct net_device *dev, int change)
 	}
 }
 
-static int vlan_calculate_locking_subclass(struct net_device *real_dev)
-{
-	int subclass = 0;
-
-	while (is_vlan_dev(real_dev)) {
-		subclass++;
-		real_dev = vlan_dev_priv(real_dev)->real_dev;
-	}
-
-	return subclass;
-}
-
-static void vlan_dev_mc_sync(struct net_device *to, struct net_device *from)
-{
-	int err = 0, subclass;
-
-	subclass = vlan_calculate_locking_subclass(to);
-
-	spin_lock_nested(&to->addr_list_lock, subclass);
-	err = __hw_addr_sync(&to->mc, &from->mc, to->addr_len);
-	if (!err)
-		__dev_set_rx_mode(to);
-	spin_unlock(&to->addr_list_lock);
-}
-
-static void vlan_dev_uc_sync(struct net_device *to, struct net_device *from)
-{
-	int err = 0, subclass;
-
-	subclass = vlan_calculate_locking_subclass(to);
-
-	spin_lock_nested(&to->addr_list_lock, subclass);
-	err = __hw_addr_sync(&to->uc, &from->uc, to->addr_len);
-	if (!err)
-		__dev_set_rx_mode(to);
-	spin_unlock(&to->addr_list_lock);
-}
-
 static void vlan_dev_set_rx_mode(struct net_device *vlan_dev)
 {
-	vlan_dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
-	vlan_dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
+	dev_mc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
+	dev_uc_sync(vlan_dev_priv(vlan_dev)->real_dev, vlan_dev);
 }
 
 /*
@@ -562,6 +524,11 @@ static void vlan_dev_set_lockdep_class(struct net_device *dev, int subclass)
 	netdev_for_each_tx_queue(dev, vlan_dev_set_lockdep_one, &subclass);
 }
 
+static int vlan_dev_get_lock_subclass(struct net_device *dev)
+{
+	return vlan_dev_priv(dev)->nest_level;
+}
+
 static const struct header_ops vlan_header_ops = {
 	.create	 = vlan_dev_hard_header,
 	.rebuild = vlan_dev_rebuild_header,
@@ -597,7 +564,6 @@ static const struct net_device_ops vlan_netdev_ops;
 static int vlan_dev_init(struct net_device *dev)
 {
 	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
-	int subclass = 0;
 
 	netif_carrier_off(dev);
 
@@ -646,8 +612,7 @@ static int vlan_dev_init(struct net_device *dev)
 
 	SET_NETDEV_DEVTYPE(dev, &vlan_type);
 
-	subclass = vlan_calculate_locking_subclass(dev);
-	vlan_dev_set_lockdep_class(dev, subclass);
+	vlan_dev_set_lockdep_class(dev, vlan_dev_get_lock_subclass(dev));
 
 	vlan_dev_priv(dev)->vlan_pcpu_stats = netdev_alloc_pcpu_stats(struct vlan_pcpu_stats);
 	if (!vlan_dev_priv(dev)->vlan_pcpu_stats)
@@ -819,6 +784,7 @@ static const struct net_device_ops vlan_netdev_ops = {
 	.ndo_netpoll_cleanup	= vlan_dev_netpoll_cleanup,
 #endif
 	.ndo_fix_features	= vlan_dev_fix_features,
+	.ndo_get_lock_subclass  = vlan_dev_get_lock_subclass,
 };
 
 void vlan_setup(struct net_device *dev)
diff --git a/net/core/dev.c b/net/core/dev.c
index a9bfef6..33a54f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5286,7 +5286,6 @@ void __dev_set_rx_mode(struct net_device *dev)
 	if (ops->ndo_set_rx_mode)
 		ops->ndo_set_rx_mode(dev);
 }
-EXPORT_SYMBOL(__dev_set_rx_mode);
 
 void dev_set_rx_mode(struct net_device *dev)
 {
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v3 4/4] macvlan: Fix lockdep warnings with stacked macvlan devices
  2014-05-16 21:04 [PATCH v3 0/4] Fix lockdep issues with stacked devices Vlad Yasevich
                   ` (2 preceding siblings ...)
  2014-05-16 21:04 ` [PATCH v3 3/4] vlan: Fix lockdep warning with stacked vlan devices Vlad Yasevich
@ 2014-05-16 21:04 ` Vlad Yasevich
  2014-05-18 16:36   ` Patrick McHardy
  3 siblings, 1 reply; 11+ messages in thread
From: Vlad Yasevich @ 2014-05-16 21:04 UTC (permalink / raw)
  To: netdev; +Cc: dingtianhong, kaber, vfalico, jiri, Vlad Yasevich

Macvlan devices try to avoid stacking, but that's not always
successfull or even desired.  As an example, the following
configuration is perefectly legal and valid:

eth0 <--- macvlan0 <---- vlan0.10 <--- macvlan1

However, this configuration produces the following lockdep
trace:
[  115.620418] ======================================================
[  115.620477] [ INFO: possible circular locking dependency detected ]
[  115.620516] 3.15.0-rc1+ #24 Not tainted
[  115.620540] -------------------------------------------------------
[  115.620577] ip/1704 is trying to acquire lock:
[  115.620604]  (&vlan_netdev_addr_lock_key/1){+.....}, at: [<ffffffff815df49c>] dev_uc_sync+0x3c/0x80
[  115.620686]
but task is already holding lock:
[  115.620723]  (&macvlan_netdev_addr_lock_key){+.....}, at: [<ffffffff815da5be>] dev_set_rx_mode+0x1e/0x40
[  115.620795]
which lock already depends on the new lock.

[  115.620853]
the existing dependency chain (in reverse order) is:
[  115.620894]
-> #1 (&macvlan_netdev_addr_lock_key){+.....}:
[  115.620935]        [<ffffffff810d57f2>] lock_acquire+0xa2/0x130
[  115.620974]        [<ffffffff816f62e7>] _raw_spin_lock_nested+0x37/0x50
[  115.621019]        [<ffffffffa07296c3>] vlan_dev_set_rx_mode+0x53/0x110 [8021q]
[  115.621066]        [<ffffffff815da557>] __dev_set_rx_mode+0x57/0xa0
[  115.621105]        [<ffffffff815da5c6>] dev_set_rx_mode+0x26/0x40
[  115.621143]        [<ffffffff815da6be>] __dev_open+0xde/0x140
[  115.621174]        [<ffffffff815da9ad>] __dev_change_flags+0x9d/0x170
[  115.621174]        [<ffffffff815daaa9>] dev_change_flags+0x29/0x60
[  115.621174]        [<ffffffff815e7f11>] do_setlink+0x321/0x9a0
[  115.621174]        [<ffffffff815ea59f>] rtnl_newlink+0x51f/0x730
[  115.621174]        [<ffffffff815e6e75>] rtnetlink_rcv_msg+0x95/0x250
[  115.621174]        [<ffffffff81608b19>] netlink_rcv_skb+0xa9/0xc0
[  115.621174]        [<ffffffff815e6dca>] rtnetlink_rcv+0x2a/0x40
[  115.621174]        [<ffffffff81608150>] netlink_unicast+0xf0/0x1c0
[  115.621174]        [<ffffffff8160851f>] netlink_sendmsg+0x2ff/0x740
[  115.621174]        [<ffffffff815bc9db>] sock_sendmsg+0x8b/0xc0
[  115.621174]        [<ffffffff815bd4b9>] ___sys_sendmsg+0x369/0x380
[  115.621174]        [<ffffffff815bdbb2>] __sys_sendmsg+0x42/0x80
[  115.621174]        [<ffffffff815bdc02>] SyS_sendmsg+0x12/0x20
[  115.621174]        [<ffffffff816ffd69>] system_call_fastpath+0x16/0x1b
[  115.621174]
-> #0 (&vlan_netdev_addr_lock_key/1){+.....}:
[  115.621174]        [<ffffffff810d4d43>] __lock_acquire+0x1773/0x1a60
[  115.621174]        [<ffffffff810d57f2>] lock_acquire+0xa2/0x130
[  115.621174]        [<ffffffff816f62e7>] _raw_spin_lock_nested+0x37/0x50
[  115.621174]        [<ffffffff815df49c>] dev_uc_sync+0x3c/0x80
[  115.621174]        [<ffffffffa0696d2a>] macvlan_set_mac_lists+0xca/0x110 [macvlan]
[  115.621174]        [<ffffffff815da557>] __dev_set_rx_mode+0x57/0xa0
[  115.621174]        [<ffffffff815da5c6>] dev_set_rx_mode+0x26/0x40
[  115.621174]        [<ffffffff815da6be>] __dev_open+0xde/0x140
[  115.621174]        [<ffffffff815da9ad>] __dev_change_flags+0x9d/0x170
[  115.621174]        [<ffffffff815daaa9>] dev_change_flags+0x29/0x60
[  115.621174]        [<ffffffff815e7f11>] do_setlink+0x321/0x9a0
[  115.621174]        [<ffffffff815ea59f>] rtnl_newlink+0x51f/0x730
[  115.621174]        [<ffffffff815e6e75>] rtnetlink_rcv_msg+0x95/0x250
[  115.621174]        [<ffffffff81608b19>] netlink_rcv_skb+0xa9/0xc0
[  115.621174]        [<ffffffff815e6dca>] rtnetlink_rcv+0x2a/0x40
[  115.621174]        [<ffffffff81608150>] netlink_unicast+0xf0/0x1c0
[  115.621174]        [<ffffffff8160851f>] netlink_sendmsg+0x2ff/0x740
[  115.621174]        [<ffffffff815bc9db>] sock_sendmsg+0x8b/0xc0
[  115.621174]        [<ffffffff815bd4b9>] ___sys_sendmsg+0x369/0x380
[  115.621174]        [<ffffffff815bdbb2>] __sys_sendmsg+0x42/0x80
[  115.621174]        [<ffffffff815bdc02>] SyS_sendmsg+0x12/0x20
[  115.621174]        [<ffffffff816ffd69>] system_call_fastpath+0x16/0x1b
[  115.621174]
other info that might help us debug this:

[  115.621174]  Possible unsafe locking scenario:

[  115.621174]        CPU0                    CPU1
[  115.621174]        ----                    ----
[  115.621174]   lock(&macvlan_netdev_addr_lock_key);
[  115.621174]                                lock(&vlan_netdev_addr_lock_key/1);
[  115.621174]                                lock(&macvlan_netdev_addr_lock_key);
[  115.621174]   lock(&vlan_netdev_addr_lock_key/1);
[  115.621174]
 *** DEADLOCK ***

[  115.621174] 2 locks held by ip/1704:
[  115.621174]  #0:  (rtnl_mutex){+.+.+.}, at: [<ffffffff815e6dbb>] rtnetlink_rcv+0x1b/0x40
[  115.621174]  #1:  (&macvlan_netdev_addr_lock_key){+.....}, at: [<ffffffff815da5be>] dev_set_rx_mode+0x1e/0x40
[  115.621174]
stack backtrace:
[  115.621174] CPU: 3 PID: 1704 Comm: ip Not tainted 3.15.0-rc1+ #24
[  115.621174] Hardware name: Hewlett-Packard HP xw8400 Workstation/0A08h, BIOS 786D5 v02.38 10/25/2010
[  115.621174]  ffffffff82339ae0 ffff880465f79568 ffffffff816ee20c ffffffff82339ae0
[  115.621174]  ffff880465f795a8 ffffffff816e9e1b ffff880465f79600 ffff880465b019c8
[  115.621174]  0000000000000001 0000000000000002 ffff880465b019c8 ffff880465b01230
[  115.621174] Call Trace:
[  115.621174]  [<ffffffff816ee20c>] dump_stack+0x4d/0x66
[  115.621174]  [<ffffffff816e9e1b>] print_circular_bug+0x200/0x20e
[  115.621174]  [<ffffffff810d4d43>] __lock_acquire+0x1773/0x1a60
[  115.621174]  [<ffffffff810d3172>] ? trace_hardirqs_on_caller+0xb2/0x1d0
[  115.621174]  [<ffffffff810d57f2>] lock_acquire+0xa2/0x130
[  115.621174]  [<ffffffff815df49c>] ? dev_uc_sync+0x3c/0x80
[  115.621174]  [<ffffffff816f62e7>] _raw_spin_lock_nested+0x37/0x50
[  115.621174]  [<ffffffff815df49c>] ? dev_uc_sync+0x3c/0x80
[  115.621174]  [<ffffffff815df49c>] dev_uc_sync+0x3c/0x80
[  115.621174]  [<ffffffffa0696d2a>] macvlan_set_mac_lists+0xca/0x110 [macvlan]
[  115.621174]  [<ffffffff815da557>] __dev_set_rx_mode+0x57/0xa0
[  115.621174]  [<ffffffff815da5c6>] dev_set_rx_mode+0x26/0x40
[  115.621174]  [<ffffffff815da6be>] __dev_open+0xde/0x140
[  115.621174]  [<ffffffff815da9ad>] __dev_change_flags+0x9d/0x170
[  115.621174]  [<ffffffff815daaa9>] dev_change_flags+0x29/0x60
[  115.621174]  [<ffffffff811e1db1>] ? mem_cgroup_bad_page_check+0x21/0x30
[  115.621174]  [<ffffffff815e7f11>] do_setlink+0x321/0x9a0
[  115.621174]  [<ffffffff810d394c>] ? __lock_acquire+0x37c/0x1a60
[  115.621174]  [<ffffffff815ea59f>] rtnl_newlink+0x51f/0x730
[  115.621174]  [<ffffffff815ea169>] ? rtnl_newlink+0xe9/0x730
[  115.621174]  [<ffffffff815e6e75>] rtnetlink_rcv_msg+0x95/0x250
[  115.621174]  [<ffffffff810d329d>] ? trace_hardirqs_on+0xd/0x10
[  115.621174]  [<ffffffff815e6dbb>] ? rtnetlink_rcv+0x1b/0x40
[  115.621174]  [<ffffffff815e6de0>] ? rtnetlink_rcv+0x40/0x40
[  115.621174]  [<ffffffff81608b19>] netlink_rcv_skb+0xa9/0xc0
[  115.621174]  [<ffffffff815e6dca>] rtnetlink_rcv+0x2a/0x40
[  115.621174]  [<ffffffff81608150>] netlink_unicast+0xf0/0x1c0
[  115.621174]  [<ffffffff8160851f>] netlink_sendmsg+0x2ff/0x740
[  115.621174]  [<ffffffff815bc9db>] sock_sendmsg+0x8b/0xc0
[  115.621174]  [<ffffffff8119d4af>] ? might_fault+0x5f/0xb0
[  115.621174]  [<ffffffff8119d4f8>] ? might_fault+0xa8/0xb0
[  115.621174]  [<ffffffff8119d4af>] ? might_fault+0x5f/0xb0
[  115.621174]  [<ffffffff815cb51e>] ? verify_iovec+0x5e/0xe0
[  115.621174]  [<ffffffff815bd4b9>] ___sys_sendmsg+0x369/0x380
[  115.621174]  [<ffffffff816faa0d>] ? __do_page_fault+0x11d/0x570
[  115.621174]  [<ffffffff810cfe9f>] ? up_read+0x1f/0x40
[  115.621174]  [<ffffffff816fab04>] ? __do_page_fault+0x214/0x570
[  115.621174]  [<ffffffff8120a10b>] ? mntput_no_expire+0x6b/0x1c0
[  115.621174]  [<ffffffff8120a0b7>] ? mntput_no_expire+0x17/0x1c0
[  115.621174]  [<ffffffff8120a284>] ? mntput+0x24/0x40
[  115.621174]  [<ffffffff815bdbb2>] __sys_sendmsg+0x42/0x80
[  115.621174]  [<ffffffff815bdc02>] SyS_sendmsg+0x12/0x20
[  115.621174]  [<ffffffff816ffd69>] system_call_fastpath+0x16/0x1b

Fix this by correctly providing macvlan lockdep class.

Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 drivers/net/macvlan.c      | 12 ++++++++++--
 include/linux/if_macvlan.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index c5fb9cf..d53e299 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -517,6 +517,11 @@ static struct lock_class_key macvlan_netdev_addr_lock_key;
 #define MACVLAN_STATE_MASK \
 	((1<<__LINK_STATE_NOCARRIER) | (1<<__LINK_STATE_DORMANT))
 
+static int macvlan_get_nest_level(struct net_device *dev)
+{
+	return ((struct macvlan_dev *)netdev_priv(dev))->nest_level;
+}
+
 static void macvlan_set_lockdep_class_one(struct net_device *dev,
 					  struct netdev_queue *txq,
 					  void *_unused)
@@ -527,8 +532,9 @@ static void macvlan_set_lockdep_class_one(struct net_device *dev,
 
 static void macvlan_set_lockdep_class(struct net_device *dev)
 {
-	lockdep_set_class(&dev->addr_list_lock,
-			  &macvlan_netdev_addr_lock_key);
+	lockdep_set_class_and_subclass(&dev->addr_list_lock,
+				       &macvlan_netdev_addr_lock_key,
+				       macvlan_get_nest_level(dev));
 	netdev_for_each_tx_queue(dev, macvlan_set_lockdep_class_one, NULL);
 }
 
@@ -723,6 +729,7 @@ static const struct net_device_ops macvlan_netdev_ops = {
 	.ndo_fdb_add		= macvlan_fdb_add,
 	.ndo_fdb_del		= macvlan_fdb_del,
 	.ndo_fdb_dump		= ndo_dflt_fdb_dump,
+	.ndo_get_lock_subclass  = macvlan_get_nest_level,
 };
 
 void macvlan_common_setup(struct net_device *dev)
@@ -851,6 +858,7 @@ int macvlan_common_newlink(struct net *src_net, struct net_device *dev,
 	vlan->dev      = dev;
 	vlan->port     = port;
 	vlan->set_features = MACVLAN_FEATURES;
+	vlan->nest_level = dev_get_nest_level(lowerdev, netif_is_macvlan) + 1;
 
 	vlan->mode     = MACVLAN_MODE_VEPA;
 	if (data && data[IFLA_MACVLAN_MODE])
diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
index 7c8b20b..a9a53b1 100644
--- a/include/linux/if_macvlan.h
+++ b/include/linux/if_macvlan.h
@@ -56,6 +56,7 @@ struct macvlan_dev {
 	int			numqueues;
 	netdev_features_t	tap_features;
 	int			minor;
+	int			nest_level;
 };
 
 static inline void macvlan_count_rx(const struct macvlan_dev *vlan,
-- 
1.9.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/4] net: Find the nesting level of a given device by type.
  2014-05-16 21:04 ` [PATCH v3 1/4] net: Find the nesting level of a given device by type Vlad Yasevich
@ 2014-05-17  2:16   ` David Miller
  0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2014-05-17  2:16 UTC (permalink / raw)
  To: vyasevic; +Cc: netdev, dingtianhong, kaber, vfalico, jiri


Ok, v3 of this series is fine, all applied, thanks Vlad.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/4] macvlan: Fix lockdep warnings with stacked macvlan devices
  2014-05-16 21:04 ` [PATCH v3 4/4] macvlan: Fix lockdep warnings with stacked macvlan devices Vlad Yasevich
@ 2014-05-18 16:36   ` Patrick McHardy
  0 siblings, 0 replies; 11+ messages in thread
From: Patrick McHardy @ 2014-05-18 16:36 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, dingtianhong, vfalico, jiri

On Fri, May 16, 2014 at 05:04:56PM -0400, Vlad Yasevich wrote:
> diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h
> index 7c8b20b..a9a53b1 100644
> --- a/include/linux/if_macvlan.h
> +++ b/include/linux/if_macvlan.h
> @@ -56,6 +56,7 @@ struct macvlan_dev {
>  	int			numqueues;
>  	netdev_features_t	tap_features;
>  	int			minor;
> +	int			nest_level;
>  };

Please use unsigned int as for 802.1q since this can't be negative.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock
  2014-05-16 21:04 ` [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock Vlad Yasevich
@ 2014-06-04 21:42   ` Keller, Jacob E
  2014-06-04 21:53     ` David Miller
  2014-06-04 23:17     ` Vlad Yasevich
  0 siblings, 2 replies; 11+ messages in thread
From: Keller, Jacob E @ 2014-06-04 21:42 UTC (permalink / raw)
  To: vyasevic; +Cc: netdev, dingtianhong, vfalico, kaber, jiri

On Fri, 2014-05-16 at 17:04 -0400, Vlad Yasevich wrote:
> Currently netif_addr_lock_nested assumes that there can be only
> a single nesting level between 2 devices.  However, if we
> have multiple devices of the same type stacked, this fails.
> For example:
>  eth0 <-- vlan0.10 <-- vlan0.10.20
> 
> A more complicated configuration may stack more then one type of
> device in different order.
> Ex:
>   eth0 <-- vlan0.10 <-- macvlan0 <-- vlan1.10.20 <-- macvlan1
> 
> This patch adds an ndo_* function that allows each stackable
> device to report its nesting level.  If the device doesn't
> provide this function default subclass of 1 is used.
> 
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>  include/linux/netdevice.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index fb912e8..9d4b1f1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1144,6 +1144,7 @@ struct net_device_ops {
>  	netdev_tx_t		(*ndo_dfwd_start_xmit) (struct sk_buff *skb,
>  							struct net_device *dev,
>  							void *priv);
> +	int			(*ndo_get_lock_subclass)(struct net_device *dev);
>  };
>  
>  /**
> @@ -2950,7 +2951,12 @@ static inline void netif_addr_lock(struct net_device *dev)
>  
>  static inline void netif_addr_lock_nested(struct net_device *dev)
>  {
> -	spin_lock_nested(&dev->addr_list_lock, SINGLE_DEPTH_NESTING);
> +	int subclass = SINGLE_DEPTH_NESTING;
> +
> +	if (dev->netdev_ops->ndo_get_lock_subclass)
> +		subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
> +
> +	spin_lock_nested(&dev->addr_list_lock, subclass);
>  }
>  

Hi,

I know this has already been applied. However, this commit causes a
warning in include/linux/netdevice.h when W=1

In file included from ixgbe/ixgbe.h:35:0,
                 from ixgbe/ixgbe_lib.c:29:
include/linux/netdevice.h: In function ‘netif_addr_lock_nested’:
include/linux/netdevice.h:2954:6: warning: variable ‘subclass’ set but not used [-Wunused-but-set-variable]
  int subclass = SINGLE_DEPTH_NESTING;
      ^

This only occurs if CONFIG_DEBUG_LOCK_ALLOC=Y, and there seems to be no
easy fix for the warning, due to how spin_lock_nested is a macro that
discards the second parameter when DEBUG_LOCK_ALLOC is disabled.

I'm not sure how big a deal this is, considering that it only occurs at
W=1, and the patch is already committed.

>  static inline void netif_addr_lock_bh(struct net_device *dev)

Regards,
Jake


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock
  2014-06-04 21:42   ` Keller, Jacob E
@ 2014-06-04 21:53     ` David Miller
  2014-06-04 22:50       ` Keller, Jacob E
  2014-06-04 23:17     ` Vlad Yasevich
  1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2014-06-04 21:53 UTC (permalink / raw)
  To: jacob.e.keller; +Cc: vyasevic, netdev, dingtianhong, vfalico, kaber, jiri

From: "Keller, Jacob E" <jacob.e.keller@intel.com>
Date: Wed, 4 Jun 2014 21:42:12 +0000

> This only occurs if CONFIG_DEBUG_LOCK_ALLOC=Y, and there seems to be no
> easy fix for the warning, due to how spin_lock_nested is a macro that
> discards the second parameter when DEBUG_LOCK_ALLOC is disabled.

spin_lock_nested(), in that case, should mark the argument as unused.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock
  2014-06-04 21:53     ` David Miller
@ 2014-06-04 22:50       ` Keller, Jacob E
  0 siblings, 0 replies; 11+ messages in thread
From: Keller, Jacob E @ 2014-06-04 22:50 UTC (permalink / raw)
  To: davem; +Cc: netdev, vyasevic, vfalico, dingtianhong, jiri, kaber

On Wed, 2014-06-04 at 14:53 -0700, David Miller wrote:
> From: "Keller, Jacob E" <jacob.e.keller@intel.com>
> Date: Wed, 4 Jun 2014 21:42:12 +0000
> 
> > This only occurs if CONFIG_DEBUG_LOCK_ALLOC=Y, and there seems to be no
> > easy fix for the warning, due to how spin_lock_nested is a macro that
> > discards the second parameter when DEBUG_LOCK_ALLOC is disabled.
> 
> spin_lock_nested(), in that case, should mark the argument as unused.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Can it do that? spin_lock_nested is a macro.

Thanks,
Jake

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock
  2014-06-04 21:42   ` Keller, Jacob E
  2014-06-04 21:53     ` David Miller
@ 2014-06-04 23:17     ` Vlad Yasevich
  1 sibling, 0 replies; 11+ messages in thread
From: Vlad Yasevich @ 2014-06-04 23:17 UTC (permalink / raw)
  To: Keller, Jacob E, vyasevic; +Cc: netdev, dingtianhong, vfalico, kaber, jiri

On 06/04/2014 05:42 PM, Keller, Jacob E wrote:
> On Fri, 2014-05-16 at 17:04 -0400, Vlad Yasevich wrote:
>> Currently netif_addr_lock_nested assumes that there can be only
>> a single nesting level between 2 devices.  However, if we
>> have multiple devices of the same type stacked, this fails.
>> For example:
>>  eth0 <-- vlan0.10 <-- vlan0.10.20
>>
>> A more complicated configuration may stack more then one type of
>> device in different order.
>> Ex:
>>   eth0 <-- vlan0.10 <-- macvlan0 <-- vlan1.10.20 <-- macvlan1
>>
>> This patch adds an ndo_* function that allows each stackable
>> device to report its nesting level.  If the device doesn't
>> provide this function default subclass of 1 is used.
>>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>  include/linux/netdevice.h | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index fb912e8..9d4b1f1 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1144,6 +1144,7 @@ struct net_device_ops {
>>  	netdev_tx_t		(*ndo_dfwd_start_xmit) (struct sk_buff *skb,
>>  							struct net_device *dev,
>>  							void *priv);
>> +	int			(*ndo_get_lock_subclass)(struct net_device *dev);
>>  };
>>  
>>  /**
>> @@ -2950,7 +2951,12 @@ static inline void netif_addr_lock(struct net_device *dev)
>>  
>>  static inline void netif_addr_lock_nested(struct net_device *dev)
>>  {
>> -	spin_lock_nested(&dev->addr_list_lock, SINGLE_DEPTH_NESTING);
>> +	int subclass = SINGLE_DEPTH_NESTING;
>> +
>> +	if (dev->netdev_ops->ndo_get_lock_subclass)
>> +		subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
>> +
>> +	spin_lock_nested(&dev->addr_list_lock, subclass);
>>  }
>>  
> 
> Hi,
> 
> I know this has already been applied. However, this commit causes a
> warning in include/linux/netdevice.h when W=1
> 
> In file included from ixgbe/ixgbe.h:35:0,
>                  from ixgbe/ixgbe_lib.c:29:
> include/linux/netdevice.h: In function ‘netif_addr_lock_nested’:
> include/linux/netdevice.h:2954:6: warning: variable ‘subclass’ set but not used [-Wunused-but-set-variable]
>   int subclass = SINGLE_DEPTH_NESTING;
>       ^
> 
> This only occurs if CONFIG_DEBUG_LOCK_ALLOC=Y, and there seems to be no
> easy fix for the warning, due to how spin_lock_nested is a macro that
> discards the second parameter when DEBUG_LOCK_ALLOC is disabled.
> 
> I'm not sure how big a deal this is, considering that it only occurs at
> W=1, and the patch is already committed.
> 

Yuck.  It's actually raw_spin_lock_nested() macro that discards the
subclass argument...

We could do something really silly like:

# define raw_spin_lock_nested(lock, subclass) \
                             sublcass;_raw_spin_lock(lock)

That seems to get rid of the warning for me.

-vlad

>>  static inline void netif_addr_lock_bh(struct net_device *dev)
> 
> Regards,
> Jake
> 
> N�����r��y���b�X��ǧv�^�)޺{.n�+���z�^�)���w*\x1fjg���\x1e�����ݢj/���z�ޖ��2�ޙ���&�)ߡ�a��\x7f��\x1e�G���h�\x0f�j:+v���w�٥
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-06-04 23:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-16 21:04 [PATCH v3 0/4] Fix lockdep issues with stacked devices Vlad Yasevich
2014-05-16 21:04 ` [PATCH v3 1/4] net: Find the nesting level of a given device by type Vlad Yasevich
2014-05-17  2:16   ` David Miller
2014-05-16 21:04 ` [PATCH v3 2/4] net: Allow for more then a single subclass for netif_addr_lock Vlad Yasevich
2014-06-04 21:42   ` Keller, Jacob E
2014-06-04 21:53     ` David Miller
2014-06-04 22:50       ` Keller, Jacob E
2014-06-04 23:17     ` Vlad Yasevich
2014-05-16 21:04 ` [PATCH v3 3/4] vlan: Fix lockdep warning with stacked vlan devices Vlad Yasevich
2014-05-16 21:04 ` [PATCH v3 4/4] macvlan: Fix lockdep warnings with stacked macvlan devices Vlad Yasevich
2014-05-18 16:36   ` Patrick McHardy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).