Re: [patch 21/33] genirq/msi: Provide msi_domain_alloc_irq_at()

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()
Date: Fri, 18 Nov 2022 10:15:33 +0100	[thread overview]
Message-ID: <87zgcok4i2.ffs@tglx> (raw)
In-Reply-To: <87k03tkrii.ffs@tglx>

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