thr3ads.net - Xen devel - [Xen-devel] [PATCH 0/3] xenoprof fixes [May 2006]

If this information is useful, please help other people find it:
Share via:

Markus Armbruster

2006-May-15 16:31 UTC

[Xen-devel] [PATCH 0/3] xenoprof fixes

These patches address issues in the kernel part of xenoprof:

* Ill-advised use of on_each_cpu() can lead to sleep with interrupts
  disabled.

* Race conditions in active_domains code.

* Cleanup of active_domains code.

Comments welcome.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 16:31 UTC

head link

[Xen-devel] [PATCH 1/3] xenoprof fixes: sleep with interrupts disabled

Ill-advised use of on_each_cpu() can lead to sleep with interrupts
disabled.  This is best explained with a backtrace:

BUG: sleeping function called from invalid context at
/home/armbru/linux-2.6.tip-xen-quintela.hg/mm/slab.c:2783
in_atomic():0, irqs_disabled():1
 [<c0104f36>] show_trace+0x13/0x15
 [<c0105439>] dump_stack+0x18/0x1c
 [<c01161ac>] __might_sleep+0x9f/0xa7
 [<c0158823>] __kmalloc+0x65/0x110
 [<c018e01d>] proc_create+0x8d/0xe2
 [<c018e12d>] proc_mkdir_mode+0x1a/0x4d
 [<c018e173>] proc_mkdir+0x13/0x15
 [<c013de9b>] register_handler_proc+0x90/0x9e
 [<c013d729>] setup_irq+0xdf/0xf5
 [<c013d7a9>] request_irq+0x6a/0x8a
 [<c0231817>] bind_virq_to_irqhandler+0xe3/0x101
 [<f4c83b02>] bind_virq_cpu+0x28/0x62 [oprofile]
 [<c0121453>] on_each_cpu+0x36/0x5d
 [<f4c83c3b>] xenoprof_setup+0x22/0x14a [oprofile]
 [<f4c820bf>] oprofile_setup+0x45/0x8b [oprofile]
 [<f4c82fa3>] event_buffer_open+0x3e/0x62 [oprofile]
 [<c0159eda>] __dentry_open+0xc7/0x1b0
 [<c015a034>] nameidata_to_filp+0x20/0x34
 [<c015a078>] do_filp_open+0x30/0x39
 [<c015afe6>] do_sys_open+0x40/0xb0
 [<c015b07f>] sys_open+0x13/0x15
 [<c01048cf>] syscall_call+0x7/0xb

on_each_cpu() disables interrupts.  proc_create() calls passes
GFP_KERNEL to kmalloc().

The patch converts from on_each_cpu() to for_each_cpu(), and then
simplifies things.


diff -rup linux-2.6.16.13-xen/arch/i386/oprofile/xenoprof.c
linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c
--- linux-2.6.16.13-xen/arch/i386/oprofile/xenoprof.c	2006-05-15
10:32:22.000000000 +0200
+++ linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c	2006-05-15
10:30:49.000000000 +0200
@@ -141,56 +141,40 @@ xenoprof_ovf_interrupt(int irq, void * d
 }
 
 
