[PATCH 2/3] xenoprof fixes: active_domains races

[Markus Armbruster <armbru@redhat.com>]

The active_domains code has race conditions:

* oprofile_set_active() calls set_active() method without holding
  start_sem.  This is clearly wrong, as xenoprof_set_active() makes
  several hypercalls.  oprofile_start(), for instance, could run in
  the middle of xenoprof_set_active().

* adomain_write(), adomain_read() and xenoprof_set_active() access
  global active_domains[] and adomains without synchronization.  I
  went for a simple, obvious fix and created another mutex.  Instead,
  one could move the shared data into oprof.c and protect it with
  start_sem, but that's more invasive.


diff -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c	2006-05-15 10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c	2006-05-15 10:31:11.000000000 +0200
@@ -42,10 +42,15 @@ extern int active_domains[MAX_OPROF_DOMA
 
 int oprofile_set_active(void)
 {
-	if (oprofile_ops.set_active)
-		return oprofile_ops.set_active(active_domains, adomains);
+	int err;
+
+	if (!oprofile_ops.set_active)
+		return -EINVAL;
 
-	return -EINVAL;
+	down(&start_sem);
+	err = oprofile_ops.set_active(active_domains, adomains);
+	up(&start_sem);
+	return err;
 }
 
 int oprofile_setup(void)
diff -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c	2006-05-15 10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c	2006-05-15 10:31:11.000000000 +0200
@@ -128,6 +128,7 @@ static struct file_operations dump_fops 
 
 unsigned int adomains = 0;
 long active_domains[MAX_OPROF_DOMAINS];
+static DECLARE_MUTEX(adom_sem);
 
 static ssize_t adomain_write(struct file * file, char const __user * buf, 
 			     size_t count, loff_t * offset)
@@ -137,6 +138,7 @@ static ssize_t adomain_write(struct file
 	char * endp = tmpbuf;
 	int i;
 	unsigned long val;
+	ssize_t retval = count;
 	
 	if (*offset)
 		return -EINVAL;	
@@ -150,6 +152,8 @@ static ssize_t adomain_write(struct file
 	if (copy_from_user(tmpbuf, buf, count))
 		return -EFAULT;
 	
+	down(&adom_sem);
+
 	for (i = 0; i < MAX_OPROF_DOMAINS; i++)
 		active_domains[i] = -1;
 	adomains = 0;
@@ -165,9 +169,12 @@ static ssize_t adomain_write(struct file
 			break;
 		startp = endp;
 	}
+
 	if (oprofile_set_active())
-		return -EINVAL; 
-	return count;
+		retval = -EINVAL;
+
+	up(&adom_sem);
+	return retval;
 }
 
 static ssize_t adomain_read(struct file * file, char __user * buf, 
@@ -176,11 +183,17 @@ static ssize_t adomain_read(struct file 
 	char tmpbuf[TMPBUFSIZE];
 	size_t len = 0;
 	int i;
+
+	down(&adom_sem);
+
 	/* This is all screwed up if we run out of space */
 	for (i = 0; i < adomains; i++) 
 		len += snprintf(tmpbuf + len, TMPBUFSIZE - len, 
 				"%u ", (unsigned int)active_domains[i]);
 	len += snprintf(tmpbuf + len, TMPBUFSIZE - len, "\n");
+
+	up(&adom_sem);
+
 	return simple_read_from_buffer((void __user *)buf, count, 
 				       offset, tmpbuf, len);
 }