pci_device_group() can return an already existing IOMMU group if the PCI
device's pagetables have to be shared with another one due to bus
toplogy, isolation features and/or DMA alias quirks.
apple_dart_device_group() however assumes that the group has just been
created and overwrites its iommudata which will eventually lead to
apple_dart_release_group leaving stale entries in sid2group.
Fix that by merging the iommudata if the returned group already exists.
Fixes: f0b636804c7c ("iommu/dart: Clear sid2group entry when a group is freed")
Signed-off-by: Sven Peter <[email protected]>
---
drivers/iommu/apple-dart.c | 51 ++++++++++++++++++++++++++++++++------
1 file changed, 44 insertions(+), 7 deletions(-)
This won't apply cleanly to iommu/fixes as it's based on the t8110 DART
changes since the USB4/Thunderbolt DART itself also depends on those.
That's not a big deal though since it's not possible to run into this
bug without complex PCI bus topologies which can only be created using
USB4/Thunderbolt on these SoCs and there's no support for that upstream
yet.
diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index a1304ba3639b..02f7a1740b14 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -840,6 +840,29 @@ static void apple_dart_release_group(void *iommu_data)
mutex_unlock(&apple_dart_groups_lock);
}
+static int apple_dart_merge_master_cfg(struct apple_dart_master_cfg *dst,
+ struct apple_dart_master_cfg *src)
+{
+ /*
+ * We know that this function is only called for groups returned from
+ * pci_device_group and that all Apple Silicon platforms never spread
+ * PCIe devices from the same bus across multiple DARTs such that we can
+ * just assume that both src and dst only have the same single DART.
+ */
+ if (src->stream_maps[1].dart)
+ return -EINVAL;
+ if (dst->stream_maps[1].dart)
+ return -EINVAL;
+ if (src->stream_maps[0].dart != dst->stream_maps[0].dart)
+ return -EINVAL;
+
+ bitmap_or(dst->stream_maps[0].sidmap,
+ dst->stream_maps[0].sidmap,
+ src->stream_maps[0].sidmap,
+ dst->stream_maps[0].dart->num_streams);
+ return 0;
+}
+
static struct iommu_group *apple_dart_device_group(struct device *dev)
{
int i, sid;
@@ -881,14 +904,28 @@ static struct iommu_group *apple_dart_device_group(struct device *dev)
if (!group)
goto out;
- group_master_cfg = kmemdup(cfg, sizeof(*group_master_cfg), GFP_KERNEL);
- if (!group_master_cfg) {
- iommu_group_put(group);
- goto out;
- }
+ group_master_cfg = iommu_group_get_iommudata(group);
+ if (group_master_cfg) {
+ int ret;
- iommu_group_set_iommudata(group, group_master_cfg,
- apple_dart_release_group);
+ ret = apple_dart_merge_master_cfg(group_master_cfg, cfg);
+ if (ret) {
+ dev_err(dev, "Failed to merge DART IOMMU grups.\n");
+ iommu_group_put(group);
+ res = ERR_PTR(ret);
+ goto out;
+ }
+ } else {
+ group_master_cfg = kmemdup(cfg, sizeof(*group_master_cfg),
+ GFP_KERNEL);
+ if (!group_master_cfg) {
+ iommu_group_put(group);
+ goto out;
+ }
+
+ iommu_group_set_iommudata(group, group_master_cfg,
+ apple_dart_release_group);
+ }
for_each_stream_map(i, cfg, stream_map)
for_each_set_bit(sid, stream_map->sidmap, stream_map->dart->num_streams)
--
2.25.1
On Sat, 28 Jan 2023 at 11:43, Sven Peter <[email protected]> wrote:
>
> pci_device_group() can return an already existing IOMMU group if the PCI
> device's pagetables have to be shared with another one due to bus
> toplogy, isolation features and/or DMA alias quirks.
> apple_dart_device_group() however assumes that the group has just been
> created and overwrites its iommudata which will eventually lead to
> apple_dart_release_group leaving stale entries in sid2group.
> Fix that by merging the iommudata if the returned group already exists.
>
> Fixes: f0b636804c7c ("iommu/dart: Clear sid2group entry when a group is freed")
> Signed-off-by: Sven Peter <[email protected]>
> ---
Reviewed-by: Eric Curtin <[email protected]>
Is mise le meas/Regards,
Eric Curtin
> drivers/iommu/apple-dart.c | 51 ++++++++++++++++++++++++++++++++------
> 1 file changed, 44 insertions(+), 7 deletions(-)
>
> This won't apply cleanly to iommu/fixes as it's based on the t8110 DART
> changes since the USB4/Thunderbolt DART itself also depends on those.
> That's not a big deal though since it's not possible to run into this
> bug without complex PCI bus topologies which can only be created using
> USB4/Thunderbolt on these SoCs and there's no support for that upstream
> yet.
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index a1304ba3639b..02f7a1740b14 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -840,6 +840,29 @@ static void apple_dart_release_group(void *iommu_data)
> mutex_unlock(&apple_dart_groups_lock);
> }
>
> +static int apple_dart_merge_master_cfg(struct apple_dart_master_cfg *dst,
> + struct apple_dart_master_cfg *src)
> +{
> + /*
> + * We know that this function is only called for groups returned from
> + * pci_device_group and that all Apple Silicon platforms never spread
> + * PCIe devices from the same bus across multiple DARTs such that we can
> + * just assume that both src and dst only have the same single DART.
> + */
> + if (src->stream_maps[1].dart)
> + return -EINVAL;
> + if (dst->stream_maps[1].dart)
> + return -EINVAL;
> + if (src->stream_maps[0].dart != dst->stream_maps[0].dart)
> + return -EINVAL;
> +
> + bitmap_or(dst->stream_maps[0].sidmap,
> + dst->stream_maps[0].sidmap,
> + src->stream_maps[0].sidmap,
> + dst->stream_maps[0].dart->num_streams);
> + return 0;
> +}
> +
> static struct iommu_group *apple_dart_device_group(struct device *dev)
> {
> int i, sid;
> @@ -881,14 +904,28 @@ static struct iommu_group *apple_dart_device_group(struct device *dev)
> if (!group)
> goto out;
>
> - group_master_cfg = kmemdup(cfg, sizeof(*group_master_cfg), GFP_KERNEL);
> - if (!group_master_cfg) {
> - iommu_group_put(group);
> - goto out;
> - }
> + group_master_cfg = iommu_group_get_iommudata(group);
> + if (group_master_cfg) {
> + int ret;
>
> - iommu_group_set_iommudata(group, group_master_cfg,
> - apple_dart_release_group);
> + ret = apple_dart_merge_master_cfg(group_master_cfg, cfg);
> + if (ret) {
> + dev_err(dev, "Failed to merge DART IOMMU grups.\n");
> + iommu_group_put(group);
> + res = ERR_PTR(ret);
> + goto out;
> + }
> + } else {
> + group_master_cfg = kmemdup(cfg, sizeof(*group_master_cfg),
> + GFP_KERNEL);
> + if (!group_master_cfg) {
> + iommu_group_put(group);
> + goto out;
> + }
> +
> + iommu_group_set_iommudata(group, group_master_cfg,
> + apple_dart_release_group);
> + }
>
> for_each_stream_map(i, cfg, stream_map)
> for_each_set_bit(sid, stream_map->sidmap, stream_map->dart->num_streams)
> --
> 2.25.1
>
>
On Sat, Jan 28, 2023 at 12:35:32PM +0100, Sven Peter wrote:
> pci_device_group() can return an already existing IOMMU group if the PCI
> device's pagetables have to be shared with another one due to bus
> toplogy, isolation features and/or DMA alias quirks.
> apple_dart_device_group() however assumes that the group has just been
> created and overwrites its iommudata which will eventually lead to
> apple_dart_release_group leaving stale entries in sid2group.
> Fix that by merging the iommudata if the returned group already exists.
>
> Fixes: f0b636804c7c ("iommu/dart: Clear sid2group entry when a group is freed")
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/iommu/apple-dart.c | 51 ++++++++++++++++++++++++++++++++------
> 1 file changed, 44 insertions(+), 7 deletions(-)
Applied, thanks.