vport-internal: Set vport to NULL when detaching.

'struct net_device' is refcounted and can stick around for quite a
while if someone is still holding a reference to it.  However, we
free the vport that it is attached to in the next RCU grace period
after detach.  This assigns the vport to NULL on detach and adds
appropriate checks.

diff --git a/datapath/dp_notify.c b/datapath/dp_notify.c
index e73a731..e0bd14c 100644 (file)
--- a/datapath/dp_notify.c
+++ b/datapath/dp_notify.c
@@ -24,12 +24,11 @@ static int dp_device_event(struct notifier_block *unused, unsigned long event,
 
        if (is_internal_dev(dev))
                vport = internal_dev_get_vport(dev);
-       else {
+       else
                vport = netdev_get_vport(dev);
 
-               if (!vport)
-                       return NOTIFY_DONE;
-       }
+       if (!vport)
+               return NOTIFY_DONE;
 
        p = vport_get_dp_port(vport);
 

diff --git a/datapath/dp_sysfs_dp.c b/datapath/dp_sysfs_dp.c
index 8214c60..74d0ed0 100644 (file)
--- a/datapath/dp_sysfs_dp.c
+++ b/datapath/dp_sysfs_dp.c
@@ -43,9 +43,18 @@
 
 struct datapath *sysfs_get_dp(struct net_device *netdev)
 {
-       return vport_get_dp_port(internal_dev_get_vport(netdev))->dp;
-}
+       struct vport *vport = internal_dev_get_vport(netdev);
+       struct dp_port *dp_port;
+
+       if (!vport)
+               return NULL;
 
+       dp_port = vport_get_dp_port(vport);
+       if (!dp_port)
+               return NULL;
+
+       return dp_port->dp;
+}
 /*
  * Common code for storing bridge parameters.
  */
