datapath: Fix deadlock in switch port removal.

dp_del_switch_port() cancels the port_task and waits for
it to finish, but the port_task requires the RTNL lock
to complete, which dp_del_switch_port() holds, thus a
deadlock.

This commit fixes the problem, by deleting the port_task
entirely and moving its work into secchan.

diff --git a/datapath/datapath.c b/datapath/datapath.c
index efa84f9f09a30ebfeb376ae9c2252a425ddcdfdb..4b5103e36dfe73b5709f7c9b84adc55083639159 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -387,7 +387,6 @@ static struct net_bridge_port *new_nbp(struct datapath *dp,
        p->dev = dev;
        p->port_no = port_no;
        spin_lock_init(&p->lock);
-       INIT_WORK(&p->port_task, NULL);
        if (port_no != OFPP_LOCAL)
                rcu_assign_pointer(dev->br_port, p);
        if (port_no < DP_MAX_PORTS)
@@ -432,7 +431,6 @@ int dp_del_switch_port(struct net_bridge_port *p)
 #endif
 
        /* First drop references to device. */
-       cancel_work_sync(&p->port_task);
        dev_set_promiscuity(p->dev, -1);
        list_del_rcu(&p->node);
        if (p->port_no != OFPP_LOCAL)
@@ -949,36 +947,6 @@ dp_send_hello(struct datapath *dp, const struct sender *sender,
        }
 }
 
-/* Callback function for a workqueue to disable an interface */
-static void
-down_port_cb(struct work_struct *work)
-{
-       struct net_bridge_port *p = container_of(work, struct net_bridge_port, 
-                       port_task);
-
-       rtnl_lock();
-       if (dev_change_flags(p->dev, p->dev->flags & ~IFF_UP) < 0)
-               if (net_ratelimit())
-                       printk("problem bringing up port %s\n", p->dev->name);
-       rtnl_unlock();
-       p->config |= OFPPC_PORT_DOWN;
-}
-
-/* Callback function for a workqueue to enable an interface */
-static void
-up_port_cb(struct work_struct *work)
-{
-       struct net_bridge_port *p = container_of(work, struct net_bridge_port, 
-                       port_task);
-
-       rtnl_lock();
-       if (dev_change_flags(p->dev, p->dev->flags | IFF_UP) < 0)
-               if (net_ratelimit())
-                       printk("problem bringing down port %s\n", p->dev->name);
-       rtnl_unlock();
-       p->config &= ~OFPPC_PORT_DOWN;
-}
-
 int
 dp_update_port_flags(struct datapath *dp, const struct ofp_port_mod *opm)
 {
@@ -999,22 +967,6 @@ dp_update_port_flags(struct datapath *dp, const struct ofp_port_mod *opm)
                p->config &= ~config_mask;
                p->config |= ntohl(opm->config) & config_mask;
        }
-
-       /* Modifying the status of an interface requires taking a lock
-        * that cannot be done from here.  For this reason, we use a shared 
-        * workqueue, which will cause it to be executed from a safer 
-        * context. */
-       if (opm->mask & htonl(OFPPC_PORT_DOWN)) {
-               if ((opm->config & htonl(OFPPC_PORT_DOWN))
-                   && (p->config & OFPPC_PORT_DOWN) == 0) {
-                       PREPARE_WORK(&p->port_task, down_port_cb);
-                       schedule_work(&p->port_task);
-               } else if ((opm->config & htonl(OFPPC_PORT_DOWN)) == 0
-                          && (p->config & OFPPC_PORT_DOWN)) {
-                       PREPARE_WORK(&p->port_task, up_port_cb);
-                       schedule_work(&p->port_task);
-               }
-       }
        spin_unlock_irqrestore(&p->lock, flags);
 
        return 0;

