From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
To: Robin Murphy <robin.murphy@arm.com>, Will Deacon <will@kernel.org>
Cc: "iommu@lists.linux-foundation.org"
<iommu@lists.linux-foundation.org>,
Linuxarm <linuxarm@huawei.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH] iommu: Check return of __iommu_attach_device()
Date: Fri, 20 Nov 2020 16:53:47 +0000 [thread overview]
Message-ID: <6375e8511bcb48209fffa0c02833e27b@huawei.com> (raw)
In-Reply-To: <337ffd34-a606-4fb1-adb0-49367c136170@arm.com>
> -----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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
To: Robin Murphy <robin.murphy@arm.com>, Will Deacon <will@kernel.org>
Cc: "iommu@lists.linux-foundation.org"
<iommu@lists.linux-foundation.org>,
Linuxarm <linuxarm@huawei.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH] iommu: Check return of __iommu_attach_device()
Date: Fri, 20 Nov 2020 16:53:47 +0000 [thread overview]
Message-ID: <6375e8511bcb48209fffa0c02833e27b@huawei.com> (raw)
In-Reply-To: <337ffd34-a606-4fb1-adb0-49367c136170@arm.com>
> -----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
next prev parent reply other threads:[~2020-11-20 16:53 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-19 16:58 [PATCH] iommu: Check return of __iommu_attach_device() Shameer Kolothum
2020-11-19 16:58 ` Shameer Kolothum
2020-11-20 11:15 ` Will Deacon
2020-11-20 11:15 ` Will Deacon
2020-11-20 14:07 ` Robin Murphy
2020-11-20 14:07 ` Robin Murphy
2020-11-20 16:53 ` Shameerali Kolothum Thodi [this message]
2020-11-20 16:53 ` Shameerali Kolothum Thodi
2020-11-23 15:47 ` Will Deacon
2020-11-23 15:47 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6375e8511bcb48209fffa0c02833e27b@huawei.com \
--to=shameerali.kolothum.thodi@huawei.com \
--cc=iommu@lists.linux-foundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linuxarm@huawei.com \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.