From: Ben Pfaff <blp@nicira.com>
Date: Fri, 26 Jun 2009 21:15:04 +0000 (-0700)
Subject: datapath: Fix use-after-free error in datapath destruction.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7793a4aaeb0d550d71fab4b7a5c075b0526c8b05;p=openvswitch

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.
---

diff --git a/datapath/datapath.c b/datapath/datapath.c
index e62eaea1..14d83cd9 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -277,18 +277,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++)
@@ -1469,6 +1470,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);
@@ -1523,6 +1525,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;