Xen devel - Nov 2013 - [PATCH] 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);





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

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


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

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

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

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

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

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