[ofa-general] [PATCH v2] ipoib: fix a deadlock between ipoib start/stop and child interface create/delete

[Yossi Etigin]

Fix a deadlock between child interface creation/deletion and ipoib start/stop.
The former takes first vlan_mutex, and might take rtnl_lock via register_netdev
or unregister_netdev. The latter is executed with rtnl_lock held, and tries
to take vlan_mutex.
 We take the vlan_mutex and bring child interface up/down on a scheduled task
instead of during stop/start, since ipoib_workqueue will not be flushed with 
rtnl_lock held. 

Signed-off-by: Yossi Etigin <yosefe at voltaire.com>

---

Changes from v1:
 - Use u8 as the vlan task up/down flag type instead of atomic_t.

Fix bug #1198.

An alternative approach might be to fine-grain the locking (for example use 
one mutex to sync child creation/deletion, and another one to sync accesses to
child_intfs list).

 drivers/infiniband/ulp/ipoib/ipoib.h      |    3 ++
 drivers/infiniband/ulp/ipoib/ipoib_main.c |   33 ++++--------------------------
 drivers/infiniband/ulp/ipoib/ipoib_vlan.c |   22 ++++++++++++++++++++
 3 files changed, 30 insertions(+), 28 deletions(-)

Index: b/drivers/infiniband/ulp/ipoib/ipoib.h
===================================================================
--- a/drivers/infiniband/ulp/ipoib/ipoib.h	2008-12-15 20:28:21.000000000 +0200
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h	2008-12-31 14:46:29.000000000 +0200
@@ -298,6 +298,8 @@ struct ipoib_dev_priv {
 	struct work_struct flush_heavy;
 	struct work_struct restart_task;
 	struct delayed_work ah_reap_task;
+	struct work_struct vlan_task;
+	u8 vlan_task_flag;
 
 	struct ib_device *ca;
 	u8		  port;
@@ -501,6 +503,7 @@ void ipoib_event(struct ib_event_handler
 
 int ipoib_vlan_add(struct net_device *pdev, unsigned short pkey);
 int ipoib_vlan_delete(struct net_device *pdev, unsigned short pkey);
+void ipoib_vlan_task(struct work_struct *work);
 
 void ipoib_pkey_poll(struct work_struct *work);
 int ipoib_pkey_dev_delay_open(struct net_device *dev);
Index: b/drivers/infiniband/ulp/ipoib/ipoib_main.c
===================================================================
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c	2008-12-15 20:28:21.000000000 +0200
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c	2008-12-31 14:51:00.000000000 +0200
@@ -125,20 +125,8 @@ int ipoib_open(struct net_device *dev)
 	}
 
 	if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
-		struct ipoib_dev_priv *cpriv;
-
-		/* Bring up any child interfaces too */
-		mutex_lock(&priv->vlan_mutex);
-		list_for_each_entry(cpriv, &priv->child_intfs, list) {
-			int flags;
-
-			flags = cpriv->dev->flags;
-			if (flags & IFF_UP)
-				continue;
-
-			dev_change_flags(cpriv->dev, flags | IFF_UP);
-		}
-		mutex_unlock(&priv->vlan_mutex);
+		priv->vlan_task_flag = 1;
+		queue_work(ipoib_workqueue, &priv->vlan_task);
 	}
 
 	netif_start_queue(dev);
@@ -161,20 +149,8 @@ static int ipoib_stop(struct net_device 
 	ipoib_ib_dev_stop(dev, 0);
 
 	if (!test_bit(IPOIB_FLAG_SUBINTERFACE, &priv->flags)) {
-		struct ipoib_dev_priv *cpriv;
-
-		/* Bring down any child interfaces too */
-		mutex_lock(&priv->vlan_mutex);
-		list_for_each_entry(cpriv, &priv->child_intfs, list) {
-			int flags;
-
-			flags = cpriv->dev->flags;
-			if (!(flags & IFF_UP))
-				continue;
-
-			dev_change_flags(cpriv->dev, flags & ~IFF_UP);
-		}
-		mutex_unlock(&priv->vlan_mutex);
+		priv->vlan_task_flag = 0;
+		queue_work(ipoib_workqueue, &priv->vlan_task);
 	}
 
 	return 0;
@@ -1071,6 +1047,7 @@ static void ipoib_setup(struct net_devic
 	INIT_WORK(&priv->flush_heavy,   ipoib_ib_dev_flush_heavy);
 	INIT_WORK(&priv->restart_task, ipoib_mcast_restart_task);
 	INIT_DELAYED_WORK(&priv->ah_reap_task, ipoib_reap_ah);
+	INIT_WORK(&priv->vlan_task, ipoib_vlan_task);
 }
 
 struct ipoib_dev_priv *ipoib_intf_alloc(const char *name)
Index: b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
===================================================================
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c	2008-12-15 20:28:21.000000000 +0200
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c	2008-12-31 14:46:50.000000000 +0200
@@ -178,3 +178,25 @@ int ipoib_vlan_delete(struct net_device 
 
 	return ret;
 }
+
+void ipoib_vlan_task(struct work_struct *work)
+{
+	struct ipoib_dev_priv *priv =
+		container_of(work, struct ipoib_dev_priv, vlan_task);
+	struct ipoib_dev_priv *cpriv;
+	int flags, new_flags, iffup_value;
+
+	iffup_value = priv->vlan_task_flag ? IFF_UP : 0;
+
+	mutex_lock(&priv->vlan_mutex);
+	list_for_each_entry(cpriv, &priv->child_intfs, list) {
+		flags = cpriv->dev->flags;
+		new_flags = (flags & ~IFF_UP) | iffup_value;
+		if (flags != new_flags) {
+			rtnl_lock();
+			dev_change_flags(cpriv->dev, new_flags);
+			rtnl_unlock();
+		}
+	}
+	mutex_unlock(&priv->vlan_mutex);
+}

-- 
--Yossi