[Xen-devel] [PATCH 2/3] xenoprof fixes: active_domains races

[Markus Armbruster <armbru@xxxxxxxxxx>]

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);
 }

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel