2011-04-21 12:33:09

by Jan Kiszka

[permalink] [raw]
Subject: [PATCH v2] intel-iommu: Fix use after release during device attach

On 2011-01-04 11:42, Jan Kiszka wrote:
> Am 10.12.2010 19:44, Chris Wright wrote:
>> * Jan Kiszka ([email protected]) wrote:
>>>>>> --- a/drivers/pci/intel-iommu.c
>>>>>> +++ b/drivers/pci/intel-iommu.c
>>>>>> @@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(struct
>>>>>> iommu_domain *domain,
>>>>>>
>>>>>> pte = dmar_domain->pgd;
>>>>>> if (dma_pte_present(pte)) {
>>>>>> - free_pgtable_page(dmar_domain->pgd);
>>>>>> dmar_domain->pgd = (struct dma_pte *)
>>>>>> phys_to_virt(dma_pte_addr(pte));
>>
>> While here, might as well remove the unnecessary cast.
>>
>>>>>> + free_pgtable_page(pte);
>>>>>> }
>>>>>> dmar_domain->agaw--;
>>>>>> }
>>>>>
>>>>> Reviewed-by: Sheng Yang <[email protected]>
>>
>> Acked-by: Chris Wright <[email protected]>
>>
>>>>> CC iommu mailing list and David.
>>>>
>>>> Ping...
>>>>
>>>> I think this fix also qualifies for stable (.35 and .36).
>>>>
>>>
>>> Still not merged?
>>
>> David, do you plan to pick this one up?
>>
>> thanks,
>> -chris
>
> Hmm, still no reaction. Trying David's Intel address now...
>
> Jan
>

Walking through my old queues, I came across this one again.

Given the still lacking reaction from the official maintainer, I'm a
bit confused about the state of intel-iommu. Is it unmaintained? Should
this bug fix better be routed through the KVM tree as its only in-tree
user? Please enlighten me.

Note that the patch became stable material for 35..38 in the meantime,
and it should go into 39 before release as well.

Thanks,
Jan

-------8<--------

Obtain the new pgd pointer before releasing the page containing this
value. Remove unneeded cast at this chance as well.

Signed-off-by: Jan Kiszka <[email protected]>
---
drivers/pci/intel-iommu.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)

v1->v2: Clean up cast as suggested by Chris.

diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 505c1c7..b3e5c43 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -3607,9 +3607,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,

pte = dmar_domain->pgd;
if (dma_pte_present(pte)) {
- free_pgtable_page(dmar_domain->pgd);
- dmar_domain->pgd = (struct dma_pte *)
- phys_to_virt(dma_pte_addr(pte));
+ dmar_domain->pgd = phys_to_virt(dma_pte_addr(pte));
+ free_pgtable_page(pte);
}
dmar_domain->agaw--;
}
--
1.7.1


2011-04-21 14:03:18

by Chris Wright

[permalink] [raw]
Subject: Re: [PATCH v2] intel-iommu: Fix use after release during device attach

* Jan Kiszka ([email protected]) wrote:
> On 2011-01-04 11:42, Jan Kiszka wrote:
> > Am 10.12.2010 19:44, Chris Wright wrote:
> >> * Jan Kiszka ([email protected]) wrote:
> >>>>>> --- a/drivers/pci/intel-iommu.c
> >>>>>> +++ b/drivers/pci/intel-iommu.c
> >>>>>> @@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(struct
> >>>>>> iommu_domain *domain,
> >>>>>>
> >>>>>> pte = dmar_domain->pgd;
> >>>>>> if (dma_pte_present(pte)) {
> >>>>>> - free_pgtable_page(dmar_domain->pgd);
> >>>>>> dmar_domain->pgd = (struct dma_pte *)
> >>>>>> phys_to_virt(dma_pte_addr(pte));
> >>
> >> While here, might as well remove the unnecessary cast.
> >>
> >>>>>> + free_pgtable_page(pte);
> >>>>>> }
> >>>>>> dmar_domain->agaw--;
> >>>>>> }
> >>>>>
> >>>>> Reviewed-by: Sheng Yang <[email protected]>
> >>
> >> Acked-by: Chris Wright <[email protected]>
> >>
> >>>>> CC iommu mailing list and David.
> >>>>
> >>>> Ping...
> >>>>
> >>>> I think this fix also qualifies for stable (.35 and .36).
> >>>>
> >>>
> >>> Still not merged?
> >>
> >> David, do you plan to pick this one up?
> >>
> >> thanks,
> >> -chris
> >
> > Hmm, still no reaction. Trying David's Intel address now...
> >
> > Jan
> >
>
> Walking through my old queues, I came across this one again.
>
> Given the still lacking reaction from the official maintainer, I'm a
> bit confused about the state of intel-iommu. Is it unmaintained? Should
> this bug fix better be routed through the KVM tree as its only in-tree
> user? Please enlighten me.
>
> Note that the patch became stable material for 35..38 in the meantime,
> and it should go into 39 before release as well.
>
> Thanks,
> Jan
>
> -------8<--------
>
> Obtain the new pgd pointer before releasing the page containing this
> value. Remove unneeded cast at this chance as well.
>
> Signed-off-by: Jan Kiszka <[email protected]>

