datapath: Fix OOPS when dp_sysfs_add_if() fails.

Until now, when dp_sysfs_add_if() failed, the caller ignored the failure.
This is a minor problem, because everything else should continue working,
without sysfs entries for the interface, in theory anyhow.  In actual
practice, the error exit path of dp_sysfs_add_if() does a kobject_put(),
and that kobject_put() calls release_nbp(), so that the new port gets
freed.  The next reference to the new port (usually in an ovs-vswitchd call
to the ODP_PORT_LIST ioctl) will then use the freed data and probably OOPS.

The fix is to make the datapath code, as opposed to the sysfs code,
responsible for creating and destroying the net_bridge_port kobject.  The
dp_sysfs_{add,del}_if() functions then just attach and detach the kobject
to sysfs and their cleanup routines no longer need to destroy the kobject
and indeed we don't care whether dp_sysfs_add_if() really succeeds.

This commit also makes the same transformation to the datapath's ifobj,
for consistency.

It is easy to trigger the OOPS fixed by this commit by adding a network
device A to a datapath, then renaming network device A to B, then renaming
network device C to A, then adding A to the datapath.  The last attempt to
add A will fail because a file named /sys/class/net/<datapath>/brif/A
already exists from the time that C was added to the datapath under the
name A.

This commit also adds some compatibility infrastructure, because it moves
code out of #ifdef SUPPORT_SYSFS and it otherwise wouldn't build.

diff --git a/datapath/datapath.c b/datapath/datapath.c
index d8bd245a175d80ae4a3f78dac0f593b5b865aa66..f4284edce58c567d3799e9ebe08698138d0d5561 100644 (file)
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -171,6 +171,16 @@ errout:
                rtnl_set_sk_err(net, RTNLGRP_LINK, err);
 }
 
+static void release_dp(struct kobject *kobj)
+{
+       struct datapath *dp = container_of(kobj, struct datapath, ifobj);
+       kfree(dp);
+}
+
+struct kobj_type dp_ktype = {
+       .release = release_dp
+};
+
 static int create_dp(int dp_idx, const char __user *devnamep)
 {
        struct net_device *dp_dev;
@@ -212,6 +222,13 @@ static int create_dp(int dp_idx, const char __user *devnamep)
                skb_queue_head_init(&dp->queues[i]);
        init_waitqueue_head(&dp->waitqueue);
 
+       /* Initialize kobject for bridge.  This will be added as
+        * /sys/class/net/<devname>/bridge later, if sysfs is enabled. */
+       kobject_set_name(&dp->ifobj, SYSFS_BRIDGE_PORT_SUBDIR); /* "bridge" */
+       dp->ifobj.kset = NULL;
+       dp->ifobj.parent = NULL;
+       kobject_init(&dp->ifobj, &dp_ktype);
+
        /* Allocate table. */
        err = -ENOMEM;
        rcu_assign_pointer(dp->table, dp_table_create(DP_L1_SIZE));
@@ -284,7 +301,7 @@ static void do_destroy_dp(struct datapath *dp)
        for (i = 0; i < DP_MAX_GROUPS; i++)
                kfree(dp->groups[i]);
        free_percpu(dp->stats_percpu);
-       kfree(dp);
+       kobject_put(&dp->ifobj);
        module_put(THIS_MODULE);
 }
 
@@ -309,6 +326,19 @@ err_unlock:
        return err;
 }
 
+static void release_nbp(struct kobject *kobj)
+{
+       struct net_bridge_port *p = container_of(kobj, struct net_bridge_port, kobj);
+       kfree(p);
+}
+
+struct kobj_type brport_ktype = {
+#ifdef SUPPORT_SYSFS
+       .sysfs_ops = &brport_sysfs_ops,
+#endif
+       .release = release_nbp
+};
+
 /* Called with RTNL lock and dp_mutex. */
 static int new_nbp(struct datapath *dp, struct net_device *dev, int port_no)
 {
@@ -338,6 +368,13 @@ static int new_nbp(struct datapath *dp, struct net_device *dev, int port_no)
        list_add_rcu(&p->node, &dp->port_list);
        dp->n_ports++;
 
+       /* Initialize kobject for bridge.  This will be added as
+        * /sys/class/net/<devname>/brport later, if sysfs is enabled. */
+       kobject_set_name(&p->kobj, SYSFS_BRIDGE_PORT_ATTR); /* "brport" */
+       p->kobj.kset = NULL;
+       p->kobj.parent = &p->dev->NETDEV_DEV_MEMBER.kobj;
+       kobject_init(&p->kobj, &brport_ktype);
+
        dp_ifinfo_notify(RTM_NEWLINK, p);
 
        return 0;
@@ -435,15 +472,12 @@ int dp_del_port(struct net_bridge_port *p)
        /* Then wait until no one is still using it, and destroy it. */
        synchronize_rcu();
 
-       if (is_dp_dev(p->dev)) {
+       if (is_dp_dev(p->dev))
                dp_dev_destroy(p->dev);
-       }
-       if (p->port_no != ODPP_LOCAL) {
+       if (p->port_no != ODPP_LOCAL)
                dp_sysfs_del_if(p);
-       } else {
-               dev_put(p->dev);
-               kfree(p);
-       }
+       dev_put(p->dev);
+       kobject_put(&p->kobj);
 
        return 0;
 }

