From: Jesse Gross <jesse@nicira.com>
Date: Wed, 8 Dec 2010 21:19:05 +0000 (-0800)
Subject: datapath: Hold mutex for DP while userspace is blocking on read.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e22d4953ec00df611fff0fd36ac81087c63e1d1e;p=openvswitch

datapath: Hold mutex for DP while userspace is blocking on read.

Currently we get a pointer to the DP in openvswitch_read() and
openvswitch_poll() and use it without any synchronization.  This means
that the DP could disappear from underneath us while we are using it.
Currently, this isn't a problem because userspace is single threaded but
it's better for the locking to be correct.

With this change we hold the mutex while doing a blocking wait, which
means that no changes can be made, including adding/removing flows.  It's
possible to make this finer grained but for the time being that isn't done,
since current userspace doesn't care.

Found with lockdep.

Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
---

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 08e7450e..49dcec50 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1841,10 +1841,9 @@ fault:
 static ssize_t openvswitch_read(struct file *f, char __user *buf,
 				size_t nbytes, loff_t *ppos)
 {
-	/* XXX is there sufficient synchronization here? */
 	int listeners = get_listen_mask(f);
 	int dp_idx = iminor(f->f_dentry->d_inode);
-	struct datapath *dp = get_dp(dp_idx);
+	struct datapath *dp = get_dp_locked(dp_idx);
 	struct sk_buff *skb;
 	size_t copy_bytes, tot_copy_bytes;
 	int retval;
@@ -1881,6 +1880,8 @@ static ssize_t openvswitch_read(struct file *f, char __user *buf,
 		}
 	}
 success:
+	mutex_unlock(&dp->mutex);
+
 	copy_bytes = tot_copy_bytes = min_t(size_t, skb->len, nbytes);
 
 	retval = 0;
@@ -1918,16 +1919,17 @@ success:
 		retval = tot_copy_bytes;
 
 	kfree_skb(skb);
+	return retval;
 
 error:
+	mutex_unlock(&dp->mutex);
 	return retval;
 }
 
 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);
+	struct datapath *dp = get_dp_locked(dp_idx);
 	unsigned int mask;
 
 	if (dp) {
@@ -1935,6 +1937,7 @@ static unsigned int openvswitch_poll(struct file *file, poll_table *wait)
 		poll_wait(file, &dp->waitqueue, wait);
 		if (dp_has_packet_of_interest(dp, get_listen_mask(file)))
 			mask |= POLLIN | POLLRDNORM;
+		mutex_unlock(&dp->mutex);
 	} else {
 		mask = POLLIN | POLLRDNORM | POLLHUP;
 	}