From 6fba0d0b8206f603e36cd03ca99e75d9d1e77fe9 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 26 Jun 2009 14:15:04 -0700 Subject: [PATCH] datapath: Fix use-after-free error in datapath destruction. When we create a datapath we do this: 1. Create local port. 2. Call add_dp hook. 3. Allow userspace to add more ports. When we deleted a datapath we were doing this: 1. Call del_dp hook 2. Delete all the ports. Unfortunately step 1 destroys dp->ifobj, then dp_del_port on any port other than the local port in step 2 tries to reference dp->ifobj through a call to sysfs_remove_link(). This commit fixes the problem by changing datapath deletion to mirror creation: 1. Delete all the ports but the local port. 2. Call dp_del hook. 3. Delete local port. Commit 010082639 "datapath: Add sysfs support for all (otherwise supported) Linux versions" makes this problem obvious on a 2.6.25+ kernel configured with slab debugging, because on such kernels the ifobj is a pointer to a slab object that is freed by the del_dp hook function (when brcompat_mod is loaded). This bug may be just as present on older kernels, but there the ifobj is part of struct datapath, not a pointer, and thus it is much harder to trigger. Bug #1465. --- datapath/datapath.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/datapath/datapath.c b/datapath/datapath.c index 6df08437..7f496c59 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -276,18 +276,19 @@ static void do_destroy_dp(struct datapath *dp, struct list_head *dp_devs) struct net_bridge_port *p, *n; int i; + list_for_each_entry_safe (p, n, &dp->port_list, node) + if (p->port_no != ODPP_LOCAL) + dp_del_port(p, dp_devs); + if (dp_del_dp_hook) dp_del_dp_hook(dp); - /* Drop references to DP. */ - list_for_each_entry_safe (p, n, &dp->port_list, node) - dp_del_port(p, dp_devs); - rcu_assign_pointer(dps[dp->dp_idx], NULL); - /* Wait until no longer in use, then destroy it. */ - synchronize_rcu(); + dp_del_port(dp->ports[ODPP_LOCAL], dp_devs); + dp_table_destroy(dp->table, 1); + for (i = 0; i < DP_N_QUEUES; i++) skb_queue_purge(&dp->queues[i]); for (i = 0; i < DP_MAX_GROUPS; i++) @@ -1463,6 +1464,7 @@ static int dp_has_packet_of_interest(struct datapath *dp, int listeners) ssize_t openvswitch_read(struct file *f, char __user *buf, size_t nbytes, loff_t *ppos) { + /* XXX is there sufficient synchronization here? */ int listeners = (int) f->private_data; int dp_idx = iminor(f->f_dentry->d_inode); struct datapath *dp = get_dp(dp_idx); @@ -1517,6 +1519,7 @@ error: static unsigned int openvswitch_poll(struct file *file, poll_table *wait) { + /* XXX is there sufficient synchronization here? */ int dp_idx = iminor(file->f_dentry->d_inode); struct datapath *dp = get_dp(dp_idx); unsigned int mask; -- 2.30.2