[PATCH] x86: prevent call to xfree() in dump_irqs() while in an irq context

[PATCH] x86: prevent call to xfree() in dump_irqs() while in an irq context

[Jan Beulich]

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

Because of c/s 24707:96987c324a4f, dump_irqs() can now be called in an
irq context when a bug condition is encountered. If this is the case,
ignore the call to xsm_show_irq_ssid() and the subsequent call to
xfree().

This prevents an assertion failure in xfree(), and should allow all the
debug information to be dumped, before failing with a BUG() because of
the underlying race condition we are attempting to reproduce.

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

Rather than using the non-obvious conditional around an xfree() that
would be passed NULL only in the inverse case (which could easily get
removed by a future change on the basis that calling xfree(NULL) is
benign), switch the order of checks in xfree() itself and only suppress
the call to XSM that could potentially call xmalloc().

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

--- 2012-04-23.orig/xen/arch/x86/irq.c	2012-05-14 17:43:58.000000000 +0200
+++ 2012-04-23/xen/arch/x86/irq.c	2012-05-21 15:38:01.000000000 +0200
@@ -2060,7 +2060,7 @@ static void dump_irqs(unsigned char key)
         if ( !irq_desc_initialized(desc) || desc->handler == &no_irq_type )
             continue;
 
-        ssid = xsm_show_irq_sid(irq);
+        ssid = in_irq() ? NULL : xsm_show_irq_sid(irq);
 
         spin_lock_irqsave(&desc->lock, flags);
 
