From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock Date: Wed, 6 Nov 2013 13:12:02 +0000 Message-ID: <527A4022.2030107@citrix.com> References: <527A428E0200007800100156@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4598036033171243704==" Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Ve2ty-0006o6-LD for xen-devel@lists.xenproject.org; Wed, 06 Nov 2013 13:12:06 +0000 In-Reply-To: <527A428E0200007800100156@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Daniel Kiper , Keir Fraser , David Vrabel List-Id: xen-devel@lists.xenproject.org --===============4598036033171243704== Content-Type: multipart/alternative; boundary="------------050709010602050500070809" --------------050709010602050500070809 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 06/11/13 12:22, Jan Beulich wrote: > This doesn't appear to have served any purpose other than causing > map_pages_to_xen() to be (incorrectly) invoked with interrupts > disabled. In particular, serialization against actual kexec-ing is done > without this lock being involved. Clarify the scope of the lock at once > by making it local to do_kexec_op_internal(). > > Once at it, also drop a pointless initializer. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/common/kexec.c > +++ b/xen/common/kexec.c > @@ -55,8 +55,6 @@ static xen_kexec_image_t kexec_image[KEX > > static unsigned long kexec_flags = 0; /* the lowest bits are for KEXEC_IMAGE... */ > > -static spinlock_t kexec_lock = SPIN_LOCK_UNLOCKED; > - > static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > static size_t vmcoreinfo_size = 0; > > @@ -851,10 +849,9 @@ static int do_kexec_op_internal(unsigned > XEN_GUEST_HANDLE_PARAM(void) uarg, > bool_t compat) > { > - unsigned long flags; > - int ret = -EINVAL; > + static DEFINE_SPINLOCK(kexec_lock); > + int ret = xsm_kexec(XSM_PRIV); > > - ret = xsm_kexec(XSM_PRIV); > if ( ret ) > return ret; > > @@ -868,7 +865,7 @@ static int do_kexec_op_internal(unsigned > break; > case KEXEC_CMD_kexec_load: > case KEXEC_CMD_kexec_unload: > - spin_lock_irqsave(&kexec_lock, flags); > + spin_lock(&kexec_lock); > if (!test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags)) > { > if (compat) > @@ -876,7 +873,7 @@ static int do_kexec_op_internal(unsigned > else > ret = kexec_load_unload(op, uarg); > } > - spin_unlock_irqrestore(&kexec_lock, flags); > + spin_unlock(&kexec_lock); > break; > case KEXEC_CMD_kexec: > ret = kexec_exec(uarg); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------050709010602050500070809 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 06/11/13 12:22, Jan Beulich wrote:
This doesn't appear to have served any purpose other than causing
map_pages_to_xen() to be (incorrectly) invoked with interrupts
disabled. In particular, serialization against actual kexec-ing is done
without this lock being involved. Clarify the scope of the lock at once
by making it local to do_kexec_op_internal().

Once at it, also drop a pointless initializer.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -55,8 +55,6 @@ static xen_kexec_image_t kexec_image[KEX
 
 static unsigned long kexec_flags = 0; /* the lowest bits are for KEXEC_IMAGE... */
 
-static spinlock_t kexec_lock = SPIN_LOCK_UNLOCKED;
-
 static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES];
 static size_t vmcoreinfo_size = 0;
 
@@ -851,10 +849,9 @@ static int do_kexec_op_internal(unsigned
                                 XEN_GUEST_HANDLE_PARAM(void) uarg,
                                 bool_t compat)
 {
-    unsigned long flags;
-    int ret = -EINVAL;
+    static DEFINE_SPINLOCK(kexec_lock);
+    int ret = xsm_kexec(XSM_PRIV);
 
-    ret = xsm_kexec(XSM_PRIV);
     if ( ret )
         return ret;
 
@@ -868,7 +865,7 @@ static int do_kexec_op_internal(unsigned
         break;
     case KEXEC_CMD_kexec_load:
     case KEXEC_CMD_kexec_unload:
-        spin_lock_irqsave(&kexec_lock, flags);
+        spin_lock(&kexec_lock);
         if (!test_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags))
         {
                 if (compat)
@@ -876,7 +873,7 @@ static int do_kexec_op_internal(unsigned
                 else
                         ret = kexec_load_unload(op, uarg);
         }
-        spin_unlock_irqrestore(&kexec_lock, flags);
+        spin_unlock(&kexec_lock);
         break;
     case KEXEC_CMD_kexec:
         ret = kexec_exec(uarg);






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

--------------050709010602050500070809-- --===============4598036033171243704== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4598036033171243704==--