WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] Re: [PATCH 3/3] xenoprof fixes: active_domains cleanup
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Mon, 15 May 2006 19:34:57 +0200
Delivery-date: Mon, 15 May 2006 10:36:22 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <874pzr2qpr.fsf@xxxxxxxxxxxxxxxxx> (Markus Armbruster's message of "Mon, 15 May 2006 18:33:04 +0200")
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <87hd3r2qst.fsf@xxxxxxxxxxxxxxxxx> <874pzr2qpr.fsf@xxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux)
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.

Signed-off-by: Markus Armbruster <armbru@xxxxxxxxxx>


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