[PATCH v2 1/5] iommu: Allow taking a reference on a group directly

[Robin Murphy]

iommu_group_get_for_dev() expects that the IOMMU driver's device_group
callback return a group with a reference held for the given device.
Whilst allocating a new group is fine, and pci_device_group() correctly
handles reusing an existing group, there is no general means for IOMMU
drivers doing their own group lookup to take additional references on an
existing group pointer without having to also store device pointers or
resort to elaborate trickery.

Add an IOMMU-driver-specific function to fill the hole.

Acked-by: Sricharan R <***@codeaurora.org>
Signed-off-by: Robin Murphy <***@arm.com>
---

v2: Fix the function name; clarify what exactly its callers are fixing.

drivers/iommu/iommu.c | 13 +++++++++++++
include/linux/iommu.h | 1 +
2 files changed, 14 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9a2f1960873b..9408c3145483 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -552,6 +552,19 @@ struct iommu_group *iommu_group_get(struct device *dev)
EXPORT_SYMBOL_GPL(iommu_group_get);

/**
+ * iommu_group_ref_get - Increment reference on a group
+ * @group: the group to use, must not be NULL
+ *
+ * This function is called by iommu drivers to take additional references on an
+ * existing group. Returns the given group for convenience.
+ */
+struct iommu_group *iommu_group_ref_get(struct iommu_group *group)
+{
+ kobject_get(group->devices_kobj);
+ return group;
+}
+
+/**
* iommu_group_put - Decrement group reference
* @group: the group to use
*
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21318af..431638110c6a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -253,6 +253,7 @@ extern void iommu_group_remove_device(struct device *dev);
extern int iommu_group_for_each_dev(struct iommu_group *group, void *data,
int (*fn)(struct device *, void *));
extern struct iommu_group *iommu_group_get(struct device *dev);
+extern struct iommu_group *iommu_group_ref_get(struct iommu_group *group);
extern void iommu_group_put(struct iommu_group *group);
extern int iommu_group_register_notifier(struct iommu_group *group,
struct notifier_block *nb);

[Robin Murphy]

When arm_smmu_device_group() finds an existing group due to Stream ID
aliasing, it should be taking an additional reference on that group.
Otherwise, the caller of iommu_group_get_for_dev() will inadvertently
remove the reference taken by iommu_group_add_device(), and the group
will be freed prematurely if any device is removed.

Reported-by: Sricharan R <***@codeaurora.org>
Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/arm-smmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 8f7281444551..b8cd5579a953 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1478,7 +1478,7 @@ static struct iommu_group *arm_smmu_device_group(struct device *dev)
}

if (group)
- return group;
+ return iommu_group_ref_get(group);

if (dev_is_pci(dev))
group = pci_device_group(dev);

[Robin Murphy]

If acpihid_device_group() finds an existing group for the relevant
devid, it should be taking an additional reference on that group.
Otherwise, the caller of iommu_group_get_for_dev() will inadvertently
remove the reference taken by iommu_group_add_device(), and the group
will be freed prematurely if any device is removed.

Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/amd_iommu.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 754595ee11b6..019e02707cd5 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -373,6 +373,8 @@ static struct iommu_group *acpihid_device_group(struct device *dev)

if (!entry->group)
entry->group = generic_device_group(dev);
+ else
+ iommu_group_ref_get(entry->group);

return entry->group;
}

[Robin Murphy]

For each subsequent device assigned to the m4u_group after its initial
allocation, we need to take an additional reference. Otherwise, the
caller of iommu_group_get_for_dev() will inadvertently remove the
reference taken by iommu_group_add_device(), and the group will be
freed prematurely if any device is removed.

Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/mtk_iommu.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index b12c12d74c33..9799daeaacde 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -410,6 +410,8 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev)
data->m4u_group = iommu_group_alloc();
if (IS_ERR(data->m4u_group))
dev_err(dev, "Failed to allocate M4U IOMMU group\n");
+ } else {
+ iommu_group_ref_get(data->m4u_group);
}
return data->m4u_group;
}

[Robin Murphy]

For each subsequent device assigned to the m4u_group after its initial
allocation, we need to take an additional reference. Otherwise, the
caller of iommu_group_get_for_dev() will inadvertently remove the
reference taken by iommu_group_add_device(), and the group will be
freed prematurely if any device is removed.

Signed-off-by: Robin Murphy <***@arm.com>
---
drivers/iommu/mtk_iommu_v1.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index b8aeb0768483..c7063e9d67d8 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -502,6 +502,8 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev)
data->m4u_group = iommu_group_alloc();
if (IS_ERR(data->m4u_group))
dev_err(dev, "Failed to allocate M4U IOMMU group\n");
+ } else {
+ iommu_group_ref_get(data->m4u_group);
}
return data->m4u_group;
}

[Joerg Roedel]

Post by Robin Murphy
iommu_group_get_for_dev() expects that the IOMMU driver's device_group
callback return a group with a reference held for the given device.
Whilst allocating a new group is fine, and pci_device_group() correctly
handles reusing an existing group, there is no general means for IOMMU
drivers doing their own group lookup to take additional references on an
existing group pointer without having to also store device pointers or
resort to elaborate trickery.
Add an IOMMU-driver-specific function to fill the hole.
---
v2: Fix the function name; clarify what exactly its callers are fixing.
drivers/iommu/iommu.c | 13 +++++++++++++
include/linux/iommu.h | 1 +
2 files changed, 14 insertions(+)

Applied the series, thanks Robin.