On 2022-05-04 08:53, Jan Stancek wrote:
[...]
> Hi,
>
> I'm getting panics after hunk above was applied in this patch
> on ppc64le KVM guest, dev->iommu is NULL.
Oof, this can probably be hit with vfio-noiommu too, and by the look of
things, `echo auto > /sys/kernel/iommu_groups/0/type` would likely blow
up as well. Does the patch below work for you?
Thanks,
Robin.
----->8-----
From abf0a38563bb2922a849e235d33d342170b5bc90 Mon Sep 17 00:00:00 2001
Message-Id: <abf0a38563bb2922a849e235d33d342170b5bc90.1651662442.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Wed, 4 May 2022 11:53:20 +0100
Subject: [PATCH] iommu: Make sysfs robust for non-API groups
Groups created by VFIO backends outside the core IOMMU API should never
be passed directly into the API itself, however they still expose their
standard sysfs attributes, so we can still stumble across them that way.
Take care to consider those cases before jumping into our normal
assumptions of a fully-initialised core API group.
Fixes: 3f6634d997db ("iommu: Use right way to retrieve iommu_ops")
Reported-by: Jan Stancek <[email protected]>
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/iommu.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 29906bc16371..41ea2deaee03 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -510,6 +510,13 @@ int iommu_get_group_resv_regions(struct iommu_group *group,
list_for_each_entry(device, &group->devices, list) {
struct list_head dev_resv_regions;
+ /*
+ * Non-API groups still expose reserved_regions in sysfs,
+ * so filter out calls that get here that way.
+ */
+ if (!device->dev->iommu)
+ break;
+
INIT_LIST_HEAD(&dev_resv_regions);
iommu_get_resv_regions(device->dev, &dev_resv_regions);
ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
@@ -2977,7 +2984,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
return -EACCES;
- if (WARN_ON(!group))
+ if (WARN_ON(!group) || !group->default_domain)
return -EINVAL;
if (sysfs_streq(buf, "identity"))
--
2.35.3.dirty
On Wed, May 04, 2022 at 12:14:07PM +0100, Robin Murphy wrote:
> On 2022-05-04 08:53, Jan Stancek wrote:
> [...]
> > Hi,
> >
> > I'm getting panics after hunk above was applied in this patch
> > on ppc64le KVM guest, dev->iommu is NULL.
>
> Oof, this can probably be hit with vfio-noiommu too, and by the look of
> things, `echo auto > /sys/kernel/iommu_groups/0/type` would likely blow
> up as well. Does the patch below work for you?
>
> Thanks,
> Robin.
>
> ----->8-----
> From abf0a38563bb2922a849e235d33d342170b5bc90 Mon Sep 17 00:00:00 2001
> Message-Id: <abf0a38563bb2922a849e235d33d342170b5bc90.1651662442.git.robin.murphy@arm.com>
> From: Robin Murphy <[email protected]>
> Date: Wed, 4 May 2022 11:53:20 +0100
> Subject: [PATCH] iommu: Make sysfs robust for non-API groups
>
> Groups created by VFIO backends outside the core IOMMU API should never
> be passed directly into the API itself, however they still expose their
> standard sysfs attributes, so we can still stumble across them that way.
> Take care to consider those cases before jumping into our normal
> assumptions of a fully-initialised core API group.
>
> Fixes: 3f6634d997db ("iommu: Use right way to retrieve iommu_ops")
> Reported-by: Jan Stancek <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/iommu.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
Reviewed-by: Jason Gunthorpe <[email protected]>
Jason
Hi Robin,
On Wed, May 04, 2022 at 12:14:07PM +0100, Robin Murphy wrote:
> Oof, this can probably be hit with vfio-noiommu too, and by the look of
> things, `echo auto > /sys/kernel/iommu_groups/0/type` would likely blow
> up as well. Does the patch below work for you?
Thanks for the quick patch! I am delaying to send iommu-fixes upstream
for now in the hope the fix can make it into the branch asap. Please
send it out as a separate patch as soon as it is successfully tested.
Regards,
Joerg
On Wed, May 4, 2022 at 1:14 PM Robin Murphy <[email protected]> wrote:
>
> On 2022-05-04 08:53, Jan Stancek wrote:
> [...]
> > Hi,
> >
> > I'm getting panics after hunk above was applied in this patch
> > on ppc64le KVM guest, dev->iommu is NULL.
>
> Oof, this can probably be hit with vfio-noiommu too, and by the look of
> things, `echo auto > /sys/kernel/iommu_groups/0/type` would likely blow
> up as well. Does the patch below work for you?
Thanks for quick reply. Yes, it does.
# cat /sys/kernel/iommu_groups/0/reserved_regions
# echo auto > /sys/kernel/iommu_groups/0/type
-bash: echo: write error: Invalid argument
Tested-by: Jan Stancek <[email protected]>
>
> Thanks,
> Robin.
>
> ----->8-----
> From abf0a38563bb2922a849e235d33d342170b5bc90 Mon Sep 17 00:00:00 2001
> Message-Id: <abf0a38563bb2922a849e235d33d342170b5bc90.1651662442.git.robin.murphy@arm.com>
> From: Robin Murphy <[email protected]>
> Date: Wed, 4 May 2022 11:53:20 +0100
> Subject: [PATCH] iommu: Make sysfs robust for non-API groups
>
> Groups created by VFIO backends outside the core IOMMU API should never
> be passed directly into the API itself, however they still expose their
> standard sysfs attributes, so we can still stumble across them that way.
> Take care to consider those cases before jumping into our normal
> assumptions of a fully-initialised core API group.
>
> Fixes: 3f6634d997db ("iommu: Use right way to retrieve iommu_ops")
> Reported-by: Jan Stancek <[email protected]>
> Signed-off-by: Robin Murphy <[email protected]>
> ---
> drivers/iommu/iommu.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 29906bc16371..41ea2deaee03 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -510,6 +510,13 @@ int iommu_get_group_resv_regions(struct iommu_group *group,
> list_for_each_entry(device, &group->devices, list) {
> struct list_head dev_resv_regions;
>
> + /*
> + * Non-API groups still expose reserved_regions in sysfs,
> + * so filter out calls that get here that way.
> + */
> + if (!device->dev->iommu)
> + break;
> +
> INIT_LIST_HEAD(&dev_resv_regions);
> iommu_get_resv_regions(device->dev, &dev_resv_regions);
> ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
> @@ -2977,7 +2984,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
> if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> return -EACCES;
>
> - if (WARN_ON(!group))
> + if (WARN_ON(!group) || !group->default_domain)
> return -EINVAL;
>
> if (sysfs_streq(buf, "identity"))
> --
> 2.35.3.dirty
>