iommu: Handle default domain attach failure

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@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Joerg Roedel <jroedel@suse.de>

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dbe7f653bb7cb6d2db0b9ecf9d8586dc719cb808..aed906a3e3db7ca7c452b3bbe584d6f6c1e30767 100644 (file)
--- 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 @@ rename:
        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 @@ rename:
        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);