@@ -53,9 +62,9 @@ static ssize_t store_bridge_parm(DEVICE_PARAMS,
                                 const char *buf, size_t len,
                                 void (*set)(struct datapath *, unsigned long))
 {
-       struct datapath *dp = sysfs_get_dp(to_net_dev(d));
        char *endp;
        unsigned long val;
+       ssize_t result = len;
 
        if (!capable(CAP_NET_ADMIN))
                return -EPERM;
@@ -69,11 +78,21 @@ static ssize_t store_bridge_parm(DEVICE_PARAMS,
         * xxx ignore the request. 
         */
        if (val != 0) {
-               printk("%s: xxx writing dp parms not supported yet!\n", 
-                      dp_name(dp));
+               struct datapath *dp;
+
+               rcu_read_lock();
+
+               dp = sysfs_get_dp(to_net_dev(d));
+               if (dp)
+                       printk("%s: xxx writing dp parms not supported yet!\n", 
+                              dp_name(dp));
+               else
+                       result = -ENODEV;
+
+               rcu_read_unlock();
        }
 
-       return len;
+       return result;
 }
 
 
@@ -159,11 +178,20 @@ static ssize_t store_stp_state(DEVICE_PARAMS,
                               const char *buf,
                               size_t len)
 {
-       struct datapath *dp = sysfs_get_dp(to_net_dev(d));
+       struct datapath *dp;
+       ssize_t result = len;
+
+       rcu_read_lock();
 
-       printk("%s: xxx attempt to set_stp_state()\n", dp_name(dp));
+       dp = sysfs_get_dp(to_net_dev(d));
+       if (dp)
+               printk("%s: xxx attempt to set_stp_state()\n", dp_name(dp));
+       else
+               result = -ENODEV;
 
-       return len;
+       rcu_read_unlock();
+
+       return result;
 }
 static INTERNAL_DEVICE_ATTR(stp_state, S_IRUGO | S_IWUSR, show_stp_state,
                   store_stp_state);
@@ -193,12 +221,24 @@ static INTERNAL_DEVICE_ATTR(root_id, S_IRUGO, show_root_id, NULL);
 
 static ssize_t show_bridge_id(DEVICE_PARAMS, char *buf)
 {
-       struct datapath *dp = sysfs_get_dp(to_net_dev(d));
-       const unsigned char *addr = vport_get_addr(dp->ports[ODPP_LOCAL]->vport);
+       struct vport *vport;
+       ssize_t result;
+
+       rcu_read_lock();
+
+       vport = internal_dev_get_vport(to_net_dev(d));
+       if (vport) {
+               const unsigned char *addr;
+
+               addr = vport_get_addr(vport);
+               result = sprintf(buf, "%.2x%.2x.%.2x%.2x%.2x%.2x%.2x%.2x\n",
+                                0, 0, addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
+       } else
+               result = -ENODEV;
 
-       /* xxx Do we need a lock of some sort? */
-       return sprintf(buf, "%.2x%.2x.%.2x%.2x%.2x%.2x%.2x%.2x\n",
-                       0, 0, addr[0], addr[1], addr[2], addr[3], addr[4], addr[5]);
+       rcu_read_unlock();
+
+       return result;
 }
 static INTERNAL_DEVICE_ATTR(bridge_id, S_IRUGO, show_bridge_id, NULL);
 
@@ -260,10 +300,20 @@ static ssize_t show_group_addr(DEVICE_PARAMS, char *buf)
 static ssize_t store_group_addr(DEVICE_PARAMS,
                                const char *buf, size_t len)
 {
-       struct datapath *dp = sysfs_get_dp(to_net_dev(d));
+       struct datapath *dp;
+       ssize_t result = len;
+
+       rcu_read_lock();
+
+       dp = sysfs_get_dp(to_net_dev(d));
+       if (dp)
+               printk("%s: xxx attempt to store_group_addr()\n", dp_name(dp));
+       else
+               result = -ENODEV;
+
+       rcu_read_unlock();
 
-       printk("%s: xxx attempt to store_group_addr()\n", dp_name(dp));
-       return len;
+       return result;
 }
 
 static INTERNAL_DEVICE_ATTR(group_addr, S_IRUGO | S_IWUSR,

diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
index eca2a60..6cbfdf8 100644 (file)
--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -19,7 +19,7 @@
 #include "vport-netdev.h"
 
 struct internal_dev {
-       struct vport *vport;
+       struct vport *attached_vport, *vport;
        struct net_device_stats stats;
 };
 
@@ -70,11 +70,12 @@ static int internal_dev_mac_addr(struct net_device *dev, void *p)
 /* Called with rcu_read_lock and bottom-halves disabled. */
 static int internal_dev_xmit(struct sk_buff *skb, struct net_device *netdev)
 {
-       struct vport *vport = internal_dev_get_vport(netdev);
+       struct internal_dev *internal_dev = internal_dev_priv(netdev);
+       struct vport *vport = rcu_dereference(internal_dev->vport);
 
        /* We need our own clone. */
        skb = skb_share_check(skb, GFP_ATOMIC);
-       if (!skb) {
+       if (unlikely(!skb)) {
                vport_record_error(vport, VPORT_E_RX_DROPPED);
                return 0;
        }
@@ -102,9 +103,15 @@ static int internal_dev_stop(struct net_device *netdev)
 static void internal_dev_getinfo(struct net_device *netdev,
                                 struct ethtool_drvinfo *info)
 {
-       struct dp_port *dp_port = vport_get_dp_port(internal_dev_get_vport(netdev));
+       struct vport *vport = internal_dev_get_vport(netdev);
+       struct dp_port *dp_port;
 
        strcpy(info->driver, "openvswitch");
+
+       if (!vport)
+               return;
+
+       dp_port = vport_get_dp_port(vport);
        if (dp_port)
                sprintf(info->bus_info, "%d.%d", dp_port->dp->dp_idx, dp_port->port_no);
 }
@@ -122,14 +129,18 @@ static struct ethtool_ops internal_dev_ethtool_ops = {
 
 static int internal_dev_change_mtu(struct net_device *netdev, int new_mtu)
 {
-       struct dp_port *dp_port = vport_get_dp_port(internal_dev_get_vport(netdev));
+       struct vport *vport = internal_dev_get_vport(netdev);
 
        if (new_mtu < 68)
                return -EINVAL;
 
-       if (dp_port) {
-               if (new_mtu > dp_min_mtu(dp_port->dp))
-                       return -EINVAL;
+       if (vport) {
+               struct dp_port *dp_port = vport_get_dp_port(vport);
+
+               if (dp_port) {
+                       if (new_mtu > dp_min_mtu(dp_port->dp))
+                               return -EINVAL;
+               }
        }
 
        netdev->mtu = new_mtu;
@@ -211,7 +222,7 @@ static struct vport *internal_dev_create(const char *name,
        }
 
        internal_dev = internal_dev_priv(netdev_vport->dev);
-       internal_dev->vport = vport;
+       rcu_assign_pointer(internal_dev->vport, vport);
 
        err = register_netdevice(netdev_vport->dev);
        if (err)
@@ -240,13 +251,11 @@ static int internal_dev_destroy(struct vport *vport)
 static int internal_dev_attach(struct vport *vport)
 {
        struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
+       struct internal_dev *internal_dev = internal_dev_priv(netdev_vport->dev);
 
+       rcu_assign_pointer(internal_dev->attached_vport, internal_dev->vport);
        dev_set_promiscuity(netdev_vport->dev, 1);
-
-       /* It would make sense to assign dev->br_port here too, but
-        * that causes packets received on internal ports to get caught
-        * in netdev_frame_hook().  In turn netdev_frame_hook() can reject them
-        * back to the network stack, but that's a waste of time. */
+       netif_start_queue(netdev_vport->dev);
 
        return 0;
 }
@@ -254,13 +263,11 @@ static int internal_dev_attach(struct vport *vport)
 static int internal_dev_detach(struct vport *vport)
 {
        struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
+       struct internal_dev *internal_dev = internal_dev_priv(netdev_vport->dev);
 
+       netif_stop_queue(netdev_vport->dev);
        dev_set_promiscuity(netdev_vport->dev, -1);
-
-       /* Make sure that no packets arrive from now on, since
-        * internal_dev_xmit() will try to find itself through
-        * p->dp->ports[], and we're about to set that to null. */
-       netif_tx_disable(netdev_vport->dev);
+       rcu_assign_pointer(internal_dev->attached_vport, NULL);
 
        return 0;
 }
@@ -322,5 +329,5 @@ int is_internal_vport(const struct vport *vport)
 struct vport *internal_dev_get_vport(struct net_device *netdev)
 {
        struct internal_dev *internal_dev = internal_dev_priv(netdev);
-       return rcu_dereference(internal_dev->vport);
+       return rcu_dereference(internal_dev->attached_vport);
 }