From: Ben Pfaff Date: Wed, 5 Aug 2009 22:22:25 +0000 (-0700) Subject: datapath: Fix OOPS when dp_sysfs_add_if() fails. X-Git-Tag: v0.90.5~71 X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=58c342f617051e9e2ffb09710b4ef2b22c34e79a;p=sliver-openvswitch.git 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//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 d8bd245a1..f4284edce 100644 --- 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//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//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 e778a70a7..63d92cbb5 100644 --- 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 d98fdf328..c0ac01b95 100644 --- 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 5764a3a35..9699a0760 100644 --- 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//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 2771d6501..178afbd73 100644 --- 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//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//brport/bridge to * /sys/class/net/. */ 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 5c3540df7..e5aa51da6 100644 --- 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 index 000000000..c0de3d2ea --- /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 + +#include +#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 32e1735dc..b71828329 100644 --- 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)