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 <[email protected]> > Tested-by: Brian Masney <[email protected]> > Reported-by: Naresh Kamboju <[email protected]> > Fixes: 0ae349a0f33f ("iommu/qcom: Add qcom_iommu") > Signed-off-by: Robin Murphy <[email protected]>
This fixes the warning reported by Naresh Kamboju [1] for me. Thank you! Tested-by: Stephan Gerhold <[email protected]> [1]: https://lore.kernel.org/linux-arm-msm/CA+G9fYtScOpkLvx=__gp903uj2v87rwzgkaul6rpf9_dtds...@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 > _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
