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

[Markus Armbruster <armbru@xxxxxxxxxx>]

This is a cleanup 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.


diff -rup linux-2.6.16.13-xen.patched-2/arch/i386/oprofile/xenoprof.c 
linux-2.6.16.13-xen.patched-3/arch/i386/oprofile/xenoprof.c
--- linux-2.6.16.13-xen.patched-2/arch/i386/oprofile/xenoprof.c 2006-05-15 
10:30:49.000000000 +0200
+++ linux-2.6.16.13-xen.patched-3/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 -rup linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c 
linux-2.6.16.13-xen.patched-3/drivers/oprofile/oprof.c
--- linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c      2006-05-15 
10:31:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-3/drivers/oprofile/oprof.c      2006-05-15 
10:31:44.000000000 +0200
@@ -37,10 +37,7 @@ 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)
 {
        int err;
 
diff -rup linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.h 
linux-2.6.16.13-xen.patched-3/drivers/oprofile/oprof.h
--- linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.h      2006-05-15 
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-3/drivers/oprofile/oprof.h      2006-05-15 
10:31:23.000000000 +0200
@@ -36,6 +36,8 @@ void oprofile_timer_init(struct oprofile
 
 int oprofile_set_backtrace(unsigned long depth);
 
-int oprofile_set_active(void);
+#ifdef CONFIG_XEN
+int oprofile_set_active(int active_domains[], unsigned int adomains);
+#endif
  
 #endif /* OPROF_H */
diff -rup linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c 
linux-2.6.16.13-xen.patched-3/drivers/oprofile/oprofile_files.c
--- linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c     
2006-05-15 10:31:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-3/drivers/oprofile/oprofile_files.c     
2006-05-15 10:31:23.000000000 +0200
@@ -126,52 +126,59 @@ 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 DECLARE_MUTEX(adom_sem);
 
 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;
-       
-       down(&adom_sem);
+       }
+       tmpbuf[count] = 0;
 
-       for (i = 0; i < MAX_OPROF_DOMAINS; i++)
-               active_domains[i] = -1;
-       adomains = 0;
+       down(&adom_sem);
 
-       while (1) {
-               val = simple_strtol(startp, &endp, 0);
+       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;
        }
+       /* Force error on trailing junk */
+       adomains = *startp ? MAX_OPROF_DOMAINS + 1 : i;
 
-       if (oprofile_set_active())
+       kfree(tmpbuf);
+
+       if (adomains > MAX_OPROF_DOMAINS
+           || oprofile_set_active(active_domains, adomains)) {
+               adomains = 0;
                retval = -EINVAL;
+       }
 
        up(&adom_sem);
        return retval;
@@ -180,22 +187,31 @@ static ssize_t adomain_write(struct file
 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;
+       ssize_t retval;
+
+       if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL)))
+               return -ENOMEM;
 
        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");
+       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';
 
        up(&adom_sem);
 
-       return simple_read_from_buffer((void __user *)buf, count, 
-                                      offset, tmpbuf, len);
+       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