Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Reinette Chatre <reinette.chatre@intel.com>,
        LKML <linux-kernel@vger.kernel.org>
Cc:     x86@kernel.org, Joerg Roedel <joro@8bytes.org>,
        Will Deacon <will@kernel.org>, linux-pci@vger.kernel.org,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Marc Zyngier <maz@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jason Gunthorpe <jgg@mellanox.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        Kevin Tian <kevin.tian@intel.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Logan Gunthorpe <logang@deltatee.com>,
        Ashok Raj <ashok.raj@intel.com>, Jon Mason <jdmason@kudzu.us>,
        Allen Hubbe <allenbh@gmail.com>,
        "Ahmed S. Darwish" <darwi@linutronix.de>
Subject: Re: [patch 21/33] genirq/msi: Provide msi_domain_alloc_irq_at()
In-Reply-To: <87k03tkrii.ffs@tglx>
References: <20221111133158.196269823@linutronix.de>
 <20221111135206.463650635@linutronix.de>
 <0cbf645b-b23a-6c85-4389-bb039a677a52@intel.com> <87k03tkrii.ffs@tglx>
Date:   Fri, 18 Nov 2022 10:15:33 +0100
Message-ID: <87zgcok4i2.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Fri, Nov 18 2022 at 01:58, Thomas Gleixner wrote:
> On Thu, Nov 17 2022 at 15:33, Reinette Chatre wrote:
>> When calling pci_ims_alloc_irq(), msi_insert_desc() ends up being
>> called twice, first with index = MSI_ANY_INDEX, second with index = 0.
>> (domid = 1 both times)
>
> How so?
>
>>>  	}
>>>  
>>>  	hwsize = msi_domain_get_hwsize(dev, domid);
>>> -	if (index >= hwsize) {
>>> -		ret = -ERANGE;
>>> -		goto fail;
>>> -	}
>>>  
>>> -	desc->msi_index = index;
>>> -	index += baseidx;
>>> -	ret = xa_insert(&md->__store, index, desc, GFP_KERNEL);
>>> -	if (ret)
>>> -		goto fail;
>>> -	return 0;
>>> +	if (index == MSI_ANY_INDEX) {
>>> +		struct xa_limit limit;
>>> +		unsigned int index;
>>> +
>>> +		limit.min = baseidx;
>>> +		limit.max = baseidx + hwsize - 1;
>>>  
>>> +		/* Let the xarray allocate a free index within the limits */
>>> +		ret = xa_alloc(&md->__store, &index, desc, limit, GFP_KERNEL);
>>> +		if (ret)
>>> +			goto fail;
>>> +
>>
>> This path (index == MSI_ANY_INDEX) is followed when msi_insert_desc()
>> is called the first time and the xa_alloc() succeeds at index 65536.
>>
>>> +		desc->msi_index = index;
>>
>> This is problematic with desc->msi_index being a u16, assigning
>> 65536 to it becomes 0.
>
> You are partially right. I need to fix that and make it explicit as it's
> a "works by chance or maybe not" construct right now.
>
> But desc->msi_index is correct to be truncated because it's the index
> within the domain space which is zero based.

It should obviously do:

   desc->msi_index = index - baseidx;

>>> +		return 0;
>>> +	} else {
>>> +		if (index >= hwsize) {
>>> +			ret = -ERANGE;
>>> +			goto fail;
>>> +		}
>>> +
>>> +		desc->msi_index = index;
>>> +		index += baseidx;
>>> +		ret = xa_insert(&md->__store, index, desc, GFP_KERNEL);
>>> +		if (ret)
>>> +			goto fail;
>>
>> This "else" path is followed when msi_insert_desc() is called the second
>> time with "index = 0". The xa_insert() above fails at index 65536
>> (baseidx = 65536) with -EBUSY, trickling up as the return code to
>> pci_ims_alloc_irq().
>
> Why is it called with index=0 the second time?
>>> +	desc = msi_alloc_desc(dev, 1, affdesc);
>>> +	if (!desc) {
>>> +		map.index = -ENOMEM;
>>> +		goto unlock;
>>> +	}
>>> +
>>> +	if (cookie)
>>> +		desc->data.cookie = *cookie;
>>> +
>>> +	ret = msi_insert_desc(dev, desc, domid, index);
>>> +	if (ret) {
>>> +		map.index = ret;
>>> +		goto unlock;
>>> +	}
>>
>> Above is the first call to msi_insert_desc(/* index = MSI_ANY_INDEX */)
>>
>>> +
>>> +	map.index = desc->msi_index;
>>
>> msi_insert_desc() did attempt to set desc->msi_index to 65536 but map.index ends
>> up being 0.
>
> which is kinda correct.
>
>>> +	ret = msi_domain_alloc_irqs_range_locked(dev, domid, map.index, map.index);
>>
>> Here is where the second call to msi_insert_desc() originates:
>>
>> msi_domain_alloc_irqs_range_locked() -> msi_domain_alloc_locked() -> \
>> __msi_domain_alloc_locked() -> msi_domain_alloc_simple_msi_descs() -> \
>> msi_domain_add_simple_msi_descs() -> msi_insert_desc()
>
> but yes, that's bogus because it tries to allocate what is allocated already.
>
> Too tired to decode this circular dependency right now. Will stare at it
> with brain awake in the morning. Duh!

Duh. I'm a moron.

Of course I "tested" this by flipping default and secondary domain
around and doing dynamic allocations from PCI/MSI-X but that won't catch
the bug because PCI/MSI-X does not have the ALLOC_SIMPLE_DESCS flag set.

Let me fix that.

Thanks,

        tglx