linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] iommu: Check return of __iommu_attach_device()
@ 2020-11-19 16:58 Shameer Kolothum
  2020-11-20 11:15 ` Will Deacon
  2020-11-23 15:47 ` Will Deacon
  0 siblings, 2 replies; 5+ messages in thread
From: Shameer Kolothum @ 2020-11-19 16:58 UTC (permalink / raw)
  To: linux-arm-kernel, iommu; +Cc: joro, robin.murphy, linuxarm

Currently iommu_create_device_direct_mappings() is called
without checking the return of __iommu_attach_device(). This
may result in failures in iommu driver if dev attach returns
error.

Fixes: ce574c27ae27("iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device()")
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
Crash log:
[   31.353605] hns3 0000:7d:00.3: Adding to iommu group 10
[   31.358822] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
[   31.367567] Mem abort info:
[   31.370350]   ESR = 0x96000004
[   31.373391]   EC = 0x25: DABT (current EL), IL = 32 bits
[   31.378680]   SET = 0, FnV = 0
[   31.381720]   EA = 0, S1PTW = 0
[   31.384847] Data abort info:
[   31.387716]   ISV = 0, ISS = 0x00000004
[   31.391535]   CM = 0, WnR = 0
[   31.394491] [0000000000000018] user address but active_mm is swapper
[   31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP
[   31.406365] Modules linked in:
[   31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc4-00008-gdd5aba9d719-dirty #79
[   31.417980] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020
[   31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--)
[   31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0
[   31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0
[   31.443190] sp : ffff80001043b4e0
...
[   31.531175] Call trace:
[   31.533613]  arm_smmu_tlb_inv_range+0x178/0x1f0
[   31.538122]  arm_smmu_iotlb_sync+0x2c/0x38
[   31.542200]  iommu_unmap+0x60/0x90
[   31.545585]  __iommu_map+0x110/0x1f0
[   31.549144]  iommu_create_device_direct_mappings.isra.34+0x1ac/0x250
[   31.555468]  iommu_probe_device+0x6c/0x110
[   31.559551]  iort_iommu_configure_id+0x114/0x218
[   31.564148]  acpi_dma_configure_id+0x94/0xe0
[   31.568402]  pci_dma_configure+0xc8/0xf0
[   31.572310]  really_probe+0xd4/0x3e0
[   31.575871]  driver_probe_device+0x5c/0xc0

---
 drivers/iommu/iommu.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b53446bb8c6b..0f4dc25d46c9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev)
 	 */
 	iommu_alloc_default_domain(group, dev);
 
-	if (group->default_domain)
+	if (group->default_domain) {
 		ret = __iommu_attach_device(group->default_domain, dev);
+		if (ret) {
+			iommu_group_put(group);
+			goto err_release;
+		}
+	}
 
 	iommu_create_device_direct_mappings(group, dev);
 
 	iommu_group_put(group);
 
-	if (ret)
-		goto err_release;
-
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Check return of __iommu_attach_device()
  2020-11-19 16:58 [PATCH] iommu: Check return of __iommu_attach_device() Shameer Kolothum
@ 2020-11-20 11:15 ` Will Deacon
  2020-11-20 14:07   ` Robin Murphy
  2020-11-23 15:47 ` Will Deacon
  1 sibling, 1 reply; 5+ messages in thread
From: Will Deacon @ 2020-11-20 11:15 UTC (permalink / raw)
  To: Shameer Kolothum; +Cc: iommu, robin.murphy, linuxarm, linux-arm-kernel

On Thu, Nov 19, 2020 at 04:58:46PM +0000, Shameer Kolothum wrote:
> Currently iommu_create_device_direct_mappings() is called
> without checking the return of __iommu_attach_device(). This
> may result in failures in iommu driver if dev attach returns
> error.
> 
> Fixes: ce574c27ae27("iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device()")
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> Crash log:
> [   31.353605] hns3 0000:7d:00.3: Adding to iommu group 10
> [   31.358822] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
> [   31.367567] Mem abort info:
> [   31.370350]   ESR = 0x96000004
> [   31.373391]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   31.378680]   SET = 0, FnV = 0
> [   31.381720]   EA = 0, S1PTW = 0
> [   31.384847] Data abort info:
> [   31.387716]   ISV = 0, ISS = 0x00000004
> [   31.391535]   CM = 0, WnR = 0
> [   31.394491] [0000000000000018] user address but active_mm is swapper
> [   31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> [   31.406365] Modules linked in:
> [   31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc4-00008-gdd5aba9d719-dirty #79
> [   31.417980] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020
> [   31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--)
> [   31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0
> [   31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0
> [   31.443190] sp : ffff80001043b4e0
> ...
> [   31.531175] Call trace:
> [   31.533613]  arm_smmu_tlb_inv_range+0x178/0x1f0
> [   31.538122]  arm_smmu_iotlb_sync+0x2c/0x38
> [   31.542200]  iommu_unmap+0x60/0x90
> [   31.545585]  __iommu_map+0x110/0x1f0
> [   31.549144]  iommu_create_device_direct_mappings.isra.34+0x1ac/0x250
> [   31.555468]  iommu_probe_device+0x6c/0x110
> [   31.559551]  iort_iommu_configure_id+0x114/0x218
> [   31.564148]  acpi_dma_configure_id+0x94/0xe0
> [   31.568402]  pci_dma_configure+0xc8/0xf0
> [   31.572310]  really_probe+0xd4/0x3e0
> [   31.575871]  driver_probe_device+0x5c/0xc0
> 
> ---
>  drivers/iommu/iommu.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index b53446bb8c6b..0f4dc25d46c9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev)
>  	 */
>  	iommu_alloc_default_domain(group, dev);
>  
> -	if (group->default_domain)
> +	if (group->default_domain) {
>  		ret = __iommu_attach_device(group->default_domain, dev);
> +		if (ret) {
> +			iommu_group_put(group);
> +			goto err_release;
> +		}
> +	}

This looks sensible to me, but what I don't understand is where that
NULL pointer is coming from in the first place. iommu_map() operates
on the domain, so why does it matter if the attach fails? What is being
accessed at arm_smmu_tlb_inv_range+0x178/0x1f0 ?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Check return of __iommu_attach_device()
  2020-11-20 11:15 ` Will Deacon
@ 2020-11-20 14:07   ` Robin Murphy
  2020-11-20 16:53     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2020-11-20 14:07 UTC (permalink / raw)
  To: Will Deacon, Shameer Kolothum; +Cc: iommu, linuxarm, linux-arm-kernel

On 2020-11-20 11:15, Will Deacon wrote:
> On Thu, Nov 19, 2020 at 04:58:46PM +0000, Shameer Kolothum wrote:
>> Currently iommu_create_device_direct_mappings() is called
>> without checking the return of __iommu_attach_device(). This
>> may result in failures in iommu driver if dev attach returns
>> error.
>>
>> Fixes: ce574c27ae27("iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device()")
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>> Crash log:
>> [   31.353605] hns3 0000:7d:00.3: Adding to iommu group 10
>> [   31.358822] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018
>> [   31.367567] Mem abort info:
>> [   31.370350]   ESR = 0x96000004
>> [   31.373391]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [   31.378680]   SET = 0, FnV = 0
>> [   31.381720]   EA = 0, S1PTW = 0
>> [   31.384847] Data abort info:
>> [   31.387716]   ISV = 0, ISS = 0x00000004
>> [   31.391535]   CM = 0, WnR = 0
>> [   31.394491] [0000000000000018] user address but active_mm is swapper
>> [   31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>> [   31.406365] Modules linked in:
>> [   31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc4-00008-gdd5aba9d719-dirty #79
>> [   31.417980] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020
>> [   31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--)
>> [   31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0
>> [   31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0
>> [   31.443190] sp : ffff80001043b4e0
>> ...
>> [   31.531175] Call trace:
>> [   31.533613]  arm_smmu_tlb_inv_range+0x178/0x1f0
>> [   31.538122]  arm_smmu_iotlb_sync+0x2c/0x38
>> [   31.542200]  iommu_unmap+0x60/0x90
>> [   31.545585]  __iommu_map+0x110/0x1f0
>> [   31.549144]  iommu_create_device_direct_mappings.isra.34+0x1ac/0x250
>> [   31.555468]  iommu_probe_device+0x6c/0x110
>> [   31.559551]  iort_iommu_configure_id+0x114/0x218
>> [   31.564148]  acpi_dma_configure_id+0x94/0xe0
>> [   31.568402]  pci_dma_configure+0xc8/0xf0
>> [   31.572310]  really_probe+0xd4/0x3e0
>> [   31.575871]  driver_probe_device+0x5c/0xc0
>>
>> ---
>>   drivers/iommu/iommu.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index b53446bb8c6b..0f4dc25d46c9 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev)
>>   	 */
>>   	iommu_alloc_default_domain(group, dev);
>>   
>> -	if (group->default_domain)
>> +	if (group->default_domain) {
>>   		ret = __iommu_attach_device(group->default_domain, dev);
>> +		if (ret) {
>> +			iommu_group_put(group);
>> +			goto err_release;
>> +		}
>> +	}
> 
> This looks sensible to me, but what I don't understand is where that
> NULL pointer is coming from in the first place. iommu_map() operates
> on the domain, so why does it matter if the attach fails? What is being
> accessed at arm_smmu_tlb_inv_range+0x178/0x1f0 ?

Probably because the domain is a hollow fake until the first successful 
attach - even TLB maintenance depends on having decided a pagetable format.

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* RE: [PATCH] iommu: Check return of __iommu_attach_device()
  2020-11-20 14:07   ` Robin Murphy
@ 2020-11-20 16:53     ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 5+ messages in thread
From: Shameerali Kolothum Thodi @ 2020-11-20 16:53 UTC (permalink / raw)
  To: Robin Murphy, Will Deacon; +Cc: iommu, Linuxarm, linux-arm-kernel



> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 20 November 2020 14:07
> To: Will Deacon <will@kernel.org>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> Cc: linux-arm-kernel@lists.infradead.org; iommu@lists.linux-foundation.org;
> Linuxarm <linuxarm@huawei.com>
> Subject: Re: [PATCH] iommu: Check return of __iommu_attach_device()
> 
> On 2020-11-20 11:15, Will Deacon wrote:
> > On Thu, Nov 19, 2020 at 04:58:46PM +0000, Shameer Kolothum wrote:
> >> Currently iommu_create_device_direct_mappings() is called
> >> without checking the return of __iommu_attach_device(). This
> >> may result in failures in iommu driver if dev attach returns
> >> error.
> >>
> >> Fixes: ce574c27ae27("iommu: Move
> iommu_group_create_direct_mappings() out of iommu_group_add_device()")
> >> Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >> Crash log:
> >> [   31.353605] hns3 0000:7d:00.3: Adding to iommu group 10
> >> [   31.358822] Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000018
> >> [   31.367567] Mem abort info:
> >> [   31.370350]   ESR = 0x96000004
> >> [   31.373391]   EC = 0x25: DABT (current EL), IL = 32 bits
> >> [   31.378680]   SET = 0, FnV = 0
> >> [   31.381720]   EA = 0, S1PTW = 0
> >> [   31.384847] Data abort info:
> >> [   31.387716]   ISV = 0, ISS = 0x00000004
> >> [   31.391535]   CM = 0, WnR = 0
> >> [   31.394491] [0000000000000018] user address but active_mm is
> swapper
> >> [   31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> >> [   31.406365] Modules linked in:
> >> [   31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted
> 5.10.0-rc4-00008-gdd5aba9d719-dirty #79
> >> [   31.417980] Hardware name: Huawei TaiShan 200 (Model
> 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020
> >> [   31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--)
> >> [   31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0
> >> [   31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0
> >> [   31.443190] sp : ffff80001043b4e0
> >> ...
> >> [   31.531175] Call trace:
> >> [   31.533613]  arm_smmu_tlb_inv_range+0x178/0x1f0
> >> [   31.538122]  arm_smmu_iotlb_sync+0x2c/0x38
> >> [   31.542200]  iommu_unmap+0x60/0x90
> >> [   31.545585]  __iommu_map+0x110/0x1f0
> >> [   31.549144]
> iommu_create_device_direct_mappings.isra.34+0x1ac/0x250
> >> [   31.555468]  iommu_probe_device+0x6c/0x110
> >> [   31.559551]  iort_iommu_configure_id+0x114/0x218
> >> [   31.564148]  acpi_dma_configure_id+0x94/0xe0
> >> [   31.568402]  pci_dma_configure+0xc8/0xf0
> >> [   31.572310]  really_probe+0xd4/0x3e0
> >> [   31.575871]  driver_probe_device+0x5c/0xc0
> >>
> >> ---
> >>   drivers/iommu/iommu.c | 10 ++++++----
> >>   1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index b53446bb8c6b..0f4dc25d46c9 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev)
> >>   	 */
> >>   	iommu_alloc_default_domain(group, dev);
> >>
> >> -	if (group->default_domain)
> >> +	if (group->default_domain) {
> >>   		ret = __iommu_attach_device(group->default_domain, dev);
> >> +		if (ret) {
> >> +			iommu_group_put(group);
> >> +			goto err_release;
> >> +		}
> >> +	}
> >
> > This looks sensible to me, but what I don't understand is where that
> > NULL pointer is coming from in the first place. iommu_map() operates
> > on the domain, so why does it matter if the attach fails? What is being
> > accessed at arm_smmu_tlb_inv_range+0x178/0x1f0 ?
> 
> Probably because the domain is a hollow fake until the first successful
> attach - even TLB maintenance depends on having decided a pagetable format.

I think, in this particular instance, what happens is, dev reports RMR
regions (IOMMU_RESV_DIRECT) but attach_dev() fails early without
setting, smmu_domain->smmu  =  smmu.

iommu_probe_device()
  __iommu_attach_dev()  -->return err, but carries on.
  iommu_create_device_direct_mappings()
    iommu_get_resv_regions()  --> dev has IOMMU_RESV_DIRECT regions
    iommu_map()  
      __iommu_map()   
        arm_smmu_map()  -->return err
      iommu_unmap()    --> unroll on map failure
        __iommu_unmap()  --> size is zero. So returns.
        iommu_iotlb_sync()
          arm_smmu_iotlb_sync()
            arm_smmu_tlb_inv_range()  --> smmu is NULL

Thanks,
Shameer 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: Check return of __iommu_attach_device()
  2020-11-19 16:58 [PATCH] iommu: Check return of __iommu_attach_device() Shameer Kolothum
  2020-11-20 11:15 ` Will Deacon
@ 2020-11-23 15:47 ` Will Deacon
  1 sibling, 0 replies; 5+ messages in thread
From: Will Deacon @ 2020-11-23 15:47 UTC (permalink / raw)
  To: Shameer Kolothum, linux-arm-kernel, iommu
  Cc: Will Deacon, catalin.marinas, joro, robin.murphy, linuxarm, kernel-team

On Thu, 19 Nov 2020 16:58:46 +0000, Shameer Kolothum wrote:
> Currently iommu_create_device_direct_mappings() is called
> without checking the return of __iommu_attach_device(). This
> may result in failures in iommu driver if dev attach returns
> error.

Applied to arm64 (for-next/iommu/fixes), thanks!

[1/1] iommu: Check return of __iommu_attach_device()
      https://git.kernel.org/arm64/c/77c38c8cf52e

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-23 15:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 16:58 [PATCH] iommu: Check return of __iommu_attach_device() Shameer Kolothum
2020-11-20 11:15 ` Will Deacon
2020-11-20 14:07   ` Robin Murphy
2020-11-20 16:53     ` Shameerali Kolothum Thodi
2020-11-23 15:47 ` Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).