| Message ID | 23e009bba72c3e46320c59acefbbdd976111cc8a.1663836372.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Define EINVAL as device/domain incompatibility | expand |
On Thu, Sep 22, 2022 at 01:53:44AM -0700, Nicolin Chen wrote: > diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c > index 6a24aa804ea3..14df722f0060 100644 > --- a/drivers/iommu/msm_iommu.c > +++ b/drivers/iommu/msm_iommu.c > @@ -394,6 +394,34 @@ static struct iommu_device *msm_iommu_probe_device(struct device *dev) > return &iommu->iommu; > } > > +static void msm_iommu_detach_dev(struct iommu_domain *domain, > + struct device *dev) > +{ > + struct msm_priv *priv = to_msm_priv(domain); > + unsigned long flags; > + struct msm_iommu_dev *iommu; > + struct msm_iommu_ctx_dev *master; > + int ret; > + > + free_io_pgtable_ops(priv->iop); > + > + spin_lock_irqsave(&msm_iommu_lock, flags); > + list_for_each_entry(iommu, &priv->list_attached, dom_node) { > + ret = __enable_clocks(iommu); > + if (ret) > + goto fail; > + > + list_for_each_entry(master, &iommu->ctx_list, list) { > + msm_iommu_free_ctx(iommu->context_map, master->num); > + __reset_context(iommu->base, master->num); This isn't safe if the loop in msm_iommu_attach_dev() exits early. Firstly the it leaves master->num set to -ERRNO so clear_bit will corrupt memory Secondly if the attach loop gets half way through the remaining entries have master->num == 0 and so the clear_bit() will corrupt an otherwise valid entry. Fixing all of this properly looks like a big deal, I think we should drop this patch from the series. Jason
On Thu, Sep 22, 2022 at 02:23:22PM -0300, Jason Gunthorpe wrote: > > +static void msm_iommu_detach_dev(struct iommu_domain *domain, > > + struct device *dev) > > +{ > > + struct msm_priv *priv = to_msm_priv(domain); > > + unsigned long flags; > > + struct msm_iommu_dev *iommu; > > + struct msm_iommu_ctx_dev *master; > > + int ret; > > + > > + free_io_pgtable_ops(priv->iop); > > + > > + spin_lock_irqsave(&msm_iommu_lock, flags); > > + list_for_each_entry(iommu, &priv->list_attached, dom_node) { > > + ret = __enable_clocks(iommu); > > + if (ret) > > + goto fail; > > + > > + list_for_each_entry(master, &iommu->ctx_list, list) { > > + msm_iommu_free_ctx(iommu->context_map, master->num); > > + __reset_context(iommu->base, master->num); > > This isn't safe if the loop in msm_iommu_attach_dev() exits > early. > > Firstly the it leaves master->num set to -ERRNO so clear_bit will > corrupt memory > > Secondly if the attach loop gets half way through the remaining > entries have master->num == 0 and so the clear_bit() will corrupt an > otherwise valid entry. > > Fixing all of this properly looks like a big deal, I think we should > drop this patch from the series. OK. Let me resend a version dropping this one. Thanks!
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c index 6a24aa804ea3..14df722f0060 100644 --- a/drivers/iommu/msm_iommu.c +++ b/drivers/iommu/msm_iommu.c @@ -394,6 +394,34 @@ static struct iommu_device *msm_iommu_probe_device(struct device *dev) return &iommu->iommu; } +static void msm_iommu_detach_dev(struct iommu_domain *domain, + struct device *dev) +{ + struct msm_priv *priv = to_msm_priv(domain); + unsigned long flags; + struct msm_iommu_dev *iommu; + struct msm_iommu_ctx_dev *master; + int ret; + + free_io_pgtable_ops(priv->iop); + + spin_lock_irqsave(&msm_iommu_lock, flags); + list_for_each_entry(iommu, &priv->list_attached, dom_node) { + ret = __enable_clocks(iommu); + if (ret) + goto fail; + + list_for_each_entry(master, &iommu->ctx_list, list) { + msm_iommu_free_ctx(iommu->context_map, master->num); + __reset_context(iommu->base, master->num); + master->num = 0; + } + __disable_clocks(iommu); + } +fail: + spin_unlock_irqrestore(&msm_iommu_lock, flags); +} + static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) { int ret = 0; @@ -418,6 +446,7 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) list_for_each_entry(master, &iommu->ctx_list, list) { if (master->num) { dev_err(dev, "domain already attached"); + __disable_clocks(iommu); ret = -EEXIST; goto fail; } @@ -425,6 +454,7 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) msm_iommu_alloc_ctx(iommu->context_map, 0, iommu->ncb); if (IS_ERR_VALUE(master->num)) { + __disable_clocks(iommu); ret = -ENODEV; goto fail; } @@ -439,37 +469,12 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) fail: spin_unlock_irqrestore(&msm_iommu_lock, flags); + if (ret) + msm_iommu_detach_dev(domain, dev); return ret; } -static void msm_iommu_detach_dev(struct iommu_domain *domain, - struct device *dev) -{ - struct msm_priv *priv = to_msm_priv(domain); - unsigned long flags; - struct msm_iommu_dev *iommu; - struct msm_iommu_ctx_dev *master; - int ret; - - free_io_pgtable_ops(priv->iop); - - spin_lock_irqsave(&msm_iommu_lock, flags); - list_for_each_entry(iommu, &priv->list_attached, dom_node) { - ret = __enable_clocks(iommu); - if (ret) - goto fail; - - list_for_each_entry(master, &iommu->ctx_list, list) { - msm_iommu_free_ctx(iommu->context_map, master->num); - __reset_context(iommu->base, master->num); - } - __disable_clocks(iommu); - } -fail: - spin_unlock_irqrestore(&msm_iommu_lock, flags); -} - static int msm_iommu_map(struct iommu_domain *domain, unsigned long iova, phys_addr_t pa, size_t len, int prot, gfp_t gfp) {
The error-out routine is missing all the reverting pieces for the iop and attached-ctx allocations. And clock enable/disable is unbalanced too. Fix it by adding __disable_clocks() and calling msm_iommu_detach_dev() at the end of the msm_iommu_attach_dev() if "ret" is non-zero. Also set the master->num to 0 in the detach_dev() since attach_dev() would check it. Fixes: 109bd48ea2e1 ("iommu/msm: Add DT adaptation") Cc: stable@vger.kernel.org Cc: Sricharan R <sricharan@codeaurora.org> Cc: Andy Gross <agross@kernel.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Cc: Konrad Dybcio <konrad.dybcio@somainline.org> Reviewed-by: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/msm_iommu.c | 59 +++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 27 deletions(-)