[PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1845 bytes --]

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>

--- 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);




[-- Attachment #2: kexec-locking.patch --]
[-- Type: text/plain, Size: 1906 bytes --]

kexec: don't disable interrupts when acquiring load/unload lock

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>

--- 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);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2128 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 2969 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[David Vrabel]

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.

I've already dropped the irqsave from this lock in my kexec series and
can fold the other part in as well (which would now be in
kexec_swap_images()).

> Signed-off-by: Jan Beulich <jbeulich@suse.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;

I find the resulting spacing of this a bit unconventional.

    int ret;

    ret = xsm_kexec(XSM_PRIV)
    if ( ret )
        return ret;

is more standard I think.  Not too bothered though.

I'm about to post v10 of the kexec series and it would be less hassle
for me if you took the fix via that series instead since this area
undergoes significant refactoring.  But if you prefer to commit this fix
now:

Acked-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[Jan Beulich]

>>> On 06.11.13 at 14:20, David Vrabel <david.vrabel@citrix.com> wrote:
> 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.
> 
> I've already dropped the irqsave from this lock in my kexec series and
> can fold the other part in as well (which would now be in
> kexec_swap_images()).

That would generally be fine, but I'd like to get this fixed no matter
whether your patches make 4.4.

Jan

Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[David Vrabel]

On 06/11/13 14:17, Jan Beulich wrote:
>>>> On 06.11.13 at 14:20, David Vrabel <david.vrabel@citrix.com> wrote:
>> 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.
>>
>> I've already dropped the irqsave from this lock in my kexec series and
>> can fold the other part in as well (which would now be in
>> kexec_swap_images()).
> 
> That would generally be fine, but I'd like to get this fixed no matter
> whether your patches make 4.4.

Sure. I was only asking if you could wait a few days.

David

Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[Jan Beulich]

>>> On 06.11.13 at 16:09, David Vrabel <david.vrabel@citrix.com> wrote:
> On 06/11/13 14:17, Jan Beulich wrote:
>>>>> On 06.11.13 at 14:20, David Vrabel <david.vrabel@citrix.com> wrote:
>>> 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.
>>>
>>> I've already dropped the irqsave from this lock in my kexec series and
>>> can fold the other part in as well (which would now be in
>>> kexec_swap_images()).
>> 
>> That would generally be fine, but I'd like to get this fixed no matter
>> whether your patches make 4.4.
> 
> Sure. I was only asking if you could wait a few days.

I could, but I'm in no way convinced that a few days would help.

Jan

Re: [PATCH] kexec: don't disable interrupts when acquiring load/unload lock

[Daniel Kiper]

On Wed, Nov 06, 2013 at 12:22:22PM +0000, 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: Daniel Kiper <daniel.kiper@oracle.com>

Daniel