linux-kernel - RE: [PATCH v7 2/2] PCI: hv: Add arm64 Hyper-V vPCI support

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] [day] [month] [year] [list]

Message-ID: <MWHPR21MB1593B704CAA86AEC071A7D9ED74A9@MWHPR21MB1593.namprd21.prod.outlook.com>
Date:   Tue, 4 Jan 2022 19:50:54 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Sunil Muthuswamy <sunilmut@...rosoft.com>,
        Sunil Muthuswamy <sunilmut@...ux.microsoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "maz@...nel.org" <maz@...nel.org>,
        Dexuan Cui <decui@...rosoft.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "robh@...nel.org" <robh@...nel.org>, "kw@...ux.com" <kw@...ux.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "arnd@...db.de" <arnd@...db.de>
CC:     "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>
Subject: RE: [PATCH v7 2/2] PCI: hv: Add arm64 Hyper-V vPCI support

From: Sunil Muthuswamy <sunilmut@...rosoft.com> Sent: Tuesday, January 4, 2022 11:24 AM
> 
> On Mon, 27 Dec 2021 17:38:07 +0000,
> "Michael Kelley (LINUX)" <mikelley@...rosoft.com> wrote:
> >
> > From: Sunil Muthuswamy <sunilmut@...ux.microsoft.com> Sent: Friday,
> > December 17, 2021 10:52 AM
> > >

[snip]

> > > +
> > > +static int hv_pci_vec_irq_domain_alloc(struct irq_domain *domain,
> > > +				       unsigned int virq, unsigned int nr_irqs,
> > > +				       void *args)
> > > +{
> > > +	irq_hw_number_t hwirq;
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	ret = hv_pci_vec_alloc_device_irq(domain, nr_irqs, &hwirq);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (i = 0; i < nr_irqs; i++) {
> > > +		ret = hv_pci_vec_irq_gic_domain_alloc(domain, virq + i,
> > > +						      hwirq + i);
> > > +		if (ret)
> > > +			goto free_irq;
> > > +
> > > +		ret = irq_domain_set_hwirq_and_chip(domain, virq + i,
> > > +						    hwirq + i,
> > > +						    &hv_arm64_msi_irq_chip,
> > > +						    domain->host_data);
> > > +		if (ret)
> > > +			goto free_irq;
> >
> > This error path doesn't clean up correctly.  While parent IRQs allocated
> > in previous iterations of the loop is cleaned up, the parent IRQ
> > allocated in the current iteration is not.
> >
> 
> 'irq_domain_set_hwirq_and_chip' really shouldn't fail. If you still feel
> that we should address this, I can.
> 

My view is that the code should be logically consistent.  If the
error path should never happen, then we can ignore the return value
and not try to do any cleanup.  But if we are going to treat the error
as possible and have cleanup code, then the cleanup code should be
correct.   It looks like the GIC irqchip drivers follow your approach and
assume irq_domain_set_hwirq_and_chip() never fails, so they don't
check the return value.  I would be OK with that approach here.

Michael