[PATCH] iommu: Handle default domain attach failure

[PATCH] iommu: Handle default domain attach failure

[Robin Murphy]

We wouldn't normally expect ops->attach_dev() to fail, but on IOMMUs
with limited hardware resources, or generally misconfigured systems,
it is certainly possible. We report failure correctly from the external
iommu_attach_device() interface, but do not do so in iommu_group_add()
when attaching to the default domain. The result of failure there is
that the device, group and domain all get left in a broken,
part-configured state which leads to weird errors and misbehaviour down
the line when IOMMU API calls sort-of-but-don't-quite work.

Check the return value of __iommu_attach_device() on the default domain,
and refactor the error handling paths to cope with its failure and clean
up correctly in such cases.

Fixes: e39cb8a3aa98 ("iommu: Make sure a device is always attached to a domain")
Reported-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/iommu.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dbe7f653bb7c..aed906a3e3db 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -383,36 +383,30 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	device->dev = dev;
 
 	ret = sysfs_create_link(&dev->kobj, &group->kobj, "iommu_group");
-	if (ret) {
-		kfree(device);
-		return ret;
-	}
+	if (ret)
+		goto err_free_device;
 
 	device->name = kasprintf(GFP_KERNEL, "%s", kobject_name(&dev->kobj));
 rename:
 	if (!device->name) {
-		sysfs_remove_link(&dev->kobj, "iommu_group");
-		kfree(device);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_remove_link;
 	}
 
 	ret = sysfs_create_link_nowarn(group->devices_kobj,
 				       &dev->kobj, device->name);
 	if (ret) {
-		kfree(device->name);
 		if (ret == -EEXIST && i >= 0) {
 			/*
 			 * Account for the slim chance of collision
 			 * and append an instance to the name.
 			 */
+			kfree(device->name);
 			device->name = kasprintf(GFP_KERNEL, "%s.%d",
 						 kobject_name(&dev->kobj), i++);
 			goto rename;
 		}
-
-		sysfs_remove_link(&dev->kobj, "iommu_group");
-		kfree(device);
-		return ret;
+		goto err_free_name;
 	}
 
 	kobject_get(group->devices_kobj);
@@ -424,8 +418,10 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_lock(&group->mutex);
 	list_add_tail(&device->list, &group->devices);
 	if (group->domain)
-		__iommu_attach_device(group->domain, dev);
+		ret = __iommu_attach_device(group->domain, dev);
 	mutex_unlock(&group->mutex);
+	if (ret)
+		goto err_put_group;
 
 	/* Notify any listeners about change to group. */
 	blocking_notifier_call_chain(&group->notifier,
@@ -436,6 +432,21 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	pr_info("Adding device %s to group %d\n", dev_name(dev), group->id);
 
 	return 0;
+
+err_put_group:
+	mutex_lock(&group->mutex);
+	list_del(&device->list);
+	mutex_unlock(&group->mutex);
+	dev->iommu_group = NULL;
+	kobject_put(group->devices_kobj);
+err_free_name:
+	kfree(device->name);
+err_remove_link:
+	sysfs_remove_link(&dev->kobj, "iommu_group");
+err_free_device:
+	kfree(device);
+	pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), group->id, ret);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
 
-- 
2.10.2.dirty

Re: [PATCH] iommu: Handle default domain attach failure

[Punit Agrawal]

Hi Robin,

Thanks for the patch.

Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> writes:

> We wouldn't normally expect ops->attach_dev() to fail, but on IOMMUs
> with limited hardware resources, or generally misconfigured systems,
> it is certainly possible. We report failure correctly from the external
> iommu_attach_device() interface, but do not do so in iommu_group_add()
> when attaching to the default domain. The result of failure there is
> that the device, group and domain all get left in a broken,
> part-configured state which leads to weird errors and misbehaviour down
> the line when IOMMU API calls sort-of-but-don't-quite work.
>
> Check the return value of __iommu_attach_device() on the default domain,
> and refactor the error handling paths to cope with its failure and clean
> up correctly in such cases.
>