diff --git a/datapath/datapath.h b/datapath/datapath.h
index 697a4c48d2839b790ddc0407832dc91a0983ff39..4f71e371b778dd42ab1a112db29d621aec8adb2e 100644 (file)
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -79,7 +79,6 @@ struct net_bridge_port {
        u32 config;             /* Some subset of OFPPC_* flags. */
        u32 state;              /* Some subset of OFPPS_* flags. */
        spinlock_t lock;
-       struct work_struct port_task;
        struct datapath *dp;
        struct net_device *dev;
        struct snat_conf *snat;  /* Only set if SNAT is configured for this port. */

diff --git a/secchan/port-watcher.c b/secchan/port-watcher.c
index 7daa370e73e7c8070519a7de0dbb34738153ce19..1f6a53d87c513fd295d5df1088c8a1358521ba11 100644 (file)
--- a/secchan/port-watcher.c
+++ b/secchan/port-watcher.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2008 The Board of Trustees of The Leland Stanford
+/* Copyright (c) 2008, 2009 The Board of Trustees of The Leland Stanford
  * Junior University
  *
  * We are making the OpenFlow specification and associated documentation
@@ -38,6 +38,7 @@
 #include <inttypes.h>
 #include <stdlib.h>
 #include "dynamic-string.h"
+#include "netdev.h"
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
 #include "poll-loop.h"
@@ -254,6 +255,31 @@ port_watcher_local_packet_cb(struct relay *r, void *pw_)
     return false;
 }
 
+static void
+bring_netdev_up_or_down(const char *name, bool down)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    struct netdev *netdev;
+    int retval;
+
+    retval = netdev_open(name, NETDEV_ETH_TYPE_NONE, &netdev);
+    if (!retval) {
+        if (down) {
+            retval = netdev_turn_flags_on(netdev, NETDEV_UP, true);
+        } else {
+            retval = netdev_turn_flags_off(netdev, NETDEV_UP, true);
+        }
+        if (retval) {
+            VLOG_WARN_RL(&rl, "failed to bring network device %s %s: %s",
+                         name, down ? "down" : "up", strerror(retval));
+        }
+        netdev_close(netdev);
+    } else {
+        VLOG_WARN_RL(&rl, "failed to open network device %s: %s",
+                     name, strerror(retval));
+    }
+}
+
 static bool
 port_watcher_remote_packet_cb(struct relay *r, void *pw_)
 {
@@ -274,6 +300,11 @@ port_watcher_remote_packet_cb(struct relay *r, void *pw_)
             if (pw_opp->port_no == htons(OFPP_LOCAL)) {
                 call_local_port_changed_callbacks(pw);
             }
+
+            if (opm->mask & htonl(OFPPC_PORT_DOWN)) {
+                bring_netdev_up_or_down((const char *) pw_opp->name,
+                                        opm->config & htonl(OFPPC_PORT_DOWN));
+            }
         }
     }
     return false;

diff --git a/udatapath/datapath.c b/udatapath/datapath.c
index 5c3f1374b680a15a5bf7b03f81cd88edacdaf503..aee71ee5e85b6a6369b0b26d27cf8cf163e6deb0 100644 (file)
--- a/udatapath/datapath.c
+++ b/udatapath/datapath.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2008 The Board of Trustees of The Leland Stanford
+/* Copyright (c) 2008, 2009 The Board of Trustees of The Leland Stanford
  * Junior University
  * 
  * We are making the OpenFlow specification and associated documentation
@@ -745,18 +745,6 @@ update_port_flags(struct datapath *dp, const struct ofp_port_mod *opm)
         p->config &= ~config_mask;
         p->config |= ntohl(opm->config) & config_mask;
     }
-
-    if (opm->mask & htonl(OFPPC_PORT_DOWN)) {
-        if ((opm->config & htonl(OFPPC_PORT_DOWN))
-            && (p->config & OFPPC_PORT_DOWN) == 0) {
-            p->config |= OFPPC_PORT_DOWN;
-            netdev_turn_flags_off(p->netdev, NETDEV_UP, true);
-        } else if ((opm->config & htonl(OFPPC_PORT_DOWN)) == 0
-                   && (p->config & OFPPC_PORT_DOWN)) {
-            p->config &= ~OFPPC_PORT_DOWN;
-            netdev_turn_flags_on(p->netdev, NETDEV_UP, true);
-        }
-    }
 }
 
 /* Update the port status field of the bridge port.  A non-zero return