* [PATCH iommu/next] iommu: Fix default domain setup
@ 2023-06-14 5:56 Vasant Hegde
2023-06-14 6:27 ` Baolu Lu
0 siblings, 1 reply; 4+ messages in thread
From: Vasant Hegde @ 2023-06-14 5:56 UTC (permalink / raw)
To: iommu, joro; +Cc: suravee.suthikulpanit, Vasant Hegde, Dheeraj Kumar Srivastava
Commit 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
accidently restored "group->domain" to "old_domain" while keeping
"group->default_domain" to new domain. Also freed new domain.
This works fine during boot as 'old_domain' is NULL. But if we try
change domain via sysfs using below command then kernel crashes with
"kernel NULL pointer dereference".
Change domain command :
- Assumed we have single device in the IOMMU group
- unbind device driver
- echo "<dom type>" > /sys/kernel/iommu_groups/<group id>/type
Fix above described issue by handling error path properly.
Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
Fixes: 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
Hi Joerg,
This patch is on top of iommu/next branch.
-Vasant
drivers/iommu/iommu.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9e0228ef612b..d21d0a217bdf 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2880,7 +2880,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
}
/* We must set default_domain early for __iommu_device_set_domain */
- group->default_domain = dom;
if (!group->domain) {
/*
* Drivers are not allowed to fail the first domain attach.
@@ -2896,7 +2895,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
ret = __iommu_group_set_domain(group, dom);
if (ret) {
iommu_domain_free(dom);
- group->default_domain = old_dom;
return ret;
}
}
@@ -2915,16 +2913,20 @@ static int iommu_setup_default_domain(struct iommu_group *group,
}
}
+out_free:
+ group->default_domain = dom;
+
+ if (old_dom) {
+ iommu_domain_free(old_dom);
+ old_dom = NULL;
+ }
+
err_restore:
if (old_dom) {
__iommu_group_set_domain_internal(
group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
iommu_domain_free(dom);
- old_dom = NULL;
}
-out_free:
- if (old_dom)
- iommu_domain_free(old_dom);
return ret;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH iommu/next] iommu: Fix default domain setup
2023-06-14 5:56 [PATCH iommu/next] iommu: Fix default domain setup Vasant Hegde
@ 2023-06-14 6:27 ` Baolu Lu
2023-06-19 6:49 ` Vasant Hegde
0 siblings, 1 reply; 4+ messages in thread
From: Baolu Lu @ 2023-06-14 6:27 UTC (permalink / raw)
To: Vasant Hegde, iommu, joro
Cc: baolu.lu, suravee.suthikulpanit, Dheeraj Kumar Srivastava
On 6/14/23 1:56 PM, Vasant Hegde wrote:
> Commit 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
> accidently restored "group->domain" to "old_domain" while keeping
> "group->default_domain" to new domain. Also freed new domain.
>
> This works fine during boot as 'old_domain' is NULL. But if we try
> change domain via sysfs using below command then kernel crashes with
> "kernel NULL pointer dereference".
>
> Change domain command :
> - Assumed we have single device in the IOMMU group
Nit: The requirement for singleton group has been removed.
> - unbind device driver
> - echo "<dom type>" > /sys/kernel/iommu_groups/<group id>/type
>
> Fix above described issue by handling error path properly.
>
> Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Fixes: 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
> Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
> Hi Joerg,
> This patch is on top of iommu/next branch.
>
> -Vasant
>
> drivers/iommu/iommu.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9e0228ef612b..d21d0a217bdf 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2880,7 +2880,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
> }
>
> /* We must set default_domain early for __iommu_device_set_domain */
Is the above comment still relevant? If yes, this fix seems to be
problematic. Or no?
> - group->default_domain = dom;
> if (!group->domain) {
> /*
> * Drivers are not allowed to fail the first domain attach.
> @@ -2896,7 +2895,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
> ret = __iommu_group_set_domain(group, dom);
> if (ret) {
> iommu_domain_free(dom);
> - group->default_domain = old_dom;
> return ret;
> }
> }
> @@ -2915,16 +2913,20 @@ static int iommu_setup_default_domain(struct iommu_group *group,
> }
> }
>
> +out_free:
> + group->default_domain = dom;
> +
> + if (old_dom) {
> + iommu_domain_free(old_dom);
> + old_dom = NULL;
> + }
> +
> err_restore:
> if (old_dom) {
> __iommu_group_set_domain_internal(
> group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
> iommu_domain_free(dom);
> - old_dom = NULL;
> }
> -out_free:
> - if (old_dom)
> - iommu_domain_free(old_dom);
> return ret;
> }
>
It's better to split the error path from the successful one.
Best regards,
baolu
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH iommu/next] iommu: Fix default domain setup
2023-06-14 6:27 ` Baolu Lu
@ 2023-06-19 6:49 ` Vasant Hegde
2023-06-20 5:12 ` Baolu Lu
0 siblings, 1 reply; 4+ messages in thread
From: Vasant Hegde @ 2023-06-19 6:49 UTC (permalink / raw)
To: Baolu Lu, iommu, joro; +Cc: suravee.suthikulpanit, Dheeraj Kumar Srivastava
Hi Baolu,
On 6/14/2023 11:57 AM, Baolu Lu wrote:
> On 6/14/23 1:56 PM, Vasant Hegde wrote:
>> Commit 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
>> accidently restored "group->domain" to "old_domain" while keeping
>> "group->default_domain" to new domain. Also freed new domain.
>>
>> This works fine during boot as 'old_domain' is NULL. But if we try
>> change domain via sysfs using below command then kernel crashes with
>> "kernel NULL pointer dereference".
>>
>> Change domain command :
>> - Assumed we have single device in the IOMMU group
>
> Nit: The requirement for singleton group has been removed.
Yeah. I have seen those changes but forgot while writing commit message. Will
fix it.
>
>> - unbind device driver
>> - echo "<dom type>" > /sys/kernel/iommu_groups/<group id>/type
>>
>> Fix above described issue by handling error path properly.
>>
>> Reported-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
>> Fixes: 1000dccd5d13 ("iommu: Allow IOMMU_RESV_DIRECT to work on ARM")
>> Tested-by: Dheeraj Kumar Srivastava <dheerajkumar.srivastava@amd.com>
>> Reviewed-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> Hi Joerg,
>> This patch is on top of iommu/next branch.
>>
>> -Vasant
>>
>> drivers/iommu/iommu.c | 14 ++++++++------
>> 1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 9e0228ef612b..d21d0a217bdf 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2880,7 +2880,6 @@ static int iommu_setup_default_domain(struct iommu_group
>> *group,
>> }
>> /* We must set default_domain early for __iommu_device_set_domain */
>
> Is the above comment still relevant? If yes, this fix seems to be
> problematic. Or no?
I think we should keep below change.
>
>> - group->default_domain = dom;
>> if (!group->domain) {
>> /*
>> * Drivers are not allowed to fail the first domain attach.
>> @@ -2896,7 +2895,6 @@ static int iommu_setup_default_domain(struct iommu_group
>> *group,
>> ret = __iommu_group_set_domain(group, dom);
>> if (ret) {
>> iommu_domain_free(dom);
>> - group->default_domain = old_dom;
>> return ret;
>> }
>> }
>> @@ -2915,16 +2913,20 @@ static int iommu_setup_default_domain(struct
>> iommu_group *group,
>> }
>> }
>> +out_free:
>> + group->default_domain = dom;
>> +
>> + if (old_dom) {
>> + iommu_domain_free(old_dom);
>> + old_dom = NULL;
>> + }
>> +
>> err_restore:
>> if (old_dom) {
>> __iommu_group_set_domain_internal(
>> group, old_dom, IOMMU_SET_DOMAIN_MUST_SUCCEED);
>> iommu_domain_free(dom);
>> - old_dom = NULL;
>> }
>> -out_free:
>> - if (old_dom)
>> - iommu_domain_free(old_dom);
>> return ret;
>> }
>>
>
> It's better to split the error path from the successful one.
Sure. Will fix it.
Thanks
-Vasant
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH iommu/next] iommu: Fix default domain setup
2023-06-19 6:49 ` Vasant Hegde
@ 2023-06-20 5:12 ` Baolu Lu
0 siblings, 0 replies; 4+ messages in thread
From: Baolu Lu @ 2023-06-20 5:12 UTC (permalink / raw)
To: Vasant Hegde, iommu, joro
Cc: baolu.lu, suravee.suthikulpanit, Dheeraj Kumar Srivastava
On 6/19/23 2:49 PM, Vasant Hegde wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 9e0228ef612b..d21d0a217bdf 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -2880,7 +2880,6 @@ static int iommu_setup_default_domain(struct iommu_group
>>> *group,
>>> }
>>> /* We must set default_domain early for __iommu_device_set_domain */
>> Is the above comment still relevant? If yes, this fix seems to be
>> problematic. Or no?
> I think we should keep below change.
If the change is necessary then we should remove above comment with
sufficient justification.
Best regards,
baolu
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-20 5:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-14 5:56 [PATCH iommu/next] iommu: Fix default domain setup Vasant Hegde
2023-06-14 6:27 ` Baolu Lu
2023-06-19 6:49 ` Vasant Hegde
2023-06-20 5:12 ` Baolu Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).