Commit a2e7e59a authored by Robin Murphy's avatar Robin Murphy Committed by Joerg Roedel
Browse files

iommu: Avoid more races around device probe 



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: default avatarZhenhua Huang <quic_zhenhuah@quicinc.com>
Link: https://lore.kernel.org/linux-iommu/e2e20e1c-6450-4ac5-9804-b0000acdf7de@quicinc.com/


Fixes: 01657bc1 ("iommu: Avoid races around device probe")
Signed-off-by: default avatarRobin Murphy <robin.murphy@arm.com>
Acked-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: default avatarAndré Draszik <andre.draszik@linaro.org>
Tested-by: default avatarAndré Draszik <andre.draszik@linaro.org>
Link: https://lore.kernel.org/r/16f433658661d7cadfea51e7c65da95826112a2b.1700071477.git.robin.murphy@arm.com


Cc: stable@vger.kernel.org
Signed-off-by: default avatarJoerg Roedel <jroedel@suse.de>
parent a99583e2

drivers/acpi/scan.c

+6 −1
Original line number Diff line number Diff line
@@ -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

drivers/iommu/iommu.c

+10 −10
Original line number Diff line number Diff line
@@ -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;

drivers/iommu/of_iommu.c

+9 −3
Original line number Diff line number Diff line
@@ -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.

include/linux/iommu.h

+1 −0
Original line number Diff line number Diff line
@@ -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);