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