Commit a9c362db3920 ("iommu: Validate that devices match domains") added
an owner token to an iommu_domain. This token is checked during domain
attachment to RID or PASID through the generic iommu interfaces.
The sva and nested domains are attached to device or PASID through the
generic iommu interfaces. Therefore, they require the owner token to be
set during allocation. Otherwise, they fail to attach.
Set the owner token for sva and nested domains.
Fixes: a9c362db3920 ("iommu: Validate that devices match domains")
Cc: Robin Murphy <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/nested.c | 1 +
drivers/iommu/iommu.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index b5a5563ab32c..014d4a4e7586 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -108,6 +108,7 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
domain->s1_cfg = vtd;
domain->domain.ops = &intel_nested_domain_ops;
domain->domain.type = IOMMU_DOMAIN_NESTED;
+ domain->domain.owner = &intel_iommu_ops;
INIT_LIST_HEAD(&domain->devices);
INIT_LIST_HEAD(&domain->dev_pasids);
spin_lock_init(&domain->lock);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0d25468d53a6..d0a28667479a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3617,6 +3617,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
domain->type = IOMMU_DOMAIN_SVA;
mmgrab(mm);
domain->mm = mm;
+ domain->owner = ops;
domain->iopf_handler = iommu_sva_handle_iopf;
domain->fault_data = mm;
--
2.34.1
On 2023-12-07 2:19 am, Lu Baolu wrote:
> Commit a9c362db3920 ("iommu: Validate that devices match domains") added
> an owner token to an iommu_domain. This token is checked during domain
> attachment to RID or PASID through the generic iommu interfaces.
>
> The sva and nested domains are attached to device or PASID through the
> generic iommu interfaces. Therefore, they require the owner token to be
> set during allocation. Otherwise, they fail to attach.
Oops, I missed that iommu_sva_domain_alloc() is a thing - when did we
get such a confusing proliferation of domain allocation paths? Sigh...
I think we should set the owner generically there, since presumably it's
being missed for SMMUv3/AMD/etc. SVA domains as well. Nested domains are
supposed to be OK since both ->domain_alloc_user callsites are covered,
or is there some other sneaky path I've also missed?
Thanks,
Robin.
> Set the owner token for sva and nested domains.
>
> Fixes: a9c362db3920 ("iommu: Validate that devices match domains")
> Cc: Robin Murphy <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel/nested.c | 1 +
> drivers/iommu/iommu.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> index b5a5563ab32c..014d4a4e7586 100644
> --- a/drivers/iommu/intel/nested.c
> +++ b/drivers/iommu/intel/nested.c
> @@ -108,6 +108,7 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
> domain->s1_cfg = vtd;
> domain->domain.ops = &intel_nested_domain_ops;
> domain->domain.type = IOMMU_DOMAIN_NESTED;
> + domain->domain.owner = &intel_iommu_ops;
> INIT_LIST_HEAD(&domain->devices);
> INIT_LIST_HEAD(&domain->dev_pasids);
> spin_lock_init(&domain->lock);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 0d25468d53a6..d0a28667479a 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3617,6 +3617,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
> domain->type = IOMMU_DOMAIN_SVA;
> mmgrab(mm);
> domain->mm = mm;
> + domain->owner = ops;
> domain->iopf_handler = iommu_sva_handle_iopf;
> domain->fault_data = mm;
>
On Thu, Dec 07, 2023 at 09:56:10AM +0000, Robin Murphy wrote:
> On 2023-12-07 2:19 am, Lu Baolu wrote:
> > Commit a9c362db3920 ("iommu: Validate that devices match domains") added
> > an owner token to an iommu_domain. This token is checked during domain
> > attachment to RID or PASID through the generic iommu interfaces.
> >
> > The sva and nested domains are attached to device or PASID through the
> > generic iommu interfaces. Therefore, they require the owner token to be
> > set during allocation. Otherwise, they fail to attach.
>
> Oops, I missed that iommu_sva_domain_alloc() is a thing - when did we get
> such a confusing proliferation of domain allocation paths? Sigh...
We have alot of different kinds of domains now, APIs that are giant
multiplexers are not good.
What I've been wanting to do for a while is to have the drivers call a
helper to allocate their domain struct and the helper would initialize
the common iommu_domain instead of doing this after the op
returns. This is more typical kernel pattern and avoids some of the
confusion about when struct members are valid or not (notice some of
driver code needs iommu_domain stuff set earlier and we confusingly
initialize things twice :()
> I think we should set the owner generically there, since presumably it's
> being missed for SMMUv3/AMD/etc. SVA domains as well. Nested domains are
> supposed to be OK since both ->domain_alloc_user callsites are covered, or
> is there some other sneaky path I've also missed?
Indeed, I also think the first hunk is not needed, the second hunk was
missed.
Jason
On 12/7/23 9:36 PM, Jason Gunthorpe wrote:
> On Thu, Dec 07, 2023 at 09:56:10AM +0000, Robin Murphy wrote:
>> On 2023-12-07 2:19 am, Lu Baolu wrote:
>>> Commit a9c362db3920 ("iommu: Validate that devices match domains") added
>>> an owner token to an iommu_domain. This token is checked during domain
>>> attachment to RID or PASID through the generic iommu interfaces.
>>>
>>> The sva and nested domains are attached to device or PASID through the
>>> generic iommu interfaces. Therefore, they require the owner token to be
>>> set during allocation. Otherwise, they fail to attach.
>> Oops, I missed that iommu_sva_domain_alloc() is a thing - when did we get
>> such a confusing proliferation of domain allocation paths? Sigh...
> We have alot of different kinds of domains now, APIs that are giant
> multiplexers are not good.
>
> What I've been wanting to do for a while is to have the drivers call a
> helper to allocate their domain struct and the helper would initialize
> the common iommu_domain instead of doing this after the op
> returns. This is more typical kernel pattern and avoids some of the
> confusion about when struct members are valid or not (notice some of
> driver code needs iommu_domain stuff set earlier and we confusingly
> initialize things twice :()
>
>> I think we should set the owner generically there, since presumably it's
>> being missed for SMMUv3/AMD/etc. SVA domains as well. Nested domains are
>> supposed to be OK since both ->domain_alloc_user callsites are covered, or
>> is there some other sneaky path I've also missed?
> Indeed, I also think the first hunk is not needed, the second hunk was
> missed.
Oh, yes! I overlooked that iommufd has already done that for nested
domain. I will update it with a v2.
Best regards,
baolu