-static void unbind_virq_cpu(void * info)
-{
-	int cpu = smp_processor_id();
-	if (ovf_irq[cpu] >= 0) {
-		unbind_from_irqhandler(ovf_irq[cpu], NULL);
-		ovf_irq[cpu] = -1;
-	}
-}
-
-
 static void unbind_virq(void)
 {
-	on_each_cpu(unbind_virq_cpu, NULL, 0, 1);
-}
-
-
-int bind_virq_error;
-
-static void bind_virq_cpu(void * info)
-{
-	int result;
-	int cpu = smp_processor_id();
+	int i;
 
-	result = bind_virq_to_irqhandler(VIRQ_XENOPROF,
-					 cpu,
-					 xenoprof_ovf_interrupt,
-					 SA_INTERRUPT,
-					 "xenoprof",
-					 NULL);
-
-	if (result<0) {
-		bind_virq_error = result;
-		printk("xenoprof.c: binding VIRQ_XENOPROF to IRQ failed on CPU "
-		       "%d\n", cpu);
-	} else {
-		ovf_irq[cpu] = result;
+	for_each_cpu(i) {
+		if (ovf_irq[i] >= 0) {
+			unbind_from_irqhandler(ovf_irq[i], NULL);
+			ovf_irq[i] = -1;
+		}
 	}
 }
 
 
 static int bind_virq(void)
 {
-	bind_virq_error = 0;
-	on_each_cpu(bind_virq_cpu, NULL, 0, 1);
-	if (bind_virq_error) {
-		unbind_virq();
-		return bind_virq_error;
-	} else {
-		return 0;
+	int i, result;
+
+	for_each_cpu(i) {
+		result = bind_virq_to_irqhandler(VIRQ_XENOPROF,
+						 i,
+						 xenoprof_ovf_interrupt,
+						 SA_INTERRUPT,
+						 "xenoprof",
+						 NULL);
+
+		if (result < 0) {
+			unbind_virq();
+			return result;
+		}
+
+		ovf_irq[i] = result;
 	}
+		
+	return 0;
 }
 
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 16:32 UTC

head link

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

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.


diff -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c
linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c	2006-05-15
10:31:11.000000000 +0200
@@ -42,10 +42,15 @@ extern int active_domains[MAX_OPROF_DOMA
 
 int oprofile_set_active(void)
 {
-	if (oprofile_ops.set_active)
-		return oprofile_ops.set_active(active_domains, adomains);
+	int err;
+
+	if (!oprofile_ops.set_active)
+		return -EINVAL;
 
-	return -EINVAL;
+	down(&start_sem);
+	err = oprofile_ops.set_active(active_domains, adomains);
+	up(&start_sem);
+	return err;
 }
 
 int oprofile_setup(void)
diff -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c
linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c	2006-05-15
10:31:11.000000000 +0200
@@ -128,6 +128,7 @@ static struct file_operations dump_fops 
 
 unsigned int adomains = 0;
 long active_domains[MAX_OPROF_DOMAINS];
+static DECLARE_MUTEX(adom_sem);
 
 static ssize_t adomain_write(struct file * file, char const __user * buf, 
 			     size_t count, loff_t * offset)
@@ -137,6 +138,7 @@ static ssize_t adomain_write(struct file
 	char * endp = tmpbuf;
 	int i;
 	unsigned long val;
+	ssize_t retval = count;
 	
 	if (*offset)
 		return -EINVAL;	
@@ -150,6 +152,8 @@ static ssize_t adomain_write(struct file
 	if (copy_from_user(tmpbuf, buf, count))
 		return -EFAULT;
 	
+	down(&adom_sem);
+
 	for (i = 0; i < MAX_OPROF_DOMAINS; i++)
 		active_domains[i] = -1;
 	adomains = 0;
@@ -165,9 +169,12 @@ static ssize_t adomain_write(struct file
 			break;
 		startp = endp;
 	}
+
 	if (oprofile_set_active())
-		return -EINVAL; 
-	return count;
+		retval = -EINVAL;
+
+	up(&adom_sem);
+	return retval;
 }
 
 static ssize_t adomain_read(struct file * file, char __user * buf, 
@@ -176,11 +183,17 @@ static ssize_t adomain_read(struct file 
 	char tmpbuf[TMPBUFSIZE];
 	size_t len = 0;
 	int i;
+
+	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");
+
+	up(&adom_sem);
+
 	return simple_read_from_buffer((void __user *)buf, count, 
 				       offset, tmpbuf, len);
 }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 16:33 UTC

head link

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

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@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-May-15 16:55 UTC

head link

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

These all look good to me. They fix some very bad bugs, Can you please 
provide a signed-off-by line, however?

  -- Keir

On 15 May 2006, at 17:31, Markus Armbruster wrote:
> These patches address issues in the kernel part of xenoprof:
>
> * Ill-advised use of on_each_cpu() can lead to sleep with interrupts
>   disabled.
>
> * Race conditions in active_domains code.
>
> * Cleanup of active_domains code.
>
> Comments welcome.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 17:14 UTC

head link

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

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:
> These all look good to me. They fix some very bad bugs, Can you please
> provide a signed-off-by line, however?
Oops, sorry.  Do you want me to resubmit with that?

Signed-off-by: Markus Armbruster <armbru@redhat.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-May-15 17:19 UTC

head link

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

On 15 May 2006, at 18:14, Markus Armbruster wrote:
>> These all look good to me. They fix some very bad bugs, Can you please
>> provide a signed-off-by line, however?
>
> Oops, sorry.  Do you want me to resubmit with that?
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Yeah, it''s supposed to be in the email with the patch. Just resend them
all.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 17:33 UTC

head link

[Xen-devel] Re: [PATCH 1/3] xenoprof fixes: sleep with interrupts disabled

Ill-advised use of on_each_cpu() can lead to sleep with interrupts
disabled.  This is best explained with a backtrace:

BUG: sleeping function called from invalid context at
/home/armbru/linux-2.6.tip-xen-quintela.hg/mm/slab.c:2783
in_atomic():0, irqs_disabled():1
 [<c0104f36>] show_trace+0x13/0x15
 [<c0105439>] dump_stack+0x18/0x1c
 [<c01161ac>] __might_sleep+0x9f/0xa7
 [<c0158823>] __kmalloc+0x65/0x110
 [<c018e01d>] proc_create+0x8d/0xe2
 [<c018e12d>] proc_mkdir_mode+0x1a/0x4d
 [<c018e173>] proc_mkdir+0x13/0x15
 [<c013de9b>] register_handler_proc+0x90/0x9e
 [<c013d729>] setup_irq+0xdf/0xf5
 [<c013d7a9>] request_irq+0x6a/0x8a
 [<c0231817>] bind_virq_to_irqhandler+0xe3/0x101
 [<f4c83b02>] bind_virq_cpu+0x28/0x62 [oprofile]
 [<c0121453>] on_each_cpu+0x36/0x5d
 [<f4c83c3b>] xenoprof_setup+0x22/0x14a [oprofile]
 [<f4c820bf>] oprofile_setup+0x45/0x8b [oprofile]
 [<f4c82fa3>] event_buffer_open+0x3e/0x62 [oprofile]
 [<c0159eda>] __dentry_open+0xc7/0x1b0
 [<c015a034>] nameidata_to_filp+0x20/0x34
 [<c015a078>] do_filp_open+0x30/0x39
 [<c015afe6>] do_sys_open+0x40/0xb0
 [<c015b07f>] sys_open+0x13/0x15
 [<c01048cf>] syscall_call+0x7/0xb

on_each_cpu() disables interrupts.  proc_create() calls passes
GFP_KERNEL to kmalloc().

The patch converts from on_each_cpu() to for_each_cpu(), and then
simplifies things.

Signed-off-by: Markus Armbruster <armbru@redhat.com>


diff -rup linux-2.6.16.13-xen/arch/i386/oprofile/xenoprof.c
linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c
--- linux-2.6.16.13-xen/arch/i386/oprofile/xenoprof.c	2006-05-15
10:32:22.000000000 +0200
+++ linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c	2006-05-15
10:30:49.000000000 +0200
@@ -141,56 +141,40 @@ xenoprof_ovf_interrupt(int irq, void * d
 }
 
 
-static void unbind_virq_cpu(void * info)
-{
-	int cpu = smp_processor_id();
-	if (ovf_irq[cpu] >= 0) {
-		unbind_from_irqhandler(ovf_irq[cpu], NULL);
-		ovf_irq[cpu] = -1;
-	}
-}
-
-
 static void unbind_virq(void)
 {
-	on_each_cpu(unbind_virq_cpu, NULL, 0, 1);
-}
-
-
-int bind_virq_error;
-
-static void bind_virq_cpu(void * info)
-{
-	int result;
-	int cpu = smp_processor_id();
+	int i;
 
-	result = bind_virq_to_irqhandler(VIRQ_XENOPROF,
-					 cpu,
-					 xenoprof_ovf_interrupt,
-					 SA_INTERRUPT,
-					 "xenoprof",
-					 NULL);
-
-	if (result<0) {
-		bind_virq_error = result;
-		printk("xenoprof.c: binding VIRQ_XENOPROF to IRQ failed on CPU "
-		       "%d\n", cpu);
-	} else {
-		ovf_irq[cpu] = result;
+	for_each_cpu(i) {
+		if (ovf_irq[i] >= 0) {
+			unbind_from_irqhandler(ovf_irq[i], NULL);
+			ovf_irq[i] = -1;
+		}
 	}
 }
 
 
 static int bind_virq(void)
 {
-	bind_virq_error = 0;
-	on_each_cpu(bind_virq_cpu, NULL, 0, 1);
-	if (bind_virq_error) {
-		unbind_virq();
-		return bind_virq_error;
-	} else {
-		return 0;
+	int i, result;
+
+	for_each_cpu(i) {
+		result = bind_virq_to_irqhandler(VIRQ_XENOPROF,
+						 i,
+						 xenoprof_ovf_interrupt,
+						 SA_INTERRUPT,
+						 "xenoprof",
+						 NULL);
+
+		if (result < 0) {
+			unbind_virq();
+			return result;
+		}
+
+		ovf_irq[i] = result;
 	}
+		
+	return 0;
 }
 
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 17:34 UTC

head link

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

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.

Signed-off-by: Markus Armbruster <armbru@redhat.com>


diff -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c
linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprof.c	2006-05-15
10:31:11.000000000 +0200
@@ -42,10 +42,15 @@ extern int active_domains[MAX_OPROF_DOMA
 
 int oprofile_set_active(void)
 {
-	if (oprofile_ops.set_active)
-		return oprofile_ops.set_active(active_domains, adomains);
+	int err;
+
+	if (!oprofile_ops.set_active)
+		return -EINVAL;
 
-	return -EINVAL;
+	down(&start_sem);
+	err = oprofile_ops.set_active(active_domains, adomains);
+	up(&start_sem);
+	return err;
 }
 
 int oprofile_setup(void)
diff -rup linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c
linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c	2006-05-15
10:31:11.000000000 +0200
@@ -128,6 +128,7 @@ static struct file_operations dump_fops 
 
 unsigned int adomains = 0;
 long active_domains[MAX_OPROF_DOMAINS];
+static DECLARE_MUTEX(adom_sem);
 
 static ssize_t adomain_write(struct file * file, char const __user * buf, 
 			     size_t count, loff_t * offset)
@@ -137,6 +138,7 @@ static ssize_t adomain_write(struct file
 	char * endp = tmpbuf;
 	int i;
 	unsigned long val;
+	ssize_t retval = count;
 	
 	if (*offset)
 		return -EINVAL;	
@@ -150,6 +152,8 @@ static ssize_t adomain_write(struct file
 	if (copy_from_user(tmpbuf, buf, count))
 		return -EFAULT;
 	
+	down(&adom_sem);
+
 	for (i = 0; i < MAX_OPROF_DOMAINS; i++)
 		active_domains[i] = -1;
 	adomains = 0;
@@ -165,9 +169,12 @@ static ssize_t adomain_write(struct file
 			break;
 		startp = endp;
 	}
+
 	if (oprofile_set_active())
-		return -EINVAL; 
-	return count;
+		retval = -EINVAL;
+
+	up(&adom_sem);
+	return retval;
 }
 
 static ssize_t adomain_read(struct file * file, char __user * buf, 
@@ -176,11 +183,17 @@ static ssize_t adomain_read(struct file 
 	char tmpbuf[TMPBUFSIZE];
 	size_t len = 0;
 	int i;
+
+	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");
+
+	up(&adom_sem);
+
 	return simple_read_from_buffer((void __user *)buf, count, 
 				       offset, tmpbuf, len);
 }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 17:34 UTC

head link

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

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@redhat.com>


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@lists.xensource.com
http://lists.xensource.com/xen-devel

Chris Wright

2006-May-15 18:53 UTC

head link

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

* Markus Armbruster (armbru@redhat.com) wrote:> --- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c
2006-05-15 10:28:11.000000000 +0200
> +++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c
2006-05-15 10:31:11.000000000 +0200
> @@ -128,6 +128,7 @@ static struct file_operations dump_fops 
>  
>  unsigned int adomains = 0;
>  long active_domains[MAX_OPROF_DOMAINS];
> +static DECLARE_MUTEX(adom_sem);
Sorry, didn''t mention this earlier.  Please use new mutex code here,
smth like:

s/DECLARE_MUTEX(adom_sec)/DEFINE_MUTEX(adomain_mutex)/
s/down(&adom_sem)/mutex_lock(&adomain_mutex)/
s/up(&adom_sem)/mutex_unlock(&admoain_mutex)/

thanks,
-chris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-15 19:15 UTC

head link

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

Chris Wright <chrisw@sous-sol.org> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> --- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c
2006-05-15 10:28:11.000000000 +0200
>> +++ linux-2.6.16.13-xen.patched-2/drivers/oprofile/oprofile_files.c
2006-05-15 10:31:11.000000000 +0200
>> @@ -128,6 +128,7 @@ static struct file_operations dump_fops 
>>  
>>  unsigned int adomains = 0;
>>  long active_domains[MAX_OPROF_DOMAINS];
>> +static DECLARE_MUTEX(adom_sem);
>
> Sorry, didn''t mention this earlier.  Please use new mutex code
here,
> smth like:
>
> s/DECLARE_MUTEX(adom_sec)/DEFINE_MUTEX(adomain_mutex)/
> s/down(&adom_sem)/mutex_lock(&adomain_mutex)/
> s/up(&adom_sem)/mutex_unlock(&admoain_mutex)/
>
> thanks,
> -chris
What about buffer_sem in drivers/oprofile/event_buffer.c and start_sem
in drivers/oprofile/oprof.c?  Want me to submit a patch to convert all
three?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Santos, Jose Renato G

2006-May-15 21:51 UTC

head link

RE: [Xen-devel] [PATCH 2/3] xenoprof fixes: active_domains races

>> > Sorry, didn''t mention this earlier.  Please use new mutex
>> code here, 
>> > smth like:
>> >
>> > s/DECLARE_MUTEX(adom_sec)/DEFINE_MUTEX(adomain_mutex)/
>> > s/down(&adom_sem)/mutex_lock(&adomain_mutex)/
>> > s/up(&adom_sem)/mutex_unlock(&admoain_mutex)/
>> >
>> > thanks,
>> > -chris
>> 
>> What about buffer_sem in drivers/oprofile/event_buffer.c and 
>> start_sem in drivers/oprofile/oprof.c?  Want me to submit a 
>> patch to convert all three?
>> 
   I think we should minimize changes to generic oprofile code
   (in drivers/oprofile) to minimize divergence from main stream linux.
   For this reason I don''t think we should touch buffer_sem and
   start_sem.

   Renato

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Chris Wright

2006-May-16 07:23 UTC

head link

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

* Santos, Jose Renato G (joserenato.santos@hp.com)
wrote:> >> What about buffer_sem in drivers/oprofile/event_buffer.c and 
> >> start_sem in drivers/oprofile/oprof.c?  Want me to submit a 
> >> patch to convert all three?
> 
>    I think we should minimize changes to generic oprofile code
>    (in drivers/oprofile) to minimize divergence from main stream linux.
>    For this reason I don''t think we should touch buffer_sem and
>    start_sem.
Well, you can certainly just send bits like that upstream.  You never
know, they might even appreciate the updates.

thanks,
-chris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-May-16 07:40 UTC

head link

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

On 15 May 2006, at 22:51, Santos, Jose Renato G wrote:
>>> What about buffer_sem in drivers/oprofile/event_buffer.c and
>>> start_sem in drivers/oprofile/oprof.c?  Want me to submit a
>>> patch to convert all three?
>>>
>
>    I think we should minimize changes to generic oprofile code
>    (in drivers/oprofile) to minimize divergence from main stream linux.
>    For this reason I don''t think we should touch buffer_sem and
>    start_sem.
Absolutely. If you clean those up the patches should go to the oprofile 
maintainers and lkml, not us.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-May-16 08:06 UTC

head link

Re: [Xen-devel] Re: [PATCH 1/3] xenoprof fixes: sleep with interrupts disabled

On 15 May 2006, at 18:33, Markus Armbruster wrote:
> on_each_cpu() disables interrupts.  proc_create() calls passes
> GFP_KERNEL to kmalloc().
>
> The patch converts from on_each_cpu() to for_each_cpu(), and then
> simplifies things.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Applied, thanks.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-May-16 08:09 UTC

head link

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

On 15 May 2006, at 18:34, Markus Armbruster wrote:
> 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.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Apart from updating to use the new mutex operations, patches 2 and 3 of 
your submitted set should be applied to 
patches/linux-2.6.16.13/xenoprof-generic.patch (yes, they need to be a 
patch of patch: sorry about that!). However, feel free to submit one 
mega-patch that merges both patches into one patch-of-a-patch.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-16 09:47 UTC

head link

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

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:

[...]> Apart from updating to use the new mutex operations, patches 2 and 3
> of your submitted set should be applied to
> patches/linux-2.6.16.13/xenoprof-generic.patch (yes, they need to be a
> patch of patch: sorry about that!). However, feel free to submit one
> mega-patch that merges both patches into one patch-of-a-patch.
>
>  -- Keir
Is it okay to include the conversion to mutex in xen-specific code?

There''s machinery to go from sparse tree + patches to full tree (make
prep-kernels).  Is there a way to propagate changes from full tree
pack to sparse tree + patches, where hg diff takes care of creating
patchfiles?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2006-May-16 09:48 UTC

head link

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

On 16 May 2006, at 10:47, Markus Armbruster wrote:
> Is it okay to include the conversion to mutex in xen-specific code?
Any semaphores that were added by the xenoprofile patches should be 
changed to mutexes. The rest should be left alone.
> There''s machinery to go from sparse tree + patches to full tree
(make
> prep-kernels).  Is there a way to propagate changes from full tree
> pack to sparse tree + patches, where hg diff takes care of creating
> patchfiles?
Not as far as I know. I touch them infrequently enough that when I do I 
simply re-create the patches manually.

If you want, provide a single patch in the same form as your two 
previous ones then I''ll turn it into patch-of-a-patch format and check 
it in.

  -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-16 11:47 UTC

head link

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

Keir Fraser <Keir.Fraser@cl.cam.ac.uk> writes:
> On 16 May 2006, at 10:47, Markus Armbruster wrote:
>
>> Is it okay to include the conversion to mutex in xen-specific code?
>
> Any semaphores that were added by the xenoprofile patches should be
> changed to mutexes. The rest should be left alone.
Done.  Patch for the rest just went to lkml.
>> There''s machinery to go from sparse tree + patches to full
tree (make
>> prep-kernels).  Is there a way to propagate changes from full tree
>> pack to sparse tree + patches, where hg diff takes care of creating
>> patchfiles?
>
> Not as far as I know. I touch them infrequently enough that when I do
> I simply re-create the patches manually.
>
> If you want, provide a single patch in the same form as your two
> previous ones then I''ll turn it into patch-of-a-patch format and
check
> it in.
On its way.  Thanks!

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Markus Armbruster

2006-May-16 11:48 UTC

head link

[Xen-devel] [PATCH] xenoprof fixes: active_domains races & cleanup

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>

diff -x ''*~'' -rup
linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c
linux-2.6.16.13-xen.patched-4/arch/i386/oprofile/xenoprof.c
--- linux-2.6.16.13-xen.patched-1/arch/i386/oprofile/xenoprof.c	2006-05-15
10:30:49.000000000 +0200
+++ linux-2.6.16.13-xen.patched-4/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 -x ''*~'' -rup
linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c
linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprof.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.c	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprof.c	2006-05-15
10:31:44.000000000 +0200
@@ -37,15 +37,17 @@ 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);
+	int err;
+
+	if (!oprofile_ops.set_active)
+		return -EINVAL;
 
-	return -EINVAL;
+	down(&start_sem);
+	err = oprofile_ops.set_active(active_domains, adomains);
+	up(&start_sem);
+	return err;
 }
 
 int oprofile_setup(void)
diff -x ''*~'' -rup
linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.h
linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprof.h
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprof.h	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprof.h	2006-05-16
13:32:42.000000000 +0200
@@ -36,6 +36,6 @@ void oprofile_timer_init(struct oprofile
 
 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 -x ''*~'' -rup
linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c
linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprofile_files.c
--- linux-2.6.16.13-xen.patched-1/drivers/oprofile/oprofile_files.c	2006-05-15
10:28:11.000000000 +0200
+++ linux-2.6.16.13-xen.patched-4/drivers/oprofile/oprofile_files.c	2006-05-16
13:42:58.000000000 +0200
@@ -126,63 +126,92 @@ 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 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 (!(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;
-	
-	for (i = 0; i < MAX_OPROF_DOMAINS; i++)
-		active_domains[i] = -1;
-	adomains = 0;
+	}
+	tmpbuf[count] = 0;
 
-	while (1) {
-		val = simple_strtol(startp, &endp, 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-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Reasonably Related Threads

Search for more possibly parallel threads

Xen devel - May 2006 - [PATCH 0/3] xenoprof fixes

[Xen-devel] [PATCH 0/3] xenoprof fixes

[Xen-devel] [PATCH 1/3] xenoprof fixes: sleep with interrupts disabled

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

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

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

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

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

[Xen-devel] Re: [PATCH 1/3] xenoprof fixes: sleep with interrupts disabled

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

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

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

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

RE: [Xen-devel] [PATCH 2/3] xenoprof fixes: active_domains races

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

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

Re: [Xen-devel] Re: [PATCH 1/3] xenoprof fixes: sleep with interrupts disabled

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

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

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

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

[Xen-devel] [PATCH] xenoprof fixes: active_domains races & cleanup

Reasonably Related Threads