Acked-by: Chris Wright <[email protected]>

> ---
> drivers/pci/intel-iommu.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> v1->v2: Clean up cast as suggested by Chris.
>
> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> index 505c1c7..b3e5c43 100644
> --- a/drivers/pci/intel-iommu.c
> +++ b/drivers/pci/intel-iommu.c
> @@ -3607,9 +3607,8 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
>
> pte = dmar_domain->pgd;
> if (dma_pte_present(pte)) {
> - free_pgtable_page(dmar_domain->pgd);
> - dmar_domain->pgd = (struct dma_pte *)
> - phys_to_virt(dma_pte_addr(pte));
> + dmar_domain->pgd = phys_to_virt(dma_pte_addr(pte));
> + free_pgtable_page(pte);
> }
> dmar_domain->agaw--;
> }

2011-04-21 14:33:33

by Alex Williamson

[permalink] [raw]
Subject: Re: [PATCH v2] intel-iommu: Fix use after release during device attach

On Thu, 2011-04-21 at 14:32 +0200, Jan Kiszka wrote:
> On 2011-01-04 11:42, Jan Kiszka wrote:
> > Am 10.12.2010 19:44, Chris Wright wrote:
> >> * Jan Kiszka ([email protected]) wrote:
> >>>>>> --- a/drivers/pci/intel-iommu.c
> >>>>>> +++ b/drivers/pci/intel-iommu.c
> >>>>>> @@ -3627,9 +3627,9 @@ static int intel_iommu_attach_device(struct
> >>>>>> iommu_domain *domain,
> >>>>>>
> >>>>>> pte = dmar_domain->pgd;
> >>>>>> if (dma_pte_present(pte)) {
> >>>>>> - free_pgtable_page(dmar_domain->pgd);
> >>>>>> dmar_domain->pgd = (struct dma_pte *)
> >>>>>> phys_to_virt(dma_pte_addr(pte));
> >>
> >> While here, might as well remove the unnecessary cast.
> >>
> >>>>>> + free_pgtable_page(pte);
> >>>>>> }
> >>>>>> dmar_domain->agaw--;
> >>>>>> }
> >>>>>
> >>>>> Reviewed-by: Sheng Yang <[email protected]>
> >>
> >> Acked-by: Chris Wright <[email protected]>
> >>
> >>>>> CC iommu mailing list and David.
> >>>>
> >>>> Ping...
> >>>>
> >>>> I think this fix also qualifies for stable (.35 and .36).
> >>>>
> >>>
> >>> Still not merged?
> >>
> >> David, do you plan to pick this one up?
> >>
> >> thanks,
> >> -chris
> >
> > Hmm, still no reaction. Trying David's Intel address now...
> >
> > Jan
> >
>
> Walking through my old queues, I came across this one again.
>
> Given the still lacking reaction from the official maintainer, I'm a
> bit confused about the state of intel-iommu. Is it unmaintained? Should
> this bug fix better be routed through the KVM tree as its only in-tree
> user? Please enlighten me.

I've been wondering the exact same thing. My last patch took weeks of
prodding, finally went into the maintainer's tree without
acknowledgment, and there's hardly been any activity there to suggest a
pull request for 2.6.39 is going to happen. David, are you still
interested in maintaining this code? Thanks,

Alex

2011-04-21 15:43:08

by David Woodhouse

[permalink] [raw]
Subject: Re: [PATCH v2] intel-iommu: Fix use after release during device attach

On Thu, 2011-04-21 at 15:28 +0100, Alex Williamson wrote:
> I've been wondering the exact same thing. My last patch took weeks of
> prodding, finally went into the maintainer's tree without
> acknowledgment, and there's hardly been any activity there to suggest
> a pull request for 2.6.39 is going to happen. David, are you still
> interested in maintaining this code? Thanks,

Yes, sorry, I've been somewhat snowed under with various things.

This patch has been in my tree for a while, and I've just merged one
more patch which is outstanding and sent Linus a pull request.

--
dwmw2

2011-04-21 16:15:11

by Alex Williamson

[permalink] [raw]
Subject: Re: [PATCH v2] intel-iommu: Fix use after release during device attach

On Thu, 2011-04-21 at 16:42 +0100, David Woodhouse wrote:
> On Thu, 2011-04-21 at 15:28 +0100, Alex Williamson wrote:
> > I've been wondering the exact same thing. My last patch took weeks of
> > prodding, finally went into the maintainer's tree without
> > acknowledgment, and there's hardly been any activity there to suggest
> > a pull request for 2.6.39 is going to happen. David, are you still
> > interested in maintaining this code? Thanks,
>
> Yes, sorry, I've been somewhat snowed under with various things.
>
> This patch has been in my tree for a while, and I've just merged one
> more patch which is outstanding and sent Linus a pull request.

Thanks David, I know you've been busy lately.

Alex