ia64/xen-unstable
changeset 10069:3dca5b4add2b
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@redhat.com>
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@redhat.com>
author | kaf24@firebug.cl.cam.ac.uk |
---|---|
date | Tue May 16 14:07:56 2006 +0100 (2006-05-16) |
parents | c20e766a1f72 |
children | aab3cd33d2ba |
files | linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c patches/linux-2.6.16.13/xenoprof-generic.patch |
line diff
1.1 --- a/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c Tue May 16 13:46:57 2006 +0100 1.2 +++ b/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c Tue May 16 14:07:56 2006 +0100 1.3 @@ -289,9 +289,13 @@ static int xenoprof_set_active(int * act 1.4 1.5 for (i=0; i<adomains; i++) { 1.6 domid = active_domains[i]; 1.7 + if (domid != active_domains[i]) { 1.8 + ret = -EINVAL; 1.9 + goto out; 1.10 + } 1.11 ret = HYPERVISOR_xenoprof_op(XENOPROF_set_active, &domid); 1.12 if (ret) 1.13 - return (ret); 1.14 + goto out; 1.15 if (active_domains[i] == 0) 1.16 set_dom0 = 1; 1.17 } 1.18 @@ -300,8 +304,11 @@ static int xenoprof_set_active(int * act 1.19 domid = 0; 1.20 ret = HYPERVISOR_xenoprof_op(XENOPROF_set_active, &domid); 1.21 } 1.22 - 1.23 - active_defined = 1; 1.24 + 1.25 +out: 1.26 + if (ret) 1.27 + HYPERVISOR_xenoprof_op(XENOPROF_reset_active_list, NULL); 1.28 + active_defined = !ret; 1.29 return ret; 1.30 } 1.31
2.1 --- a/patches/linux-2.6.16.13/xenoprof-generic.patch Tue May 16 13:46:57 2006 +0100 2.2 +++ b/patches/linux-2.6.16.13/xenoprof-generic.patch Tue May 16 14:07:56 2006 +0100 2.3 @@ -225,19 +225,21 @@ diff -pruN ../pristine-linux-2.6.16.13/d 2.4 struct oprofile_operations oprofile_ops; 2.5 2.6 unsigned long oprofile_started; 2.7 -@@ -33,6 +37,17 @@ static DECLARE_MUTEX(start_sem); 2.8 +@@ -33,6 +37,19 @@ static DECLARE_MUTEX(start_sem); 2.9 */ 2.10 static int timer = 0; 2.11 2.12 -+extern unsigned int adomains; 2.13 -+extern int active_domains[MAX_OPROF_DOMAINS]; 2.14 -+ 2.15 -+int oprofile_set_active(void) 2.16 ++int oprofile_set_active(int active_domains[], unsigned int adomains) 2.17 +{ 2.18 -+ if (oprofile_ops.set_active) 2.19 -+ return oprofile_ops.set_active(active_domains, adomains); 2.20 ++ int err; 2.21 ++ 2.22 ++ if (!oprofile_ops.set_active) 2.23 ++ return -EINVAL; 2.24 + 2.25 -+ return -EINVAL; 2.26 ++ down(&start_sem); 2.27 ++ err = oprofile_ops.set_active(active_domains, adomains); 2.28 ++ up(&start_sem); 2.29 ++ return err; 2.30 +} 2.31 + 2.32 int oprofile_setup(void) 2.33 @@ -251,7 +253,7 @@ diff -pruN ../pristine-linux-2.6.16.13/d 2.34 2.35 int oprofile_set_backtrace(unsigned long depth); 2.36 + 2.37 -+int oprofile_set_active(void); 2.38 ++int oprofile_set_active(int active_domains[], unsigned int adomains); 2.39 2.40 #endif /* OPROF_H */ 2.41 diff -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprofile_files.c ./drivers/oprofile/oprofile_files.c 2.42 @@ -280,7 +282,7 @@ diff -pruN ../pristine-linux-2.6.16.13/d 2.43 unsigned long fs_buffer_size = 131072; 2.44 unsigned long fs_cpu_buffer_size = 8192; 2.45 unsigned long fs_buffer_watershed = 32768; /* FIXME: tune */ 2.46 -@@ -117,11 +123,79 @@ static ssize_t dump_write(struct file * 2.47 +@@ -117,11 +123,108 @@ static ssize_t dump_write(struct file * 2.48 static struct file_operations dump_fops = { 2.49 .write = dump_write, 2.50 }; 2.51 @@ -288,63 +290,92 @@ diff -pruN ../pristine-linux-2.6.16.13/d 2.52 + 2.53 +#define TMPBUFSIZE 512 2.54 + 2.55 -+unsigned int adomains = 0; 2.56 -+long active_domains[MAX_OPROF_DOMAINS]; 2.57 ++static unsigned int adomains = 0; 2.58 ++static int active_domains[MAX_OPROF_DOMAINS + 1]; 2.59 ++static DEFINE_MUTEX(adom_mutex); 2.60 + 2.61 +static ssize_t adomain_write(struct file * file, char const __user * buf, 2.62 + size_t count, loff_t * offset) 2.63 +{ 2.64 -+ char tmpbuf[TMPBUFSIZE]; 2.65 -+ char * startp = tmpbuf; 2.66 -+ char * endp = tmpbuf; 2.67 ++ char *tmpbuf; 2.68 ++ char *startp, *endp; 2.69 + int i; 2.70 + unsigned long val; 2.71 ++ ssize_t retval = count; 2.72 + 2.73 + if (*offset) 2.74 + return -EINVAL; 2.75 -+ if (!count) 2.76 -+ return 0; 2.77 + if (count > TMPBUFSIZE - 1) 2.78 + return -EINVAL; 2.79 + 2.80 -+ memset(tmpbuf, 0x0, TMPBUFSIZE); 2.81 ++ if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL))) 2.82 ++ return -ENOMEM; 2.83 + 2.84 -+ if (copy_from_user(tmpbuf, buf, count)) 2.85 ++ if (copy_from_user(tmpbuf, buf, count)) { 2.86 ++ kfree(tmpbuf); 2.87 + return -EFAULT; 2.88 -+ 2.89 -+ for (i = 0; i < MAX_OPROF_DOMAINS; i++) 2.90 -+ active_domains[i] = -1; 2.91 -+ adomains = 0; 2.92 ++ } 2.93 ++ tmpbuf[count] = 0; 2.94 ++ 2.95 ++ mutex_lock(&adom_mutex); 2.96 + 2.97 -+ while (1) { 2.98 -+ val = simple_strtol(startp, &endp, 0); 2.99 ++ startp = tmpbuf; 2.100 ++ /* Parse one more than MAX_OPROF_DOMAINS, for easy error checking */ 2.101 ++ for (i = 0; i <= MAX_OPROF_DOMAINS; i++) { 2.102 ++ val = simple_strtoul(startp, &endp, 0); 2.103 + if (endp == startp) 2.104 + break; 2.105 -+ while (ispunct(*endp)) 2.106 ++ while (ispunct(*endp) || isspace(*endp)) 2.107 + endp++; 2.108 -+ active_domains[adomains++] = val; 2.109 -+ if (adomains >= MAX_OPROF_DOMAINS) 2.110 -+ break; 2.111 ++ active_domains[i] = val; 2.112 ++ if (active_domains[i] != val) 2.113 ++ /* Overflow, force error below */ 2.114 ++ i = MAX_OPROF_DOMAINS + 1; 2.115 + startp = endp; 2.116 + } 2.117 -+ if (oprofile_set_active()) 2.118 -+ return -EINVAL; 2.119 -+ return count; 2.120 ++ /* Force error on trailing junk */ 2.121 ++ adomains = *startp ? MAX_OPROF_DOMAINS + 1 : i; 2.122 ++ 2.123 ++ kfree(tmpbuf); 2.124 ++ 2.125 ++ if (adomains > MAX_OPROF_DOMAINS 2.126 ++ || oprofile_set_active(active_domains, adomains)) { 2.127 ++ adomains = 0; 2.128 ++ retval = -EINVAL; 2.129 ++ } 2.130 ++ 2.131 ++ mutex_unlock(&adom_mutex); 2.132 ++ return retval; 2.133 +} 2.134 + 2.135 +static ssize_t adomain_read(struct file * file, char __user * buf, 2.136 + size_t count, loff_t * offset) 2.137 +{ 2.138 -+ char tmpbuf[TMPBUFSIZE]; 2.139 -+ size_t len = 0; 2.140 ++ char * tmpbuf; 2.141 ++ size_t len; 2.142 + int i; 2.143 -+ /* This is all screwed up if we run out of space */ 2.144 -+ for (i = 0; i < adomains; i++) 2.145 -+ len += snprintf(tmpbuf + len, TMPBUFSIZE - len, 2.146 -+ "%u ", (unsigned int)active_domains[i]); 2.147 -+ len += snprintf(tmpbuf + len, TMPBUFSIZE - len, "\n"); 2.148 -+ return simple_read_from_buffer((void __user *)buf, count, 2.149 -+ offset, tmpbuf, len); 2.150 ++ ssize_t retval; 2.151 ++ 2.152 ++ if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL))) 2.153 ++ return -ENOMEM; 2.154 ++ 2.155 ++ mutex_lock(&adom_mutex); 2.156 ++ 2.157 ++ len = 0; 2.158 ++ for (i = 0; i < adomains; i++) 2.159 ++ len += snprintf(tmpbuf + len, 2.160 ++ len < TMPBUFSIZE ? TMPBUFSIZE - len : 0, 2.161 ++ "%u ", active_domains[i]); 2.162 ++ WARN_ON(len > TMPBUFSIZE); 2.163 ++ if (len != 0 && len <= TMPBUFSIZE) 2.164 ++ tmpbuf[len-1] = '\n'; 2.165 ++ 2.166 ++ mutex_unlock(&adom_mutex); 2.167 ++ 2.168 ++ retval = simple_read_from_buffer(buf, count, offset, tmpbuf, len); 2.169 ++ 2.170 ++ kfree(tmpbuf); 2.171 ++ return retval; 2.172 +} 2.173 + 2.174 +