linux-kernel - Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driver load first time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130521134059.GE492@phenom.dumpdata.com>
Date:	Tue, 21 May 2013 09:40:59 -0400
From:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To:	David Vrabel <david.vrabel@...rix.com>
Cc:	Stefano Stabellini <stefano.stabellini@...citrix.com>,
	"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
	Feng Jin <joe.jin@...cle.com>,
	Zhenzhong Duan <zhenzhong.duan@...cle.com>,
	Yuval Shaia <yuval.shaia@...cle.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Chien Yen <chien.yen@...cle.com>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when
 driver load first time

> Looking at the hypervisor code I couldn't see anything obviously wrong.

I think the culprit is "physdev_unmap_pirq":

   if ( is_hvm_domain(d) )                                                     
    {                                                                           
        spin_lock(&d->event_lock);                                              
        gdprintk(XENLOG_WARNING,"d%d, pirq: %d is %x %s, irq: %d\n",            
            d->domain_id, pirq, domain_pirq_to_emuirq(d, pirq),                 
            domain_pirq_to_emuirq(d, pirq) == IRQ_UNBOUND ? "unbound" : "",        
            domain_pirq_to_irq(d, pirq));                                       
                                                                                
        if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )                    
            ret = unmap_domain_pirq_emuirq(d, pirq);                            
        spin_unlock(&d->event_lock);                                            
        if ( domid == DOMID_SELF || ret )                                       
            goto free_domain;                                             

It always tells me unbound:

(XEN) physdev.c:237:d14 14, pirq: 54 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 53 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 52 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 51 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 50 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(a bit older debug code, so the 'unbound' does not show up here).

Which means that the call to unmap_domain_pirq_emuirq does not happen.
The checks in unmap_domain_pirq_emuirq also look to be depend
on the code being IRQ_UNBOUND.

In other words, all of that code looks to only clear things when
they are !IRQ_UNBOUND.

But the other logic (IRQ_UNBOUND) looks to be missing a removal
in the radix tree:

  if ( emuirq != IRQ_PT )                                                     
        radix_tree_delete(&d->arch.hvm_domain.emuirq_pirq, emuirq);             
                                                                        
And I think that is what is causing the leak - the radix tree
needs to be pruned? Or perhaps the allocate_pirq should check
the radix tree for IRQ_UNBOUND ones and re-use them?

>  I do note that Xen doesn't free the pirq until it has been unbound by
> the guest.  Xen will warn if the guest unmaps a pirq that is still bound
> ("domD: forcing unbind of pirq P").  Is this what is happening?  If so,

No. It does not b/c of this check in  physdev_unmap_pirq:

if ( domid == DOMID_SELF || ret )
   goto free_domain

which jumps over the call to unmap_domain_pirq.

> that would suggest a bug in the guest rather than the hypervisor.

No. But then I am not even using it. See attached module.

> 
> David

View attachment "alloc_and_unmap.c" of type "text/plain" (1233 bytes)