diff --git a/datapath/datapath.h b/datapath/datapath.h
index e778a70a74aa641ec3b7da0b60c07a51271deebe..63d92cbb5f723070a67cf27c9f85ea72e7384150 100644 (file)
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -64,9 +64,7 @@ struct datapath {
        struct mutex mutex;
        int dp_idx;
 
-#ifdef SUPPORT_SYSFS
        struct kobject ifobj;
-#endif
 
        int drop_frags;
 
@@ -94,9 +92,7 @@ struct net_bridge_port {
        u16 port_no;
        struct datapath *dp;
        struct net_device *dev;
-#ifdef SUPPORT_SYSFS
        struct kobject kobj;
-#endif
        struct list_head node;   /* Element in datapath.ports. */
 };
 

diff --git a/datapath/dp_sysfs.h b/datapath/dp_sysfs.h
index d98fdf328180ff89825bffe95cd1d348a7dd383b..c0ac01b95fbe116105c2bd078ffd6f3f6c917885 100644 (file)
--- a/datapath/dp_sysfs.h
+++ b/datapath/dp_sysfs.h
@@ -29,5 +29,9 @@ int dp_sysfs_del_if(struct net_bridge_port *p);
  * multiple versions. */
 #endif
 
+#ifdef SUPPORT_SYSFS
+extern struct sysfs_ops brport_sysfs_ops;
+#endif
+
 #endif /* dp_sysfs.h */
 

diff --git a/datapath/dp_sysfs_dp.c b/datapath/dp_sysfs_dp.c
index 5764a3a356dbf73a982f63b6f2a8ba8dc7a28ef4..9699a0760e4441ecd35f0e4da41ed5e4ef449215 100644 (file)
--- a/datapath/dp_sysfs_dp.c
+++ b/datapath/dp_sysfs_dp.c
@@ -487,12 +487,7 @@ int dp_sysfs_add_dp(struct datapath *dp)
        }
 
        /* Create /sys/class/net/<devname>/bridge directory. */
-       kobject_set_name(&dp->ifobj, SYSFS_BRIDGE_PORT_SUBDIR); /* "bridge" */
-       dp->ifobj.ktype = NULL;
-       dp->ifobj.kset = NULL;
        dp->ifobj.parent = kobj;