--- 2012-04-23.orig/xen/common/xmalloc_tlsf.c	2011-10-17 08:35:00.000000000 +0200
+++ 2012-04-23/xen/common/xmalloc_tlsf.c	2012-05-21 15:38:31.000000000 +0200
@@ -604,11 +604,11 @@ void xfree(void *p)
 {
     struct bhdr *b;
 
-    ASSERT(!in_irq());
-
     if ( p == NULL )
         return;
 
+    ASSERT(!in_irq());
+
     /* Strip alignment padding. */
     b = (struct bhdr *)((char *) p - BHDR_OVERHEAD);
     if ( b->size & 1 )




[-- Attachment #2: x86-irq-fix-dump.patch --]
[-- Type: text/plain, Size: 1873 bytes --]

x86: prevent call to xfree() in dump_irqs() while in an irq context

Because of c/s 24707:96987c324a4f, dump_irqs() can now be called in an
irq context when a bug condition is encountered. If this is the case,
ignore the call to xsm_show_irq_ssid() and the subsequent call to
xfree().

This prevents an assertion failure in xfree(), and should allow all the
debug information to be dumped, before failing with a BUG() because of
the underlying race condition we are attempting to reproduce.

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

Rather than using the non-obvious conditional around an xfree() that
would be passed NULL only in the inverse case (which could easily get
removed by a future change on the basis that calling xfree(NULL) is
benign), switch the order of checks in xfree() itself and only suppress
the call to XSM that could potentially call xmalloc().

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

--- 2012-04-23.orig/xen/arch/x86/irq.c	2012-05-14 17:43:58.000000000 +0200
+++ 2012-04-23/xen/arch/x86/irq.c	2012-05-21 15:38:01.000000000 +0200
@@ -2060,7 +2060,7 @@ static void dump_irqs(unsigned char key)
         if ( !irq_desc_initialized(desc) || desc->handler == &no_irq_type )
             continue;
 
-        ssid = xsm_show_irq_sid(irq);
+        ssid = in_irq() ? NULL : xsm_show_irq_sid(irq);
 
         spin_lock_irqsave(&desc->lock, flags);
 
--- 2012-04-23.orig/xen/common/xmalloc_tlsf.c	2011-10-17 08:35:00.000000000 +0200
+++ 2012-04-23/xen/common/xmalloc_tlsf.c	2012-05-21 15:38:31.000000000 +0200
@@ -604,11 +604,11 @@ void xfree(void *p)
 {
     struct bhdr *b;
 
-    ASSERT(!in_irq());
-
     if ( p == NULL )
         return;
 
+    ASSERT(!in_irq());
+
     /* Strip alignment padding. */
     b = (struct bhdr *)((char *) p - BHDR_OVERHEAD);
     if ( b->size & 1 )

[-- 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] x86: prevent call to xfree() in dump_irqs() while in an irq context

[Andrew Cooper]

On 21/05/12 14:50, Jan Beulich wrote:
> Because of c/s 24707:96987c324a4f, dump_irqs() can now be called in an
> irq context when a bug condition is encountered. If this is the case,
> ignore the call to xsm_show_irq_ssid() and the subsequent call to
> xfree().
>
> This prevents an assertion failure in xfree(), and should allow all the
> debug information to be dumped, before failing with a BUG() because of
> the underlying race condition we are attempting to reproduce.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Rather than using the non-obvious conditional around an xfree() that
> would be passed NULL only in the inverse case (which could easily get
> removed by a future change on the basis that calling xfree(NULL) is
> benign), switch the order of checks in xfree() itself and only suppress
> the call to XSM that could potentially call xmalloc().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

>
> --- 2012-04-23.orig/xen/arch/x86/irq.c	2012-05-14 17:43:58.000000000 +0200
> +++ 2012-04-23/xen/arch/x86/irq.c	2012-05-21 15:38:01.000000000 +0200
> @@ -2060,7 +2060,7 @@ static void dump_irqs(unsigned char key)
>          if ( !irq_desc_initialized(desc) || desc->handler == &no_irq_type )
>              continue;
>  
> -        ssid = xsm_show_irq_sid(irq);
> +        ssid = in_irq() ? NULL : xsm_show_irq_sid(irq);
>  
>          spin_lock_irqsave(&desc->lock, flags);
>  
> --- 2012-04-23.orig/xen/common/xmalloc_tlsf.c	2011-10-17 08:35:00.000000000 +0200
> +++ 2012-04-23/xen/common/xmalloc_tlsf.c	2012-05-21 15:38:31.000000000 +0200
> @@ -604,11 +604,11 @@ void xfree(void *p)
>  {
>      struct bhdr *b;
>  
> -    ASSERT(!in_irq());
> -
>      if ( p == NULL )
>          return;
>  
> +    ASSERT(!in_irq());
> +
>      /* Strip alignment padding. */
>      b = (struct bhdr *)((char *) p - BHDR_OVERHEAD);
>      if ( b->size & 1 )
>
>
>

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com

Re: [PATCH] x86: prevent call to xfree() in dump_irqs() while in an irq context

[Keir Fraser]

On 21/05/2012 14:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> On 21/05/12 14:50, Jan Beulich wrote:
>> Because of c/s 24707:96987c324a4f, dump_irqs() can now be called in an
>> irq context when a bug condition is encountered. If this is the case,
>> ignore the call to xsm_show_irq_ssid() and the subsequent call to
>> xfree().
>> 
>> This prevents an assertion failure in xfree(), and should allow all the
>> debug information to be dumped, before failing with a BUG() because of
>> the underlying race condition we are attempting to reproduce.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> 
>> Rather than using the non-obvious conditional around an xfree() that
>> would be passed NULL only in the inverse case (which could easily get
>> removed by a future change on the basis that calling xfree(NULL) is
>> benign), switch the order of checks in xfree() itself and only suppress
>> the call to XSM that could potentially call xmalloc().
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'm a bit dubious about having a function that can be called in irq context
for some input values but not others. I suppose this trivial case for
xfree() is obvious enough, so I'm okay with it. If it was anything more
subtle, I would probably nack.

 -- Keir

>> --- 2012-04-23.orig/xen/arch/x86/irq.c 2012-05-14 17:43:58.000000000 +0200
>> +++ 2012-04-23/xen/arch/x86/irq.c 2012-05-21 15:38:01.000000000 +0200
>> @@ -2060,7 +2060,7 @@ static void dump_irqs(unsigned char key)
>>          if ( !irq_desc_initialized(desc) || desc->handler == &no_irq_type )
>>              continue;
>>  
>> -        ssid = xsm_show_irq_sid(irq);
>> +        ssid = in_irq() ? NULL : xsm_show_irq_sid(irq);
>>  
>>          spin_lock_irqsave(&desc->lock, flags);
>>  
>> --- 2012-04-23.orig/xen/common/xmalloc_tlsf.c 2011-10-17 08:35:00.000000000
>> +0200
>> +++ 2012-04-23/xen/common/xmalloc_tlsf.c 2012-05-21 15:38:31.000000000 +0200
>> @@ -604,11 +604,11 @@ void xfree(void *p)
>>  {
>>      struct bhdr *b;
>>  
>> -    ASSERT(!in_irq());
>> -
>>      if ( p == NULL )
>>          return;
>>  
>> +    ASSERT(!in_irq());
>> +
>>      /* Strip alignment padding. */
>>      b = (struct bhdr *)((char *) p - BHDR_OVERHEAD);
>>      if ( b->size & 1 )
>> 
>> 
>> 

Re: [PATCH] x86: prevent call to xfree() in dump_irqs() while in an irq context

[Jan Beulich]

>>> On 21.05.12 at 17:06, Keir Fraser <keir.xen@gmail.com> wrote:
> On 21/05/2012 14:59, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
> 
>> On 21/05/12 14:50, Jan Beulich wrote:
>>> Because of c/s 24707:96987c324a4f, dump_irqs() can now be called in an
>>> irq context when a bug condition is encountered. If this is the case,
>>> ignore the call to xsm_show_irq_ssid() and the subsequent call to
>>> xfree().
>>> 
>>> This prevents an assertion failure in xfree(), and should allow all the
>>> debug information to be dumped, before failing with a BUG() because of
>>> the underlying race condition we are attempting to reproduce.
>>> 
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> 
>>> Rather than using the non-obvious conditional around an xfree() that
>>> would be passed NULL only in the inverse case (which could easily get
>>> removed by a future change on the basis that calling xfree(NULL) is
>>> benign), switch the order of checks in xfree() itself and only suppress
>>> the call to XSM that could potentially call xmalloc().
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I'm a bit dubious about having a function that can be called in irq context
> for some input values but not others. I suppose this trivial case for
> xfree() is obvious enough, so I'm okay with it. If it was anything more
> subtle, I would probably nack.

I did ask that in the original thread, but you never responded
either way. Is your above reply an ack then, or should I commit
Andy's original patch instead?

Jan

Re: [PATCH] x86: prevent call to xfree() in dump_irqs() while in an irq context

[Keir Fraser]

On 22/05/2012 09:05, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> Rather than using the non-obvious conditional around an xfree() that
>>>> would be passed NULL only in the inverse case (which could easily get
>>>> removed by a future change on the basis that calling xfree(NULL) is
>>>> benign), switch the order of checks in xfree() itself and only suppress
>>>> the call to XSM that could potentially call xmalloc().
>>>> 
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> 
>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> 
>> I'm a bit dubious about having a function that can be called in irq context
>> for some input values but not others. I suppose this trivial case for
>> xfree() is obvious enough, so I'm okay with it. If it was anything more
>> subtle, I would probably nack.
> 
> I did ask that in the original thread, but you never responded
> either way. Is your above reply an ack then, or should I commit
> Andy's original patch instead?

It's an Ack :)