Re: [PATCH] iommu: Check return of __iommu_attach_device()
On Thu, 19 Nov 2020 16:58:46 +, 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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH] iommu: Check return of __iommu_attach_device()
> -Original Message-
> From: Robin Murphy [mailto:robin.mur...@arm.com]
> Sent: 20 November 2020 14:07
> To: Will Deacon ; Shameerali Kolothum Thodi
>
> Cc: linux-arm-ker...@lists.infradead.org; iommu@lists.linux-foundation.org;
> Linuxarm
> 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 +, 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
>
> >> ---
> >> Crash log:
> >> [ 31.353605] hns3 :7d:00.3: Adding to iommu group 10
> >> [ 31.358822] Unable to handle kernel NULL pointer dereference at virtual
> address 0018
> >> [ 31.367567] Mem abort info:
> >> [ 31.370350] ESR = 0x9604
> >> [ 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 = 0x0004
> >> [ 31.391535] CM = 0, WnR = 0
> >> [ 31.394491] [0018] user address but active_mm is
> swapper
> >> [ 31.400818] Internal error: Oops: 9604 [#1] PREEMPT SMP
> >> [ 31.406365] Modules linked in:
> >> [ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted
> 5.10.0-rc4-8-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: 00c9 (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 : 80001043b4e0
> >> ...
> >> [ 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
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 +, 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
---
Crash log:
[ 31.353605] hns3 :7d:00.3: Adding to iommu group 10
[ 31.358822] Unable to handle kernel NULL pointer dereference at virtual
address 0018
[ 31.367567] Mem abort info:
[ 31.370350] ESR = 0x9604
[ 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 = 0x0004
[ 31.391535] CM = 0, WnR = 0
[ 31.394491] [0018] user address but active_mm is swapper
[ 31.400818] Internal error: Oops: 9604 [#1] PREEMPT SMP
[ 31.406365] Modules linked in:
[ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted
5.10.0-rc4-8-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: 00c9 (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 : 80001043b4e0
...
[ 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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu: Check return of __iommu_attach_device()
On Thu, Nov 19, 2020 at 04:58:46PM +, 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
> ---
> Crash log:
> [ 31.353605] hns3 :7d:00.3: Adding to iommu group 10
> [ 31.358822] Unable to handle kernel NULL pointer dereference at virtual
> address 0018
> [ 31.367567] Mem abort info:
> [ 31.370350] ESR = 0x9604
> [ 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 = 0x0004
> [ 31.391535] CM = 0, WnR = 0
> [ 31.394491] [0018] user address but active_mm is swapper
> [ 31.400818] Internal error: Oops: 9604 [#1] PREEMPT SMP
> [ 31.406365] Modules linked in:
> [ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted
> 5.10.0-rc4-8-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: 00c9 (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 : 80001043b4e0
> ...
> [ 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu: Check return of __iommu_attach_device()
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
---
Crash log:
[ 31.353605] hns3 :7d:00.3: Adding to iommu group 10
[ 31.358822] Unable to handle kernel NULL pointer dereference at virtual
address 0018
[ 31.367567] Mem abort info:
[ 31.370350] ESR = 0x9604
[ 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 = 0x0004
[ 31.391535] CM = 0, WnR = 0
[ 31.394491] [0018] user address but active_mm is swapper
[ 31.400818] Internal error: Oops: 9604 [#1] PREEMPT SMP
[ 31.406365] Modules linked in:
[ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted
5.10.0-rc4-8-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: 00c9 (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 : 80001043b4e0
...
[ 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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