-       kboject_init(&dp->ifobj);
-
        err = kobject_add(&dp->ifobj);
        if (err) {
                pr_info("%s: can't add kobject (directory) %s/%s\n",
@@ -513,7 +508,6 @@ int dp_sysfs_del_dp(struct datapath *dp)
        struct kobject *kobj = to_kobj(dp->ports[ODPP_LOCAL]->dev);
 
        kobject_del(&dp->ifobj);
-       kobject_put(&dp->ifobj);
        sysfs_remove_group(kobj, &bridge_group);
 
        return 0;

diff --git a/datapath/dp_sysfs_if.c b/datapath/dp_sysfs_if.c
index 2771d65010fb059630acc513d4f313a4838143c1..178afbd733dea14af34f4502ee7cbebd57b05129 100644 (file)
--- a/datapath/dp_sysfs_if.c
+++ b/datapath/dp_sysfs_if.c
@@ -266,18 +266,6 @@ struct sysfs_ops brport_sysfs_ops = {
        .store = brport_store,
 };
 
-static void release_nbp(struct kobject *kobj)
-{
-       struct net_bridge_port *p
-               = container_of(kobj, struct net_bridge_port, kobj);
-       kfree(p);
-}
-
-struct kobj_type brport_ktype = {
-       .sysfs_ops = &brport_sysfs_ops,
-       .release = release_nbp
-};
-
 /*
  * Add sysfs entries to ethernet device added to a bridge.
  * Creates a brport subdirectory with bridge attributes.
@@ -290,12 +278,6 @@ int dp_sysfs_add_if(struct net_bridge_port *p)
        int err;
 
        /* Create /sys/class/net/<devname>/brport directory. */
-       kobject_set_name(&p->kobj, SYSFS_BRIDGE_PORT_ATTR); /* "brport" */
-       p->kobj.ktype = &brport_ktype;
-       p->kobj.kset = NULL;
-       p->kobj.parent = &(p->dev->class_dev.kobj);
-       kobject_init(&p->kobj);
-
        err = kobject_add(&p->kobj);
        if (err)
                goto err_put;
@@ -303,7 +285,7 @@ int dp_sysfs_add_if(struct net_bridge_port *p)
        /* Create symlink from /sys/class/net/<devname>/brport/bridge to
         * /sys/class/net/<bridgename>. */
        err = sysfs_create_link(&p->kobj,
-                               &dp->ports[ODPP_LOCAL]->dev->class_dev.kobj,
+                               &dp->ports[ODPP_LOCAL]->dev->NETDEV_DEV_MEMBER.kobj,
                                SYSFS_BRIDGE_PORT_LINK); /* "bridge" */
        if (err)
                goto err_del;
@@ -329,20 +311,18 @@ err_del:
        kobject_del(&p->kobj);
 err_put:
        kobject_put(&p->kobj);
+
+       /* Ensure that dp_sysfs_del_if becomes a no-op. */
+       p->kobj.dentry = NULL;
        return err;
 }
 
 int dp_sysfs_del_if(struct net_bridge_port *p)
 {
-       struct net_device *dev = p->dev;
-
-       kobject_uevent(&p->kobj, KOBJ_REMOVE);
-       kobject_del(&p->kobj);
-
-       dev_put(dev);
-
-       kobject_put(&p->kobj);
-
+       if (p->kobj.dentry) {
+               kobject_uevent(&p->kobj, KOBJ_REMOVE);
+               kobject_del(&p->kobj);
+       }
        return 0;
 }
 #endif /* SUPPORT_SYSFS */

diff --git a/datapath/linux-2.6/Modules.mk b/datapath/linux-2.6/Modules.mk
index 5c3540df72693087fe53c759328f6ce6d2bd922b..e5aa51da64821590e71847c7c2cbc9fecb98b7a5 100644 (file)
--- a/datapath/linux-2.6/Modules.mk
+++ b/datapath/linux-2.6/Modules.mk
@@ -12,6 +12,7 @@ openvswitch_headers += \
        linux-2.6/compat-2.6/include/linux/ipv6.h \
        linux-2.6/compat-2.6/include/linux/jiffies.h \
        linux-2.6/compat-2.6/include/linux/kernel.h \
+       linux-2.6/compat-2.6/include/linux/kobject.h \
        linux-2.6/compat-2.6/include/linux/log2.h \
        linux-2.6/compat-2.6/include/linux/lockdep.h \
        linux-2.6/compat-2.6/include/linux/mutex.h \

diff --git a/datapath/linux-2.6/compat-2.6/include/linux/kobject.h b/datapath/linux-2.6/compat-2.6/include/linux/kobject.h
new file mode 100644 (file)
index 0000000..c0de3d2
--- /dev/null
+++ b/datapath/linux-2.6/compat-2.6/include/linux/kobject.h
@@ -0,0 +1,16 @@
+#ifndef __LINUX_KOBJECT_WRAPPER_H
+#define __LINUX_KOBJECT_WRAPPER_H 1
+
+#include_next <linux/kobject.h>
+
+#include <linux/version.h>
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,25)
+#define kobject_init(kobj, ktype) rpl_kobject_init(kobj, ktype)
+static inline void rpl_kobject_init(struct kobject *kobj, struct kobj_type *ktype)
+{
+       kobj->ktype = ktype;
+       (kobject_init)(kobj);
+}
+#endif
+
+#endif /* linux/kobject.h wrapper */

diff --git a/datapath/linux-2.6/compat-2.6/include/linux/netdevice.h b/datapath/linux-2.6/compat-2.6/include/linux/netdevice.h
index 32e1735dcd84d73049762a19aa69414d22c3f6a6..b7182832999dd9da3a44207d72a2c89327ad2fcb 100644 (file)
--- a/datapath/linux-2.6/compat-2.6/include/linux/netdevice.h
+++ b/datapath/linux-2.6/compat-2.6/include/linux/netdevice.h
@@ -5,8 +5,18 @@
 
 struct net;
 
+/* Before 2.6.21, struct net_device has a "struct class_device" member named
+ * class_dev.  Beginning with 2.6.21, struct net_device instead has a "struct
+ * device" member named dev.  Otherwise the usage of these members is pretty
+ * much the same. */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,21)
+#define NETDEV_DEV_MEMBER class_dev
+#else
+#define NETDEV_DEV_MEMBER dev
+#endif
+
 #ifndef to_net_dev
-#define to_net_dev(class) container_of(class, struct net_device, class_dev)
+#define to_net_dev(class) container_of(class, struct net_device, NETDEV_DEV_MEMBER)
 #endif
 
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,26)