| Message ID | be92829c6e5467634b109add002351e6cf9e18d2.1582049382.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | faf305c51aeabd1ea2d7131e798ef5f55f4a7750 |
| Headers | show |
| Series | iommu/qcom: Fix bogus detach logic | expand |
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 >
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.
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,