linux-kernel - Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free

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: <alpine.DEB.2.11.1607261022330.19896@nanos>
Date:	Tue, 26 Jul 2016 11:00:21 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Auger Eric <eric.auger@...hat.com>
cc:	eric.auger.pro@...il.com, Marc Zyngier <marc.zyngier@....com>,
	christoffer.dall@...aro.org, andre.przywara@....com,
	robin.murphy@....com, alex.williamson@...hat.com,
	will.deacon@....com, joro@...tes.org,
	Jason Cooper <jason@...edaemon.net>,
	LAK <linux-arm-kernel@...ts.infradead.org>, drjones@...hat.com,
	kvmarm@...ts.cs.columbia.edu, kvm@...r.kernel.org,
	pbonzini@...hat.com, LKML <linux-kernel@...r.kernel.org>,
	Bharat.Bhushan@...escale.com, pranav.sawargaonkar@...il.com,
	p.fedin@...sung.com, iommu@...ts.linux-foundation.org,
	Jean-Philippe.Brucker@....com, yehuday@...vell.com,
	Manish.Jaggi@...iumnetworks.com, robert.richter@...iumnetworks.com,
	Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v11 09/10] genirq/msi: map/unmap the MSI doorbells on
 msi_domain_alloc/free_irqs

B1;2802;0cEric,

On Mon, 25 Jul 2016, Auger Eric wrote:
> On 20/07/2016 11:04, Thomas Gleixner wrote:
> > On Tue, 19 Jul 2016, Eric Auger wrote:
> >> +		if (ret) {
> >> +			for (; i >= 0; i--) {
> >> +				struct irq_data *d = irq_get_irq_data(virq + i);
> >> +
> >> +				msi_handle_doorbell_mappings(d, false);
> >> +			}
> >> +			irq_domain_free_irqs(virq, desc->nvec_used);
> >> +			desc->irq = 0;
> >> +			goto error;
> > 
> > How is that supposed to work? You clear desc->irq and then you call
> > ops->handle_error.
>
> if I don't clear the desc->irq I enter an infinite loop in
> pci_enable_msix_range.
>
> This happens because msix_capability_init and pcie_enable_msix returns 1.
> In msix_capability_init, at out_avail: we enumerate the msi_desc which have
> a non zero irq, hence the returned value equal to 1.
>
> Currently the only handle_error ops I found, pci_msi_domain_handle_error
> does not use irq field so works although questionable.

The logic here is: If the allocation does not succeed for the requested number
of interrupts, we tell the caller how many interrupts we were able to set up.
So the caller can decide what to do.

In your case you don't want to have a partial allocation, so instead of
playing silly games with desc->irq you should add a flag which tells the PCI
code that you are not interested in a partial allocation and that it should
return an error code instead.

Something like PCI_DEV_FLAGS_MSI_NO_PARTIAL_ALLOC should do the trick.

> As for the irq_domain_free_irqs I think I can remove it since handled later.

Not only the free_irqs(). You should let the teardown function handle
everything including your doorbell mapping teardown. It's nothing special and
free_msi_irqs() at the end of msix_capability_init() will take care of it.

Thanks,

	tglx