| 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
| ||
|
Date: Fri, 28 Jun 2013 10:33:04 +0800
From: Zhenzhong Duan <zhenzhong.duan@...cle.com>
To: Stefano Stabellini <stefano.stabellini@...citrix.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
David Vrabel <david.vrabel@...rix.com>,
"xen-devel@...ts.xensource.com" <xen-devel@...ts.xensource.com>,
Feng Jin <joe.jin@...cle.com>,
Yuval Shaia <yuval.shaia@...cle.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Chien Yen <chien.yen@...cle.com>
Subject: Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driver
load first time
On 2013-06-27 19:52, Stefano Stabellini wrote:
> On Thu, 27 Jun 2013, Zhenzhong Duan wrote:
>> On 2013-06-27 02:08, Stefano Stabellini wrote:
>>> On Wed, 26 Jun 2013, Zhenzhong Duan wrote:
>>>> On 2013-06-26 01:51, Stefano Stabellini wrote:
>>>>> On Tue, 25 Jun 2013, DuanZhenzhong wrote:
>>>>>> Stefano Stabellini wrote:
>>>>>>> Trimming some of the people in CC
>>>>>>>
>>>>>>> On Mon, 24 Jun 2013, Zhenzhong Duan wrote:
>>>>>>>
>>>>>>>> On 2013-06-20 22:21, Stefano Stabellini wrote:
>>>>>>>>
>>>>>>>>> On Thu, 20 Jun 2013, Zhenzhong Duan wrote:
>>>>>>>>>
>>>>>>>>>> On 2013-06-05 20:50, Stefano Stabellini wrote:
>>>>>>>>>>
>>>>>>>>>>> On Wed, 5 Jun 2013, Zhenzhong Duan wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Stefano Stabellini wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, 21 May 2013, Stefano Stabellini wrote:
>>>>>>>>>>>> On Tue, 21 May 2013, Konrad Rzeszutek Wilk wrote:
>>>>>>>>>>>> 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 think that you are looking in the wrong place.
>>>>>>>>>>>> The issue is that QEMU doesn't call pt_msi_disable in
>>>>>>>>>>>> pt_msgctrl_reg_write if (!val & PCI_MSI_FLAGS_ENABLE).
>>>>>>>>>>>>
>>>>>>>>>>>> The code above is correct as is because it is trying to
>>>>>>>>>>>> handle
>>>>>>>>>>>> emulated
>>>>>>>>>>>> IRQs and MSIs, not real passthrough MSIs. They latter are
>>>>>>>>>>>> not
>>>>>>>>>>>> added to
>>>>>>>>>>>> that radix tree, see physdev_hvm_map_pirq and
>>>>>>>>>>>> physdev_map_pirq.
>>>>>>>>>>>> This patch fixes the issue, I have only tested MSI
>>>>>>>>>>>> (MSI-X
>>>>>>>>>>>> completely
>>>>>>>>>>>> untested).
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/hw/pass-through.c b/hw/pass-through.c
>>>>>>>>>>>> index 304c438..079e465 100644
>>>>>>>>>>>> --- a/hw/pass-through.c
>>>>>>>>>>>> +++ b/hw/pass-through.c
>>>>>>>>>>>> @@ -3866,7 +3866,11 @@ static int
>>>>>>>>>>>> pt_msgctrl_reg_write(struct
>>>>>>>>>>>> pt_dev
>>>>>>>>>>>> *ptdev,
>>>>>>>>>>>> ptdev->msi->flags |= PCI_MSI_FLAGS_ENABLE;
>>>>>>>>>>>> }
>>>>>>>>>>>> else
>>>>>>>>>>>> - ptdev->msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
>>>>>>>>>>>> + {
>>>>>>>>>>>> + if (ptdev->msi->flags & PT_MSI_MAPPED) {
>>>>>>>>>>>> + pt_msi_disable(ptdev);
>>>>>>>>>>>> + }
>>>>>>>>>>>> + }
>>>>>>>>>>>> /* pass through MSI_ENABLE bit when no MSI-INTx
>>>>>>>>>>>> translation
>>>>>>>>>>>> */
>>>>>>>>>>>> if (!ptdev->msi_trans_en) {
>>>>>>>>>>>> @@ -4013,6 +4017,8 @@ static int
>>>>>>>>>>>> pt_msixctrl_reg_write(struct
>>>>>>>>>>>> pt_dev
>>>>>>>>>>>> *ptdev,
>>>>>>>>>>>> pt_disable_msi_translate(ptdev);
>>>>>>>>>>>> }
>>>>>>>>>>>> pt_msix_update(ptdev);
>>>>>>>>>>>> + } else if (!(*value & PCI_MSIX_ENABLE) &&
>>>>>>>>>>>> ptdev->msix->enabled) {
>>>>>>>>>>>> + pt_msix_delete(ptdev);
>>>>>>>>>>>> Hi Stefano,
>>>>>>>>>>>> I made a test with this patch, os reboot when driver
>>>>>>>>>>>> reload.
>>>>>>>>>>>> If
>>>>>>>>>>>> use
>>>>>>>>>>>> pt_msix_disable
>>>>>>>>>>>> instead of pt_msix_delete, driver could be reloaded.
>>>>>>>>>>>> But I still see some error in qemu.log and xen console.
>>>>>>>>>>>> Seems
>>>>>>>>>>>> four
>>>>>>>>>>>> IRQs
>>>>>>>>>>>> are not freed
>>>>>>>>>>>> when unmap.
>>>>>>>>>>>> --------------first load---------------------------
>>>>>>>>>>>> pt_msix_update_one: pt_msix_update_one requested pirq =
>>>>>>>>>>>> 103
>>>>>>>>>>>> pt_msix_update_one: Update msix entry 0 with pirq 67 gvec
>>>>>>>>>>>> 0
>>>>>>>>>>>> pt_msix_update_one: pt_msix_update_one requested pirq =
>>>>>>>>>>>> 102
>>>>>>>>>>>> pt_msix_update_one: Update msix entry 1 with pirq 66 gvec
>>>>>>>>>>>> 0
>>>>>>>>>>>> pt_msix_update_one: pt_msix_update_one requested pirq =
>>>>>>>>>>>> 101
>>>>>>>>>>>> pt_msix_update_one: Update msix entry 2 with pirq 65 gvec
>>>>>>>>>>>> 0
>>>>>>>>>>>> pt_msix_update_one: pt_msix_update_one requested pirq =
>>>>>>>>>>>> 100
>>>>>>>>>>>> pt_msix_update_one: Update msix entry 3 with pirq 64 gvec
>>>>>>>>>>>> 0
>>>>>>>>>>>> ------------- first unload---------------------------
>>>>>>>>>>>> pt_msix_disable: Unbind msix with pirq 67, gvec 0
>>>>>>>>>>>> pt_msix_disable: Unmap msix with pirq 67
>>>>>>>>>>>> pt_msix_disable: Error: Unmapping of MSI-X failed.
>>>>>>>>>>>> [00:04.0]
>>>>>>>>>>>> pt_msix_disable: Unbind msix with pirq 66, gvec 0
>>>>>>>>>>>> pt_msix_disable: Unmap msix with pirq 66
>>>>>>>>>>>> pt_msix_disable: Error: Unmapping of MSI-X failed.
>>>>>>>>>>>> [00:04.0]
>>>>>>>>>>>> pt_msix_disable: Unbind msix with pirq 65, gvec 0
>>>>>>>>>>>> pt_msix_disable: Unmap msix with pirq 65
>>>>>>>>>>>> pt_msix_disable: Error: Unmapping of MSI-X failed.
>>>>>>>>>>>> [00:04.0]
>>>>>>>>>>>> pt_msix_disable: Unbind msix with pirq 64, gvec 0
>>>>>>>>>>>> pt_msix_disable: Unmap msix with pirq 64
>>>>>>>>>>>> pt_msix_disable: Error: Unmapping of MSI-X failed.
>>>>>>>>>>>> [00:04.0]
>>>>>>>>>>>>
>>>>>>>>>>> Can you add some printks in Xen (the hypercall name is
>>>>>>>>>>> PHYSDEVOP_unmap_pirq) to figure out exactly why they are
>>>>>>>>>>> failing?
>>>>>>>>>>>
>>>>>>>>>> Did some test, domain_pirq_to_emuirq(d, unmap->pirq) =
>>>>>>>>>> IRQ_UNBOUND
>>>>>>>>>> in
>>>>>>>>>> physdev_unmap_pirq.
>>>>>>>>>>
>>>>>>>>> That means that Linux didn't call irq_enable on the MSI-X in
>>>>>>>>> question:
>>>>>>>>>
>>>>>>>>> irq_enable -> __startup_pirq -> EVTCHNOP_bind_pirq
>>>>>>>>>
>>>>>>>>> EVTCHNOP_bind_pirq is implemented by evtchn_bind_pirq in Xen and
>>>>>>>>> calls
>>>>>>>>> map_domain_emuirq_pirq, so domain_pirq_to_emuirq(d, unmap->pirq)
>>>>>>>>> should
>>>>>>>>> be IRQ_PT.
>>>>>>>>>
>>>>>>>>> I don't know if that's a normal condition, but in any case it
>>>>>>>>> should
>>>>>>>>> not create any problems to physdev_unmap_pirq, in fact the
>>>>>>>>> folloing
>>>>>>>>> check:
>>>>>>>>>
>>>>>>>>> if ( domid == DOMID_SELF || ret )
>>>>>>>>> goto free_domain;
>>>>>>>>>
>>>>>>>>> should fail so Xen should continue and execute
>>>>>>>>> unmap_domain_pirq.
>>>>>>>>> That's
>>>>>>>>> what we want.
>>>>>>>>>
>>>>>>>> From linux side, request_irq-> request_threaded_irq->
>>>>>>>> __setup_irq->
>>>>>>>> irq_startup-> startup_pirq-> EVTCHNOP_bind_pirq
>>>>>>>> If irq_enable isn't called, how does the driver receive interrupt,
>>>>>>>> I
>>>>>>>> did
>>>>>>>> see
>>>>>>>> four interrupts in /proc/interrupt and driver works ok.
>>>>>>>>
>>>>>>> Good to know
>>>>>>>
>>>>>>>
>>>>>>>> Could you have a look if there is something wrong in xen side of
>>>>>>>> clearing
>>>>>>>> the
>>>>>>>> mapping?
>>>>>>>>
>>>>>>> What I am saying is that the error you are getting:
>>>>>>>
>>>>>>> pt_msix_disable: Unbind msix with pirq 67, gvec 0
>>>>>>> pt_msix_disable: Unmap msix with pirq 67
>>>>>>> pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
>>>>>>>
>>>>>>> cannot be caused by domain_pirq_to_emuirq(d, pirq) returning
>>>>>>> IRQ_UNBOUND.
>>>>>>> So, why are you getting this error? What is failing?
>>>>>>> I am ready to believe the problem is in Xen but Without
>>>>>>> understanding
>>>>>>> why you are getting the error it's hard to find a solution.
>>>>>>>
>>>>>> I found the reason, you are looking at xen-unstable, I was working
>>>>>> with
>>>>>> 4.1.30-OVM, it has patch of CVE-2012-4536 / XSA-21.
>>>>>> That patch set ret to -EINVAL initially. After remove that line, unmap
>>>>>> succeed.
>>>>>> But we still need below patch to let driver reload succeed everytime.
>>>>>> Without
>>>>>> that, 1st reload failed, 2nd succeed, 3 failed, ...
>>>>>>
>>>>>> diff -up --new-file ./hw/pt-msi.c.old1 ./hw/pt-msi.c
>>>>>> --- ./hw/pt-msi.c.old1 2013-06-26 01:36:08.000000000 +0800
>>>>>> +++ ./hw/pt-msi.c 2013-06-26 01:37:41.000000000 +0800
>>>>>> @@ -469,7 +469,7 @@ static void pci_msix_writel(void *opaque
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> - if ( offset != 3 && entry->io_mem[offset] != val )
>>>>>> + if ( offset != 3 && (entry->io_mem[offset] != val || entry->pirq
>>>>>> ==
>>>>>> -1))
>>>>>> entry->flags = 1;
>>>>>> entry->io_mem[offset] = val;
>>>>> Interesting. I don't think this is the proper fix though.
>>>>> Does the appended patch change anything?
>>>>>
>>>>> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
>>>>> index 71fa6f0..cd5d9c7 100644
>>>>> --- a/hw/pt-msi.c
>>>>> +++ b/hw/pt-msi.c
>>>>> @@ -302,7 +302,7 @@ static int pt_msix_update_one(struct pt_dev *dev,
>>>>> int
>>>>> entry_nr)
>>>>> uint32_t gflags = __get_msi_gflags(entry->io_mem[2], gaddr);
>>>>> int ret;
>>>>> - if ( !entry->flags )
>>>>> + if ( !entry->flags && ptdev->msix->enabled )
>>>>> return 0;
>>>>> if (!gvec) {
>>>> Tested, not work.
>>>> If you look at msix_capability_init in kernel, line 707,722,
>>>> dev->msix->enabled is already set when pt_msix_update is called.
>>> Yeah, but it shouldn't be already set in QEMU. In fact in QEMU
>>> dev->msix->enabled is modified in pt_msixctrl_reg_write after calling to
>>> pt_msix_update.
>> It does.
>> line 707, PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE is set
>> this will set dev->msix->enabled first time
>> line 722, PCI_MSIX_FLAGS_ENABLE set
>> this trigger call of pt_msix_update
>> then dev->msix->enabled was set a second time
>>> I was assuming that you needed to add "|| entry->pirq == -1" because you
>>> needed to pass the check:
>>>
>>> if ( !entry->flags )
>>> return 0;
>>>
>>> at the beginning of pt_msix_update_one. Am I getting it right?
>> Right, as entry->pirq is set to -1 when driver unload.
>>> If that is case that I thought that we just needed to make sure that
>>> when ptdev->msix->enabled is still zero then we go through the test in
>>> pt_msix_update_one. Where is the mistake?
>> If you want to use dev->msix->enabled for checking, below patch could work.
>> But in this case, dev->msix->enabled doesn't represent PCI_MSIX_ENABLE any
>> more,
>> but a combination of PCI_MSIX_ENABLE and ~PCI_MSIX_MASK
>>
>> zduan
>> ***********************************************************
>> diff -up ./hw/pass-through.c.old2 ./hw/pass-through.c
>> --- ./hw/pass-through.c.old2 2013-06-27 11:05:30.000000000 +0800
>> +++ ./hw/pass-through.c 2013-06-27 11:07:35.000000000 +0800
>> @@ -4027,7 +4027,7 @@ static int pt_msixctrl_reg_write(struct
>> pt_msix_disable(ptdev);
>> }
>>
>> - ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE);
>> + ptdev->msix->enabled = (*value & PCI_MSIX_ENABLE) && !(*value &
>> PCI_MSIX_MASK);
>>
>> return 0;
>> }
>> diff -up ./hw/pt-msi.c.old2 ./hw/pt-msi.c
>> --- ./hw/pt-msi.c.old2 2013-06-27 11:26:12.000000000 +0800
>> +++ ./hw/pt-msi.c 2013-06-27 11:27:13.000000000 +0800
>> @@ -294,7 +294,7 @@ static int pt_msix_update_one(struct pt_
>> uint32_t gflags = __get_msi_gflags(entry->io_mem[2], gaddr);
>> int ret;
>>
>> - if ( !entry->flags )
>> + if ( !entry->flags && dev->msix->enabled )
>> return 0;
>>
>> if (!gvec) {
> I understand now, thanks for the explanation.
> Among the two alternatives, I think that your first change is actually
> better.
>
> However we can probably still improve it a little bit by setting
> entry->flags to 1 directly in pt_msix_disable? So that we don't confuse
> the driver reload case from the first driver initialization.
>
> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> index 71fa6f0..cc4e280 100644
> --- a/hw/pt-msi.c
> +++ b/hw/pt-msi.c
> @@ -408,7 +408,7 @@ void pt_msix_disable(struct pt_dev *dev)
> }
> /* clear msi-x info */
> entry->pirq = -1;
> - entry->flags = 0;
> + entry->flags = 1;
> }
> }
Test passed.
But this change set entry->flags in all entrys, dev->msix->total_entries
count of pirqs are
mapped when driver reload, no matter how many msix entrys driver is
initializing.
zduan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists