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

[Xen-changelog] Fixes to the xenoprofile Linux driver.

To: xen-changelog@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-changelog] Fixes to the xenoprofile Linux driver.
From: Xen patchbot-unstable <patchbot-unstable@xxxxxxxxxxxxxxxxxxx>
Date: Tue, 16 May 2006 17:04:14 +0000
Delivery-date: Tue, 16 May 2006 10:06:11 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-changelog-request@lists.xensource.com?subject=help>
List-id: BK change log <xen-changelog.lists.xensource.com>
List-post: <mailto:xen-changelog@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=unsubscribe>
Reply-to: xen-devel@xxxxxxxxxxxxxxxxxxx
Sender: xen-changelog-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User kaf24@xxxxxxxxxxxxxxxxxxxx
# Node ID 3dca5b4add2be8b6da2052aeddffc497920b9ce1
# Parent  c20e766a1f725013b8f7409649291c1267fa8be6
Fixes to the xenoprofile Linux driver.

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>
---
 linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c |   13 ++++-
 patches/linux-2.6.16.13/xenoprof-generic.patch     |   46 ---------------------
 2 files changed, 12 insertions(+), 47 deletions(-)

diff -r c20e766a1f72 -r 3dca5b4add2b 
linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c
--- a/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c        Tue May 16 
13:46:57 2006 +0100
+++ b/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c        Tue May 16 
14:07:56 2006 +0100
@@ -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 -r c20e766a1f72 -r 3dca5b4add2b 
patches/linux-2.6.16.13/xenoprof-generic.patch
--- a/patches/linux-2.6.16.13/xenoprof-generic.patch    Tue May 16 13:46:57 
2006 +0100
+++ b/patches/linux-2.6.16.13/xenoprof-generic.patch    Tue May 16 14:07:56 
2006 +0100
@@ -225,19 +225,21 @@ diff -pruN ../pristine-linux-2.6.16.13/d
  struct oprofile_operations oprofile_ops;
  
  unsigned long oprofile_started;
-@@ -33,6 +37,17 @@ static DECLARE_MUTEX(start_sem);
+@@ -33,6 +37,19 @@ 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);
-+
-+      return -EINVAL;
++      int err;
++
++      if (!oprofile_ops.set_active)
++              return -EINVAL;
++
++      down(&start_sem);
++      err = oprofile_ops.set_active(active_domains, adomains);
++      up(&start_sem);
++      return err;
 +}
 +
  int oprofile_setup(void)
@@ -251,7 +253,7 @@ diff -pruN ../pristine-linux-2.6.16.13/d
  
  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 -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprofile_files.c 
./drivers/oprofile/oprofile_files.c
@@ -280,7 +282,7 @@ diff -pruN ../pristine-linux-2.6.16.13/d
  unsigned long fs_buffer_size = 131072;
  unsigned long fs_cpu_buffer_size = 8192;
  unsigned long fs_buffer_watershed = 32768; /* FIXME: tune */
-@@ -117,11 +123,79 @@ static ssize_t dump_write(struct file * 
+@@ -117,11 +123,108 @@ static ssize_t dump_write(struct file * 
  static struct file_operations dump_fops = {
        .write          = dump_write,
  };
@@ -288,63 +290,92 @@ diff -pruN ../pristine-linux-2.6.16.13/d
 +
 +#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 (copy_from_user(tmpbuf, buf, count))
++      if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL)))
++              return -ENOMEM;
++
++      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;
-+
-+      while (1) {
-+              val = simple_strtol(startp, &endp, 0);
++      }
++      tmpbuf[count] = 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-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-changelog

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-changelog] Fixes to the xenoprofile Linux driver., Xen patchbot-unstable <=