* [PATCH] iommu/qcom: Fix bogus detach logic
@ 2020-02-18 18:12 Robin Murphy
2020-02-18 19:05 ` Stephan Gerhold
2020-02-19 10:56 ` Joerg Roedel
0 siblings, 2 replies; 3+ messages in thread
From: Robin Murphy @ 2020-02-18 18:12 UTC (permalink / raw)
To: joro, robdclark
Cc: stephan, linux-arm-msm, Naresh Kamboju, iommu, linux-arm-kernel,
Brian Masney
Currently, the implementation of qcom_iommu_domain_free() is guaranteed
to do one of two things: WARN() and leak everything, or dereference NULL
and crash. That alone is terrible, but in fact the whole idea of trying
to track the liveness of a domain via the qcom_domain->iommu pointer as
a sanity check is full of fundamentally flawed assumptions. Make things
robust and actually functional by not trying to be quite so clever.
Reported-by: Brian Masney <masneyb@onstation.org>
Tested-by: Brian Masney <masneyb@onstation.org>
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Fixes: 0ae349a0f33f ("iommu/qcom: Add qcom_iommu")
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/iommu/qcom_iommu.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index 39759db4f003..4328da0b0a9f 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -344,21 +344,19 @@ static void qcom_iommu_domain_free(struct iommu_domain *domain)
{
struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
- if (WARN_ON(qcom_domain->iommu)) /* forgot to detach? */
- return;
-
iommu_put_dma_cookie(domain);
- /* NOTE: unmap can be called after client device is powered off,
- * for example, with GPUs or anything involving dma-buf. So we
- * cannot rely on the device_link. Make sure the IOMMU is on to
- * avoid unclocked accesses in the TLB inv path:
- */
- pm_runtime_get_sync(qcom_domain->iommu->dev);
-
- free_io_pgtable_ops(qcom_domain->pgtbl_ops);
-
- pm_runtime_put_sync(qcom_domain->iommu->dev);
+ if (qcom_domain->iommu) {
+ /*
+ * NOTE: unmap can be called after client device is powered
+ * off, for example, with GPUs or anything involving dma-buf.
+ * So we cannot rely on the device_link. Make sure the IOMMU
+ * is on to avoid unclocked accesses in the TLB inv path:
+ */
+ pm_runtime_get_sync(qcom_domain->iommu->dev);
+ free_io_pgtable_ops(qcom_domain->pgtbl_ops);
+ pm_runtime_put_sync(qcom_domain->iommu->dev);
+ }
kfree(qcom_domain);
}
@@ -404,7 +402,7 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
unsigned i;
- if (!qcom_domain->iommu)
+ if (WARN_ON(!qcom_domain->iommu))
return;
pm_runtime_get_sync(qcom_iommu->dev);
@@ -417,8 +415,6 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
ctx->domain = NULL;
}
pm_runtime_put_sync(qcom_iommu->dev);
-
- qcom_domain->iommu = NULL;
}
static int qcom_iommu_map(struct iommu_domain *domain, unsigned long iova,
--
2.23.0.dirty
_______________________________________________
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] 3+ messages in thread* Re: [PATCH] iommu/qcom: Fix bogus detach logic
2020-02-18 18:12 [PATCH] iommu/qcom: Fix bogus detach logic Robin Murphy
@ 2020-02-18 19:05 ` Stephan Gerhold
2020-02-19 10:56 ` Joerg Roedel
1 sibling, 0 replies; 3+ messages in thread
From: Stephan Gerhold @ 2020-02-18 19:05 UTC (permalink / raw)
To: Robin Murphy
Cc: linux-arm-msm, joro, iommu, robdclark, Naresh Kamboju,
linux-arm-kernel, Brian Masney
On Tue, Feb 18, 2020 at 06:12:41PM +0000, Robin Murphy wrote:
> Currently, the implementation of qcom_iommu_domain_free() is guaranteed
> to do one of two things: WARN() and leak everything, or dereference NULL
> and crash. That alone is terrible, but in fact the whole idea of trying
> to track the liveness of a domain via the qcom_domain->iommu pointer as
> a sanity check is full of fundamentally flawed assumptions. Make things
> robust and actually functional by not trying to be quite so clever.
>
> Reported-by: Brian Masney <masneyb@onstation.org>
> Tested-by: Brian Masney <masneyb@onstation.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Fixes: 0ae349a0f33f ("iommu/qcom: Add qcom_iommu")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
This fixes the warning reported by Naresh Kamboju [1] for me. Thank you!
Tested-by: Stephan Gerhold <stephan@gerhold.net>
[1]: https://lore.kernel.org/linux-arm-msm/CA+G9fYtScOpkLvx=__gP903uJ2v87RwZgkAuL6RpF9_DTDs9Zw@mail.gmail.com/
> ---
> drivers/iommu/qcom_iommu.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> index 39759db4f003..4328da0b0a9f 100644
> --- a/drivers/iommu/qcom_iommu.c
> +++ b/drivers/iommu/qcom_iommu.c
> @@ -344,21 +344,19 @@ static void qcom_iommu_domain_free(struct iommu_domain *domain)
> {
> struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
>
> - if (WARN_ON(qcom_domain->iommu)) /* forgot to detach? */
> - return;
> -
> iommu_put_dma_cookie(domain);
>
> - /* NOTE: unmap can be called after client device is powered off,
> - * for example, with GPUs or anything involving dma-buf. So we
> - * cannot rely on the device_link. Make sure the IOMMU is on to
> - * avoid unclocked accesses in the TLB inv path:
> - */
> - pm_runtime_get_sync(qcom_domain->iommu->dev);
> -
> - free_io_pgtable_ops(qcom_domain->pgtbl_ops);
> -
> - pm_runtime_put_sync(qcom_domain->iommu->dev);
> + if (qcom_domain->iommu) {
> + /*
> + * NOTE: unmap can be called after client device is powered
> + * off, for example, with GPUs or anything involving dma-buf.
> + * So we cannot rely on the device_link. Make sure the IOMMU
> + * is on to avoid unclocked accesses in the TLB inv path:
> + */
> + pm_runtime_get_sync(qcom_domain->iommu->dev);
> + free_io_pgtable_ops(qcom_domain->pgtbl_ops);
> + pm_runtime_put_sync(qcom_domain->iommu->dev);
> + }
>
> kfree(qcom_domain);
> }
> @@ -404,7 +402,7 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
> struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
> unsigned i;
>
> - if (!qcom_domain->iommu)
> + if (WARN_ON(!qcom_domain->iommu))
> return;
>
> pm_runtime_get_sync(qcom_iommu->dev);
> @@ -417,8 +415,6 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
> ctx->domain = NULL;
> }
> pm_runtime_put_sync(qcom_iommu->dev);
> -
> - qcom_domain->iommu = NULL;
> }
>
> static int qcom_iommu_map(struct iommu_domain *domain, unsigned long iova,
> --
> 2.23.0.dirty
>
_______________________________________________
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] 3+ messages in thread* Re: [PATCH] iommu/qcom: Fix bogus detach logic
2020-02-18 18:12 [PATCH] iommu/qcom: Fix bogus detach logic Robin Murphy
2020-02-18 19:05 ` Stephan Gerhold
@ 2020-02-19 10:56 ` Joerg Roedel
1 sibling, 0 replies; 3+ messages in thread
From: Joerg Roedel @ 2020-02-19 10:56 UTC (permalink / raw)
To: Robin Murphy
Cc: stephan, linux-arm-msm, Naresh Kamboju, iommu, robdclark,
linux-arm-kernel, Brian Masney
On Tue, Feb 18, 2020 at 06:12:41PM +0000, Robin Murphy wrote:
> Currently, the implementation of qcom_iommu_domain_free() is guaranteed
> to do one of two things: WARN() and leak everything, or dereference NULL
> and crash. That alone is terrible, but in fact the whole idea of trying
> to track the liveness of a domain via the qcom_domain->iommu pointer as
> a sanity check is full of fundamentally flawed assumptions. Make things
> robust and actually functional by not trying to be quite so clever.
>
> Reported-by: Brian Masney <masneyb@onstation.org>
> Tested-by: Brian Masney <masneyb@onstation.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Fixes: 0ae349a0f33f ("iommu/qcom: Add qcom_iommu")
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/iommu/qcom_iommu.c | 28 ++++++++++++----------------
> 1 file changed, 12 insertions(+), 16 deletions(-)
Applied, thanks.
_______________________________________________
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] 3+ messages in thread
end of thread, other threads:[~2020-02-19 10:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-18 18:12 [PATCH] iommu/qcom: Fix bogus detach logic Robin Murphy
2020-02-18 19:05 ` Stephan Gerhold
2020-02-19 10:56 ` Joerg Roedel
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).