Hi Robin, On Wed, 2023-11-15 at 18:25 +0000, Robin Murphy wrote: > It turns out there are more subtle races beyond just the main part of > __iommu_probe_device() itself running in parallel - the > dev_iommu_free() > on the way out of an unsuccessful probe can still manage to trip up > concurrent accesses to a device's fwspec. Thus, extend the scope of > iommu_probe_device_lock() to also serialise fwspec creation and > initial > retrieval. > > Reported-by: Zhenhua Huang <quic_zhenhuah@xxxxxxxxxxx> > Link: > https://lore.kernel.org/linux-iommu/e2e20e1c-6450-4ac5-9804-b0000acdf7de@xxxxxxxxxxx/ > Fixes: 01657bc14a39 ("iommu: Avoid races around device probe") > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx> Reviewed-by: André Draszik <andre.draszik@xxxxxxxxxx> Tested-by: André Draszik <andre.draszik@xxxxxxxxxx> (using continuous boot test loop) I like that this is easily back-portable to 6.1, thanks for this patch :-) Cheers, André > --- > > This is my idea of a viable fix, since it does not need a 700-line > diffstat to make the code do what it was already *trying* to do > anyway. > This stuff should fundamentally not be hanging off driver probe in > the > first place, so I'd rather get on with removing the underlying > brokenness than waste time and effort polishing it any further. > > drivers/acpi/scan.c | 7 ++++++- > drivers/iommu/iommu.c | 20 ++++++++++---------- > drivers/iommu/of_iommu.c | 12 +++++++++--- > include/linux/iommu.h | 1 + > 4 files changed, 26 insertions(+), 14 deletions(-) > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c > index fa5dd71a80fa..02bb2cce423f 100644 > --- a/drivers/acpi/scan.c > +++ b/drivers/acpi/scan.c > @@ -1568,17 +1568,22 @@ static const struct iommu_ops > *acpi_iommu_configure_id(struct device *dev, > int err; > const struct iommu_ops *ops; > > + /* Serialise to make dev->iommu stable under our potential > fwspec */ > + mutex_lock(&iommu_probe_device_lock); > /* > * If we already translated the fwspec there is nothing left > to do, > * return the iommu_ops. > */ > ops = acpi_iommu_fwspec_ops(dev); > - if (ops) > + if (ops) { > + mutex_unlock(&iommu_probe_device_lock); > return ops; > + } > > err = iort_iommu_configure_id(dev, id_in); > if (err && err != -EPROBE_DEFER) > err = viot_iommu_configure(dev); > + mutex_unlock(&iommu_probe_device_lock); > > /* > * If we have reason to believe the IOMMU driver missed the > initial > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index f17a1113f3d6..e0c962648dde 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -485,11 +485,12 @@ static void iommu_deinit_device(struct device > *dev) > dev_iommu_free(dev); > } > > +DEFINE_MUTEX(iommu_probe_device_lock); > + > static int __iommu_probe_device(struct device *dev, struct list_head > *group_list) > { > const struct iommu_ops *ops = dev->bus->iommu_ops; > struct iommu_group *group; > - static DEFINE_MUTEX(iommu_probe_device_lock); > struct group_device *gdev; > int ret; > > @@ -502,17 +503,15 @@ static int __iommu_probe_device(struct device > *dev, struct list_head *group_list > * probably be able to use device_lock() here to minimise > the scope, > * but for now enforcing a simple global ordering is fine. > */ > - mutex_lock(&iommu_probe_device_lock); > + lockdep_assert_held(&iommu_probe_device_lock); > > /* Device is probed already if in a group */ > - if (dev->iommu_group) { > - ret = 0; > - goto out_unlock; > - } > + if (dev->iommu_group) > + return 0; > > ret = iommu_init_device(dev, ops); > if (ret) > - goto out_unlock; > + return ret; > > group = dev->iommu_group; > gdev = iommu_group_alloc_device(group, dev); > @@ -548,7 +547,6 @@ static int __iommu_probe_device(struct device > *dev, struct list_head *group_list > list_add_tail(&group->entry, group_list); > } > mutex_unlock(&group->mutex); > - mutex_unlock(&iommu_probe_device_lock); > > if (dev_is_pci(dev)) > iommu_dma_set_pci_32bit_workaround(dev); > @@ -562,8 +560,6 @@ static int __iommu_probe_device(struct device > *dev, struct list_head *group_list > iommu_deinit_device(dev); > mutex_unlock(&group->mutex); > iommu_group_put(group); > -out_unlock: > - mutex_unlock(&iommu_probe_device_lock); > > return ret; > } > @@ -573,7 +569,9 @@ int iommu_probe_device(struct device *dev) > const struct iommu_ops *ops; > int ret; > > + mutex_lock(&iommu_probe_device_lock); > ret = __iommu_probe_device(dev, NULL); > + mutex_unlock(&iommu_probe_device_lock); > if (ret) > return ret; > > @@ -1822,7 +1820,9 @@ static int probe_iommu_group(struct device > *dev, void *data) > struct list_head *group_list = data; > int ret; > > + mutex_lock(&iommu_probe_device_lock); > ret = __iommu_probe_device(dev, group_list); > + mutex_unlock(&iommu_probe_device_lock); > if (ret == -ENODEV) > ret = 0; > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 157b286e36bf..c25b4ae6aeee 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -112,16 +112,20 @@ const struct iommu_ops > *of_iommu_configure(struct device *dev, > const u32 *id) > { > const struct iommu_ops *ops = NULL; > - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > + struct iommu_fwspec *fwspec; > int err = NO_IOMMU; > > if (!master_np) > return NULL; > > + /* Serialise to make dev->iommu stable under our potential > fwspec */ > + mutex_lock(&iommu_probe_device_lock); > + fwspec = dev_iommu_fwspec_get(dev); > if (fwspec) { > - if (fwspec->ops) > + if (fwspec->ops) { > + mutex_unlock(&iommu_probe_device_lock); > return fwspec->ops; > - > + } > /* In the deferred case, start again from scratch */ > iommu_fwspec_free(dev); > } > @@ -155,6 +159,8 @@ const struct iommu_ops *of_iommu_configure(struct > device *dev, > fwspec = dev_iommu_fwspec_get(dev); > ops = fwspec->ops; > } > + mutex_unlock(&iommu_probe_device_lock); > + > /* > * If we have reason to believe the IOMMU driver missed the > initial > * probe for dev, replay it to get things in order. > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index ec289c1016f5..6291aa7b079b 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -845,6 +845,7 @@ static inline void dev_iommu_priv_set(struct > device *dev, void *priv) > dev->iommu->priv = priv; > } > > +extern struct mutex iommu_probe_device_lock; > int iommu_probe_device(struct device *dev); > > int iommu_dev_enable_feature(struct device *dev, enum > iommu_dev_features f);
