* [PATCH 1/2] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
2011-09-13 13:44 [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci' Igor Mammedov
@ 2011-09-13 13:44 ` Igor Mammedov
2011-09-13 13:44 ` Igor Mammedov
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2011-09-13 13:44 UTC (permalink / raw)
To: xen-devel; +Cc: keir.fraser, JBeulich
On a system with Intel C600 series Patsburg SAS controller
if following commands are executed:
rmmod isci
modprobe isci
the host will crash in pirq_guest_bind in attempt to dereference
NULL 'action' pointer.
This is caused by isci driver which does not cleanup irq properly,
removing device first and then os tries to unbind its irqs afterwards.
c/s 20093 and 20844 fixed host crashes when removing isci module.
However in dynamic_irq_cleanup 'action' field of irq_desc is set to
NULL but IRQ_GUEST flag in 'status' field is not cleared. So on next
attempt to bind pirq (modprobe isci) in pirq_guest_bind with IRQ_GUEST
flag set, the branch
if ( !(desc->status & IRQ_GUEST) )
is skipped and host ends up with dereferencing NULL pointer 'action'.
__pirq_guest_unbind is the only place where IRQ_GUEST flag is cleared,
lets keep it this way. Besides it is not safe to clear IRQ_GUEST flag
in dynamic_irq_cleanup, because we can later hit
BUG_ON(!(desc->status & IRQ_GUEST));
in pirq_guest_unbind -> __pirq_guest_unbind
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
diff -r 0312575dc35e -r 2049f7ca3177 xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Thu Sep 08 15:13:06 2011 +0100
+++ b/xen/arch/x86/irq.c Tue Sep 13 14:44:59 2011 +0200
@@ -1472,6 +1472,7 @@ static irq_guest_action_t *__pirq_guest_
if ( unlikely(action == NULL) )
{
+ desc->status &= ~IRQ_GUEST;
dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
d->domain_id, pirq->pirq);
return NULL;
@@ -1599,6 +1600,7 @@ static int pirq_guest_force_unbind(struc
action = (irq_guest_action_t *)desc->action;
if ( unlikely(action == NULL) )
{
+ desc->status &= ~IRQ_GUEST;
dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
d->domain_id, pirq->pirq);
goto out;
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
2011-09-13 13:44 [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci' Igor Mammedov
2011-09-13 13:44 ` [PATCH 1/2] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL Igor Mammedov
@ 2011-09-13 13:44 ` Igor Mammedov
2011-09-13 13:56 ` Igor Mammedov
2011-09-13 13:54 ` [PATCH 2/2] xen: remove duplicate code and keep IRQ_GUEST flag reset at one place Igor Mammedov
2011-09-16 9:13 ` [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci' Igor Mammedov
3 siblings, 1 reply; 6+ messages in thread
From: Igor Mammedov @ 2011-09-13 13:44 UTC (permalink / raw)
To: xen-devel; +Cc: keir.fraser, JBeulich
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
diff -r 2049f7ca3177 -r 884814bfc22e xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Tue Sep 13 14:44:59 2011 +0200
+++ b/xen/arch/x86/irq.c Tue Sep 13 14:47:46 2011 +0200
@@ -1599,12 +1599,7 @@ static int pirq_guest_force_unbind(struc
action = (irq_guest_action_t *)desc->action;
if ( unlikely(action == NULL) )
- {
- desc->status &= ~IRQ_GUEST;
- dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
- d->domain_id, pirq->pirq);
- goto out;
- }
+ goto unbind;
for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ )
continue;
@@ -1612,6 +1607,7 @@ static int pirq_guest_force_unbind(struc
goto out;
bound = 1;
+ unbind:
oldaction = __pirq_guest_unbind(d, pirq, desc);
out:
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
2011-09-13 13:44 ` Igor Mammedov
@ 2011-09-13 13:56 ` Igor Mammedov
0 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2011-09-13 13:56 UTC (permalink / raw)
To: xen-devel; +Cc: keir.fraser, JBeulich
This one has wrong Subject. Just forget about it, please.
I'll send it with correct subj.
On 09/13/2011 03:44 PM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> Reviewed-by: Jan Beulich<JBeulich@suse.com>
>
> diff -r 2049f7ca3177 -r 884814bfc22e xen/arch/x86/irq.c
> --- a/xen/arch/x86/irq.c Tue Sep 13 14:44:59 2011 +0200
> +++ b/xen/arch/x86/irq.c Tue Sep 13 14:47:46 2011 +0200
> @@ -1599,12 +1599,7 @@ static int pirq_guest_force_unbind(struc
>
> action = (irq_guest_action_t *)desc->action;
> if ( unlikely(action == NULL) )
> - {
> - desc->status&= ~IRQ_GUEST;
> - dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
> - d->domain_id, pirq->pirq);
> - goto out;
> - }
> + goto unbind;
>
> for ( i = 0; (i< action->nr_guests)&& (action->guest[i] != d); i++ )
> continue;
> @@ -1612,6 +1607,7 @@ static int pirq_guest_force_unbind(struc
> goto out;
>
> bound = 1;
> + unbind:
> oldaction = __pirq_guest_unbind(d, pirq, desc);
>
> out:
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
--
Thanks,
Igor
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] xen: remove duplicate code and keep IRQ_GUEST flag reset at one place
2011-09-13 13:44 [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci' Igor Mammedov
2011-09-13 13:44 ` [PATCH 1/2] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL Igor Mammedov
2011-09-13 13:44 ` Igor Mammedov
@ 2011-09-13 13:54 ` Igor Mammedov
2011-09-16 9:13 ` [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci' Igor Mammedov
3 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2011-09-13 13:54 UTC (permalink / raw)
To: xen-devel; +Cc: keir.fraser, JBeulich
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
diff -r 2049f7ca3177 -r 884814bfc22e xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c Tue Sep 13 14:44:59 2011 +0200
+++ b/xen/arch/x86/irq.c Tue Sep 13 14:47:46 2011 +0200
@@ -1599,12 +1599,7 @@ static int pirq_guest_force_unbind(struc
action = (irq_guest_action_t *)desc->action;
if ( unlikely(action == NULL) )
- {
- desc->status &= ~IRQ_GUEST;
- dprintk(XENLOG_G_WARNING, "dom%d: pirq %d: desc->action is NULL!\n",
- d->domain_id, pirq->pirq);
- goto out;
- }
+ goto unbind;
for ( i = 0; (i < action->nr_guests) && (action->guest[i] != d); i++ )
continue;
@@ -1612,6 +1607,7 @@ static int pirq_guest_force_unbind(struc
goto out;
bound = 1;
+ unbind:
oldaction = __pirq_guest_unbind(d, pirq, desc);
out:
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci'
2011-09-13 13:44 [PATCH 0/2] Prevent host crash after executing in dom0 'rmmod isci; modprobe isci' Igor Mammedov
` (2 preceding siblings ...)
2011-09-13 13:54 ` [PATCH 2/2] xen: remove duplicate code and keep IRQ_GUEST flag reset at one place Igor Mammedov
@ 2011-09-16 9:13 ` Igor Mammedov
3 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2011-09-16 9:13 UTC (permalink / raw)
To: xen-devel
On 09/13/2011 03:44 PM, Igor Mammedov wrote:
> [PATCH 1/2] xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL
> [PATCH 2/2] xen: remove duplicate code and keep IRQ_GUEST flag reset at one place
>
> Changes since first patch "xen: Clear IRQ_GUEST bit from irq_desc status if its action is NULL"
>
> - split patch in 2, fix (1) and cleanup (2)
> - move 'unbind' label after 'bound = 1;'
>
Self-NACK this one.
I'll post version 3 that might be better.
--
Thanks,
Igor
^ permalink raw reply [flat|nested] 6+ messages in thread