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

[Keir Fraser <keir.xen@gmail.com>]

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