Skip to contentSkip to conversation

amd_iommu: attach device fails on the last pci device

PATCH In review Shuah Khan 3 participants 6 messages Oct 12, 2012 – Oct 23, 2012 x86

lore
Share & tools
lore

Shuah Khan

author
#0 lore raw

PATCH 1/1 amd_iommu: attach device fails on the last pci device

amd_iommu_attach_device() checks if device id is within the limits of amd_iommu_last_bdf and instead checking if devid > amd_iommu_last_bdf, it checks devid >= amd_iommu_last_bdf. As a result the last device attach fails because amd_iommu_attach_device() returns an -EINVAL.

This bug is in linux-2.6.32 and an equivalent fix in linux-2.6.33 and has been carried forward to later kernels and is in the upstream kernel. This equivalent fix includes restructuring and consolidating device checks into a routine check_device(). Instead of back-porting all of that work, spot-fixed the bug in amd_iommu_attach_device() for linux-2.6.32.

Signed-off-by Shuah Khan <[email protected]>
CC <[email protected]> v2.6.32
---
 arch/x86/kernel/amd_iommu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/amd_iommu.c +1 -1
index 3a44b75..67de7d7 100644
@@ -2288,7 +2288,7 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 
 	devid = calc_devid(pdev->bus->number, pdev->devfn);
 
-	if (devid >= amd_iommu_last_bdf ||
+	if (devid > amd_iommu_last_bdf ||
 			devid != amd_iommu_alias_table[devid])
 		return -EINVAL;
Signature · 1 line
1.7.9.5
Reviewed-by on v1 1/1
#1 lore raw
Quoted 22 lines · Shuah Khan

amd_iommu_attach_device() checks if device id is within the limits of amd_iommu_last_bdf and instead checking if devid > amd_iommu_last_bdf, it checks devid >= amd_iommu_last_bdf. As a result the last device attach fails because amd_iommu_attach_device() returns an -EINVAL.

This bug is in linux-2.6.32 and an equivalent fix in linux-2.6.33 and has been carried forward to later kernels and is in the upstream kernel. This equivalent fix includes restructuring and consolidating device checks into a routine check_device(). Instead of back-porting all of that work, spot-fixed the bug in amd_iommu_attach_device() for linux-2.6.32.

Signed-off-by Shuah Khan <[email protected]>
CC <[email protected]> v2.6.32
---
 arch/x86/kernel/amd_iommu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/amd_iommu.c +0 -0
index 3a44b75..67de7d7 100644

arch/x86/kernel/amd_iommu.c static int amd_iommu_attach_device(struct iommu_domain *dom, · line 2294

Quoted 8 lines
arch/x86/kernel/amd_iommu.c +1 -1
@@ -2288,7 +2288,7 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 
 	devid = calc_devid(pdev->bus->number, pdev->devfn);
 
-	if (devid >= amd_iommu_last_bdf ||
+	if (devid > amd_iommu_last_bdf ||
 			devid != amd_iommu_alias_table[devid])
 		return -EINVAL;

Looks correct to me, this case was clearly inconsistent with other users of amd_iommu_last_bdf.

Reviewed-by Alex Williamson <[email protected]>
#2 lore raw
In reply to Shuah Khan
Quoted 5 lines · Shuah Khan

This bug is in linux-2.6.32 and an equivalent fix in linux-2.6.33 and has been carried forward to later kernels and is in the upstream kernel. This equivalent fix includes restructuring and consolidating device checks into a routine check_device(). Instead of back-porting all of that work, spot-fixed the bug in amd_iommu_attach_device() for linux-2.6.32.

To save Willy time: am I correct in guessing the upstream commit you are referring to is 98fc5a693bbdda498a556654c70d1e31a186c988 (x86/amd-iommu: Use get_device_id and check_device where appropriate, 2009-11-24)?

Thanks,
Jonathan

author
#3 lore raw
Quoted 8 lines · Jonathan Nieder
Quoted 5 lines · Shuah Khan

This bug is in linux-2.6.32 and an equivalent fix in linux-2.6.33 and has been carried forward to later kernels and is in the upstream kernel. This equivalent fix includes restructuring and consolidating device checks into a routine check_device(). Instead of back-porting all of that work, spot-fixed the bug in amd_iommu_attach_device() for linux-2.6.32.

Quoted 4 lines

To save Willy time: am I correct in guessing the upstream commit you are referring to is 98fc5a693bbdda498a556654c70d1e31a186c988 (x86/amd-iommu: Use get_device_id and check_device where appropriate, 2009-11-24)?

Yes that is one. I looked that up again and it has the change to amd_iommu_attach_device() to use check_device() and removes the check

- if (devid >= amd_iommu_last_bdf ||
- devid != amd_iommu_alias_table[devid])
- return -EINVAL;
+ devid = get_device_id(dev);

Ref: http://www.spinics.net/lists/linux-tip-commits/msg06129.html

Thanks,
-- Shuah

#4 lore raw
Quoted 8 lines · Shuah Khan
Quoted 4 lines · Jonathan Nieder

To save Willy time: am I correct in guessing the upstream commit you are referring to is 98fc5a693bbdda498a556654c70d1e31a186c988 (x86/amd-iommu: Use get_device_id and check_device where appropriate, 2009-11-24)?

Yes that is one.

Great, thanks for the quick confirmation.

Ciao,
Jonathan

author
#5 lore raw
Quoted 11 lines · Jonathan Nieder
Quoted 8 lines · Shuah Khan
Quoted 4 lines · Jonathan Nieder

To save Willy time: am I correct in guessing the upstream commit you are referring to is 98fc5a693bbdda498a556654c70d1e31a186c988 (x86/amd-iommu: Use get_device_id and check_device where appropriate, 2009-11-24)?

Yes that is one.

Great, thanks for the quick confirmation.

Is this patch in the queue to be pulled into 2.6.32 tree?

Thanks,
-- Shuah