Re: Crash in intel_iommu_assign_device

[Jan Kiszka]

Am 02.11.2010 07:52, Sheng Yang wrote:
> On Monday 01 November 2010 19:41:21 Jan Kiszka wrote:
>> Hi Sheng,
>>
>> I'm not claiming to understand the details, but this looks like use
>> (dereference of pte via dma_pte_addr) after release (free_pgtable_page
>> of dmar_domain->pgd aka pte) to me:
>>
>> static int intel_iommu_attach_device(struct iommu_domain *domain,
>> 				     struct device *dev)
>> {
>> 	[...]
>> 		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));
>> 		}
>>
>> At least it crashes here right on pte->val access. Swap both lines?
> 
> I think code is right.
> 
> The comment above indicate the case: the code want to decrease the level of page 
> table. Mostly it is a 4 level page table, and the code would turn it into 3 levels 
> pagetable. What the code did is just get the first entry of the old pagetable level 
> 4, then free the level 4 pagetable's page, and make the pagetable to a level 3 
> pagetable.
> 
> Seems it make no sense to swap the lines...

It fixes the crash here, and I'm convinced the current code is wrong.
See the patch I've just sent out.

Jan

Attachment: signature.asc
Description: OpenPGP digital signature