diff mbox

[v2] intel-iommu: Fix use after release during device attach

Message ID 4DB023C7.5070007@siemens.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Kiszka April 21, 2011, 12:32 p.m. UTC
On 2011-01-04 11:42, Jan Kiszka wrote:
> Am 10.12.2010 19:44, Chris Wright wrote:
>> * Jan Kiszka (jan.kiszka@siemens.com) 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 <sheng@linux.intel.com>
>>
>> Acked-by: Chris Wright <chrisw@sous-sol.org>
>>
>>>>> 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 <jan.kiszka@siemens.com>
---
 drivers/pci/intel-iommu.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

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

Comments

Chris Wright April 21, 2011, 2:02 p.m. UTC | #1
* Jan Kiszka (jan.kiszka@siemens.com) wrote:
> On 2011-01-04 11:42, Jan Kiszka wrote:
> > Am 10.12.2010 19:44, Chris Wright wrote:
> >> * Jan Kiszka (jan.kiszka@siemens.com) 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 <sheng@linux.intel.com>
> >>
> >> Acked-by: Chris Wright <chrisw@sous-sol.org>
> >>
> >>>>> 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 <jan.kiszka@siemens.com>

Acked-by: Chris Wright <chrisw@sous-sol.org>

> ---
>  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--;
>  	}
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Williamson April 21, 2011, 2:28 p.m. UTC | #2
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 (jan.kiszka@siemens.com) 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 <sheng@linux.intel.com>
> >>
> >> Acked-by: Chris Wright <chrisw@sous-sol.org>
> >>
> >>>>> 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


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Woodhouse April 21, 2011, 3:42 p.m. UTC | #3
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.
Alex Williamson April 21, 2011, 4:14 p.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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--;
 	}