RE: [PATCH 06/12] evtchn: don't bypass unlinking pIRQ when closing port

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Jan 
> Beulich
> Sent: 28 September 2020 11:59
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap 
> <George.Dunlap@xxxxxxxxxxxxx>; Ian
> Jackson <iwj@xxxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>
> Subject: [PATCH 06/12] evtchn: don't bypass unlinking pIRQ when closing port
> 
> There's no other path causing a terminal unlink_pirq_port() to be called
> (evtchn_bind_vcpu() relinks it right away) and hence _if_ pirq can
> indeed be NULL when closing the port, list corruption would occur when
> bypassing the unlink (unless the structure never gets linked again). As
> we can't come here after evtchn_destroy() anymore, (late) domain
> destruction also isn't a reason for a possible exception, and hence the
> only alternative looks to be that the check was pointless in the first
> place. While I haven't observed the case, from code inspection I'm far
> from sure I can exclude this being possible, so it feels more safe to
> re-arrange the code instead.
> 
> Fixes: c24536b636f2 ("replace d->nr_pirqs sized arrays with radix tree")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Paul Durrant <paul@xxxxxxx>

> 
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -615,17 +615,18 @@ int evtchn_close(struct domain *d1, int
>      case ECS_PIRQ: {
>          struct pirq *pirq = pirq_info(d1, chn1->u.pirq.irq);
> 
> -        if ( !pirq )
> -            break;
> -        if ( !is_hvm_domain(d1) )
> -            pirq_guest_unbind(d1, pirq);
> -        pirq->evtchn = 0;
> -        pirq_cleanup_check(pirq, d1);
> -        unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]);
> +        if ( pirq )
> +        {
> +            if ( !is_hvm_domain(d1) )
> +                pirq_guest_unbind(d1, pirq);
> +            pirq->evtchn = 0;
> +            pirq_cleanup_check(pirq, d1);
>  #ifdef CONFIG_X86
> -        if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 )
> -            unmap_domain_pirq_emuirq(d1, pirq->pirq);
> +            if ( is_hvm_domain(d1) && domain_pirq_to_irq(d1, pirq->pirq) > 0 
> )
> +                unmap_domain_pirq_emuirq(d1, pirq->pirq);
>  #endif
> +        }
> +        unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]);
>          break;
>      }
> 
>