Booting with the patch, the boot time warnings I was seeing go away. I
also now see a bunch of messages when adding devices, such as -

[    4.805466] iommu: Failed to add device 0000:06:10.2 to group 10: -28


> Fixes: e39cb8a3aa98 ("iommu: Make sure a device is always attached to a domain")
> Reported-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>

FWIW,

Tested-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>

> ---
>  drivers/iommu/iommu.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index dbe7f653bb7c..aed906a3e3db 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -383,36 +383,30 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>  	device->dev = dev;
>  
>  	ret = sysfs_create_link(&dev->kobj, &group->kobj, "iommu_group");
> -	if (ret) {
> -		kfree(device);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_free_device;
>  
>  	device->name = kasprintf(GFP_KERNEL, "%s", kobject_name(&dev->kobj));
>  rename:
>  	if (!device->name) {
> -		sysfs_remove_link(&dev->kobj, "iommu_group");
> -		kfree(device);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_remove_link;
>  	}
>  
>  	ret = sysfs_create_link_nowarn(group->devices_kobj,
>  				       &dev->kobj, device->name);
>  	if (ret) {
> -		kfree(device->name);
>  		if (ret == -EEXIST && i >= 0) {
>  			/*
>  			 * Account for the slim chance of collision
>  			 * and append an instance to the name.
>  			 */
> +			kfree(device->name);
>  			device->name = kasprintf(GFP_KERNEL, "%s.%d",
>  						 kobject_name(&dev->kobj), i++);
>  			goto rename;
>  		}
> -
> -		sysfs_remove_link(&dev->kobj, "iommu_group");
> -		kfree(device);
> -		return ret;
> +		goto err_free_name;
>  	}
>  
>  	kobject_get(group->devices_kobj);
> @@ -424,8 +418,10 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>  	mutex_lock(&group->mutex);
>  	list_add_tail(&device->list, &group->devices);
>  	if (group->domain)
> -		__iommu_attach_device(group->domain, dev);
> +		ret = __iommu_attach_device(group->domain, dev);
>  	mutex_unlock(&group->mutex);
> +	if (ret)
> +		goto err_put_group;
>  
>  	/* Notify any listeners about change to group. */
>  	blocking_notifier_call_chain(&group->notifier,
> @@ -436,6 +432,21 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>  	pr_info("Adding device %s to group %d\n", dev_name(dev), group->id);
>  
>  	return 0;
> +
> +err_put_group:
> +	mutex_lock(&group->mutex);
> +	list_del(&device->list);
> +	mutex_unlock(&group->mutex);
> +	dev->iommu_group = NULL;
> +	kobject_put(group->devices_kobj);
> +err_free_name:
> +	kfree(device->name);
> +err_remove_link:
> +	sysfs_remove_link(&dev->kobj, "iommu_group");
> +err_free_device:
> +	kfree(device);
> +	pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), group->id, ret);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_add_device);

Re: [PATCH] iommu: Handle default domain attach failure

[Joerg Roedel]

On Mon, Jan 16, 2017 at 12:58:07PM +0000, Robin Murphy wrote:
> We wouldn't normally expect ops->attach_dev() to fail, but on IOMMUs
> with limited hardware resources, or generally misconfigured systems,
> it is certainly possible. We report failure correctly from the external
> iommu_attach_device() interface, but do not do so in iommu_group_add()
> when attaching to the default domain. The result of failure there is
> that the device, group and domain all get left in a broken,
> part-configured state which leads to weird errors and misbehaviour down
> the line when IOMMU API calls sort-of-but-don't-quite work.
> 
> Check the return value of __iommu_attach_device() on the default domain,
> and refactor the error handling paths to cope with its failure and clean
> up correctly in such cases.
> 
> Fixes: e39cb8a3aa98 ("iommu: Make sure a device is always attached to a domain")
> Reported-by: Punit Agrawal <punit.agrawal-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)

Nice cleanup along the fix! Applied, thanks.