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] [PATCH] Protect xenoprof.c global state

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] [PATCH] Protect xenoprof.c global state
From: Markus Armbruster <armbru@xxxxxxxxxx>
Date: Sat, 14 Oct 2006 08:50:31 +0200
Delivery-date: Fri, 13 Oct 2006 23:51:01 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux)
xen/arch/x86/oprofile/xenoprof.c accesses global state without
locking, which asks for random violence with SMP.

Stupidest conceivable fix: slap a lock around it.

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

diff -r efea4c3b8bcc xen/arch/x86/oprofile/xenoprof.c
--- a/xen/arch/x86/oprofile/xenoprof.c  Fri Oct 13 17:10:27 2006 +0100
+++ b/xen/arch/x86/oprofile/xenoprof.c  Fri Oct 13 20:09:36 2006 +0200
@@ -12,6 +12,9 @@
 
 /* Limit amount of pages used for shared buffer (per domain) */
 #define MAX_OPROF_SHARED_PAGES 32
+
+/* Lock protecting the following global state */
+static spinlock_t xenoprof_lock = SPIN_LOCK_UNLOCKED;
 
 struct domain *active_domains[MAX_OPROF_DOMAINS];
 int active_ready[MAX_OPROF_DOMAINS];
@@ -515,6 +517,8 @@ int do_xenoprof_op(int op, XEN_GUEST_HAN
         return -EPERM;
     }
 
+    spin_lock(&xenoprof_lock);
+    
     switch ( op )
     {
     case XENOPROF_init:
@@ -540,23 +544,31 @@ int do_xenoprof_op(int op, XEN_GUEST_HAN
     case XENOPROF_set_active:
     {
         domid_t domid;
-        if ( xenoprof_state != XENOPROF_IDLE )
-            return -EPERM;
-        if ( copy_from_guest(&domid, arg, 1) )
-            return -EFAULT;
+        if ( xenoprof_state != XENOPROF_IDLE ) {
+            ret = -EPERM;
+            break;
+        }
+        if ( copy_from_guest(&domid, arg, 1) ) {
+            ret = -EFAULT;
+            break;
+        }
         ret = add_active_list(domid);
         break;
     }
     case XENOPROF_set_passive:
     {
-        if ( xenoprof_state != XENOPROF_IDLE )
-            return -EPERM;
+        if ( xenoprof_state != XENOPROF_IDLE ) {
+            ret = -EPERM;
+            break;
+        }
         ret = add_passive_list(arg);
         break;
     }
     case XENOPROF_reserve_counters:
-        if ( xenoprof_state != XENOPROF_IDLE )
-            return -EPERM;
+        if ( xenoprof_state != XENOPROF_IDLE ) {
+            ret = -EPERM;
+            break;
+        }
         ret = nmi_reserve_counters();
         if ( !ret )
             xenoprof_state = XENOPROF_COUNTERS_RESERVED;
@@ -565,16 +577,20 @@ int do_xenoprof_op(int op, XEN_GUEST_HAN
     case XENOPROF_counter:
     {
         struct xenoprof_counter counter;
-        if ( xenoprof_state != XENOPROF_COUNTERS_RESERVED )
-            return -EPERM;
-        if ( adomains == 0 )
-            return -EPERM;
-
-        if ( copy_from_guest(&counter, arg, 1) )
-            return -EFAULT;
-
-        if ( counter.ind > OP_MAX_COUNTER )
-            return -E2BIG;
+        if ( xenoprof_state != XENOPROF_COUNTERS_RESERVED || adomains == 0) {
+            ret = -EPERM;
+            break;
+        }
+
+        if ( copy_from_guest(&counter, arg, 1) ) {
+            ret = -EFAULT;
+            break;
+        }
+
+        if ( counter.ind > OP_MAX_COUNTER ) {
+            ret = -E2BIG;
+            break;
+        }
 
         counter_config[counter.ind].count     = (unsigned long) counter.count;
         counter_config[counter.ind].enabled   = (unsigned long) 
counter.enabled;
@@ -588,8 +604,10 @@ int do_xenoprof_op(int op, XEN_GUEST_HAN
     }
 
     case XENOPROF_setup_events:
-        if ( xenoprof_state != XENOPROF_COUNTERS_RESERVED )
-            return -EPERM;
+        if ( xenoprof_state != XENOPROF_COUNTERS_RESERVED ) {
+            ret = -EPERM;
+            break;
+        }
         ret = nmi_setup_events();
         if ( !ret )
             xenoprof_state = XENOPROF_READY;
@@ -622,16 +640,20 @@ int do_xenoprof_op(int op, XEN_GUEST_HAN
         break;
 
     case XENOPROF_stop:
-        if ( xenoprof_state != XENOPROF_PROFILING )
-            return -EPERM;
+        if ( xenoprof_state != XENOPROF_PROFILING ) {
+            ret = -EPERM;
+            break;
+        }
         nmi_stop();
         xenoprof_state = XENOPROF_READY;
         break;
 
     case XENOPROF_disable_virq:
         if ( (xenoprof_state == XENOPROF_PROFILING) && 
-             (is_active(current->domain)) )
-            return -EPERM;
+             (is_active(current->domain)) ) {
+            ret = -EPERM;
+            break;
+        }
         ret = reset_active(current->domain);
         break;
 
@@ -663,6 +685,8 @@ int do_xenoprof_op(int op, XEN_GUEST_HAN
         ret = -EINVAL;
     }
 
+    spin_unlock(&xenoprof_lock);
+
     if ( ret < 0 )
         printk("xenoprof: operation %d failed for dom %d (status : %d)\n",
                op, current->domain->domain_id, ret);

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

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-devel] [PATCH] Protect xenoprof.c global state, Markus Armbruster <=