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.
Also clean up the code dealing with /dev/oprofile/active_domains:
* Use parameters instead of global variables to pass domain ids
 around. Give those globals internal linkage.
* Allocate buffers dynamically to conserve stack space.
* Treat writes with size zero exactly like a write containing no
 domain id. Before, zero-sized write was ignored, which is not the
 same.
* Parse domain ids as unsigned numbers. Before, the first one was
 parsed as signed number.
 Because ispunct()-punctuation is ignored between domain ids, signs
 are still silently ignored except for the first number. Hmm.
* Make parser accept whitespace as domain separator, because that's
 what you get when reading the file.
* EINVAL on domain ids overflowing domid_t. Before, they were
 silently truncated.
* EINVAL on too many domain ids. Before, the excess ones were
 silently ignored.
* Reset active domains on failure halfway through setting them.
* Fix potential buffer overflow in adomain_read(). Couldn't really
 happen because buffer was sufficient for current value of
 MAX_OPROF_DOMAINS.
Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>
diff -x '*~' -rup linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c 
linux-2.6.16.13-xen.patched-4/arch/i386/oprofile/xenoprof.c
--- linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c 2006年05月15日 
10:30:49.000000000 +0200
+++ linux-2.6.16.13-xen.patched-4/arch/i386/oprofile/xenoprof.c 2006年05月15日 
10:31:23.000000000 +0200
@@ -289,9 +289,13 @@ static int xenoprof_set_active(int * act
 
 for (i=0; i<adomains; i++) {
 domid = active_domains[i];
+ if (domid != active_domains[i]) {
+ ret = -EINVAL;
+ goto out;
+ }
 ret = HYPERVISOR_xenoprof_op(XENOPROF_set_active, &domid);
 if (ret)
- return (ret);
+ goto out;
 if (active_domains[i] == 0)
 set_dom0 = 1;
 }
@@ -300,8 +304,11 @@ static int xenoprof_set_active(int * act
 domid = 0;
 ret = HYPERVISOR_xenoprof_op(XENOPROF_set_active, &domid);
 }
- 
- active_defined = 1;
+
+out:
+ if (ret)
+ HYPERVISOR_xenoprof_op(XENOPROF_reset_active_list, NULL);
+ active_defined = !ret;
 return ret;
 }
 
diff -x '*~' -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c 
linux-2.6.16.13-xen.patched-4/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-4/drivers/oprofile/oprof.c 2006年05月15日 
10:31:44.000000000 +0200
@@ -37,15 +37,17 @@ static DECLARE_MUTEX(start_sem);
 */
 static int timer = 0;
 
-extern unsigned int adomains;
-extern int active_domains[MAX_OPROF_DOMAINS];
-
-int oprofile_set_active(void)
+int oprofile_set_active(int active_domains[], unsigned int adomains)
 {
- 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 -x '*~' -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.h 
linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprof.h
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.h 2006年05月15日 
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprof.h 2006年05月16日 
13:32:42.000000000 +0200
@@ -36,6 +36,6 @@ void oprofile_timer_init(struct oprofile
 
 int oprofile_set_backtrace(unsigned long depth);
 
-int oprofile_set_active(void);
+int oprofile_set_active(int active_domains[], unsigned int adomains);
 
 #endif /* OPROF_H */
diff -x '*~' -rup 
linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c 
linux-2.6.16.13-xen.patched-4/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-4/drivers/oprofile/oprofile_files.c 
2006年05月16日 13:42:58.000000000 +0200
@@ -126,63 +126,92 @@ static struct file_operations dump_fops 
 
 #define TMPBUFSIZE 512
 
-unsigned int adomains = 0;
-long active_domains[MAX_OPROF_DOMAINS];
+static unsigned int adomains = 0;
+static int active_domains[MAX_OPROF_DOMAINS + 1];
+static DEFINE_MUTEX(adom_mutex);
 
 static ssize_t adomain_write(struct file * file, char const __user * buf, 
 size_t count, loff_t * offset)
 {
- char tmpbuf[TMPBUFSIZE];
- char * startp = tmpbuf;
- char * endp = tmpbuf;
+ char *tmpbuf;
+ char *startp, *endp;
 int i;
 unsigned long val;
+ ssize_t retval = count;
 
 if (*offset)
 return -EINVAL; 
- if (!count)
- return 0;
 if (count > TMPBUFSIZE - 1)
 return -EINVAL;
 
- memset(tmpbuf, 0x0, TMPBUFSIZE);
+ if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL)))
+ return -ENOMEM;
 
- if (copy_from_user(tmpbuf, buf, count))
+ if (copy_from_user(tmpbuf, buf, count)) {
+ kfree(tmpbuf);
 return -EFAULT;
- 
- for (i = 0; i < MAX_OPROF_DOMAINS; i++)
- active_domains[i] = -1;
- adomains = 0;
+ }
+ tmpbuf[count] = 0;
 
- while (1) {
- val = simple_strtol(startp, &endp, 0);
+ mutex_lock(&adom_mutex);
+
+ startp = tmpbuf;
+ /* Parse one more than MAX_OPROF_DOMAINS, for easy error checking */
+ for (i = 0; i <= MAX_OPROF_DOMAINS; i++) {
+ val = simple_strtoul(startp, &endp, 0);
 if (endp == startp)
 break;
- while (ispunct(*endp))
+ while (ispunct(*endp) || isspace(*endp))
 endp++;
- active_domains[adomains++] = val;
- if (adomains >= MAX_OPROF_DOMAINS)
- break;
+ active_domains[i] = val;
+ if (active_domains[i] != val)
+ /* Overflow, force error below */
+ i = MAX_OPROF_DOMAINS + 1;
 startp = endp;
 }
- if (oprofile_set_active())
- return -EINVAL; 
- return count;
+ /* Force error on trailing junk */
+ adomains = *startp ? MAX_OPROF_DOMAINS + 1 : i;
+
+ kfree(tmpbuf);
+
+ if (adomains > MAX_OPROF_DOMAINS
+ || oprofile_set_active(active_domains, adomains)) {
+ adomains = 0;
+ retval = -EINVAL;
+ }
+
+ mutex_unlock(&adom_mutex);
+ return retval;
 }
 
 static ssize_t adomain_read(struct file * file, char __user * buf, 
 size_t count, loff_t * offset)
 {
- char tmpbuf[TMPBUFSIZE];
- size_t len = 0;
+ char * tmpbuf;
+ size_t len;
 int i;
- /* 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");
- return simple_read_from_buffer((void __user *)buf, count, 
- offset, tmpbuf, len);
+ ssize_t retval;
+
+ if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL)))
+ return -ENOMEM;
+
+ mutex_lock(&adom_mutex);
+
+ len = 0;
+ for (i = 0; i < adomains; i++)
+ len += snprintf(tmpbuf + len,
+ len < TMPBUFSIZE ? TMPBUFSIZE - len : 0,
+ "%u ", active_domains[i]);
+ WARN_ON(len > TMPBUFSIZE);
+ if (len != 0 && len <= TMPBUFSIZE)
+ tmpbuf[len-1] = '\n';
+
+ mutex_unlock(&adom_mutex);
+
+ retval = simple_read_from_buffer(buf, count, offset, tmpbuf, len);
+
+ kfree(tmpbuf);
+ return retval;
 }
 
 
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel