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>
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 ++int oprofile_set_active(int active_domains[], unsigned int adomains)
    2.15 ++{
    2.16 ++	int err;
    2.17  +
    2.18 -+int oprofile_set_active(void)
    2.19 -+{
    2.20 -+	if (oprofile_ops.set_active)
    2.21 -+		return oprofile_ops.set_active(active_domains, adomains);
    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 -+	while (1) {
    2.96 -+		val = simple_strtol(startp, &endp, 0);
    2.97 ++	mutex_lock(&adom_mutex);
    2.98 ++
    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  +