[PATCH 1/3] iommu: Attach device group to old domain in error path

[PATCH 1/3] iommu: Attach device group to old domain in error path

[Vasant Hegde]

iommu_attach_group() attaches all devices in a group to domain and then
sets group domain (group->domain). Current code (__iommu_attach_group())
does not handle error path. This creates problem as devices to domain
attachment is in inconsistent state.

To recover from this situation, we need force attaching all devices back
to the old domain. This patch introduces `force` parameter to
__iommu_group_set_domain() so that we can attach devices back to old
domain.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/iommu.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index de91dd88705b..e58683cd3bf7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -95,7 +95,8 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group);
 static int __iommu_group_set_domain(struct iommu_group *group,
-				    struct iommu_domain *new_domain);
+				    struct iommu_domain *new_domain,
+				    bool force);
 static int iommu_create_device_direct_mappings(struct iommu_group *group,
 					       struct device *dev);
 static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -1974,7 +1975,7 @@ static void __iommu_group_set_core_domain(struct iommu_group *group)
 	else
 		new_domain = group->default_domain;
 
-	ret = __iommu_group_set_domain(group, new_domain);
+	ret = __iommu_group_set_domain(group, new_domain, false);
 	WARN(ret, "iommu driver failed to attach the default/blocking domain");
 }
 
@@ -2124,8 +2125,16 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 
 	ret = __iommu_group_for_each_dev(group, domain,
 					 iommu_group_do_attach_device);
-	if (ret == 0)
+	if (ret == 0) {
 		group->domain = domain;
+	} else {
+		/*
+		 * To recover from the case when certain device within the
+		 * group fails to attach to the new domain, we need force
+		 * attaching all devices back to the old domain.
+		 */
+		__iommu_group_set_domain(group, group->domain, true);
+	}
 
 	return ret;
 }
@@ -2164,11 +2173,12 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
 }
 
 static int __iommu_group_set_domain(struct iommu_group *group,
-				    struct iommu_domain *new_domain)
+				    struct iommu_domain *new_domain,
+				    bool force)
 {
 	int ret;
 
-	if (group->domain == new_domain)
+	if (!force && group->domain == new_domain)
 		return 0;
 
 	/*
@@ -3135,7 +3145,7 @@ static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner)
 	ret = __iommu_group_alloc_blocking_domain(group);
 	if (ret)
 		return ret;
-	ret = __iommu_group_set_domain(group, group->blocking_domain);
+	ret = __iommu_group_set_domain(group, group->blocking_domain, false);
 	if (ret)
 		return ret;
 
@@ -3222,7 +3232,7 @@ static void __iommu_release_dma_ownership(struct iommu_group *group)
 
 	group->owner_cnt = 0;
 	group->owner = NULL;
-	ret = __iommu_group_set_domain(group, group->default_domain);
+	ret = __iommu_group_set_domain(group, group->default_domain, false);
 	WARN(ret, "iommu driver failed to attach the default domain");
 }
 
-- 
2.31.1

[PATCH 2/3] iommu/amd: Skip attach device domain is same as new domain

[Vasant Hegde]

If device->domain is same as new domain then we can skip the
device attach process.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2f8c018e1ffd..26c96b821fa2 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2161,6 +2161,13 @@ static int amd_iommu_attach_device(struct iommu_domain *dom,
 	struct amd_iommu *iommu = rlookup_amd_iommu(dev);
 	int ret;
 
+	/*
+	 * Skip attach device to domain if new domain is same as
+	 * devices current domain
+	 */
+	if (dev_data->domain == domain)
+		return 0;
+
 	dev_data->defer_attach = false;
 
 	if (dev_data->domain)
-- 
2.31.1

[PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Vasant Hegde]

If IOMMU domain for device group is not setup properly then we may hit
IOMMU page fault. Current page fault handler assumes that domain is
always setup and it will hit NULL pointer dereference (see below sample log).

This is mostly programming error and difficult to recover from here. Hence
call BUG_ON().

Sample log:
----------
Jan 06 02:07:52 kernel: amdgpu 0000:00:01.0: amdgpu: SE 1, SH per SE 1, CU per SH 8, active_cu_number 6
Jan 06 02:07:52 kernel: BUG: kernel NULL pointer dereference, address: 0000000000000058
Jan 06 02:07:52 kernel: #PF: supervisor read access in kernel mode
Jan 06 02:07:53 kernel: #PF: error_code(0x0000) - not-present page
Jan 06 02:07:53 kernel: PGD 0 P4D 0
Jan 06 02:07:53 kernel: Oops: 0000 [#1] PREEMPT SMP NOPTI
Jan 06 02:07:53 kernel: CPU: 2 PID: 56 Comm: irq/24-AMD-Vi Not tainted 6.2.0-rc2+ #89
Jan 06 02:07:53 kernel: Hardware name: xxx
Jan 06 02:07:53 kernel: RIP: 0010:report_iommu_fault+0x11/0x90
Jan 06 02:07:53 kernel: Code: 0f 0b eb cd 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 55 41 54 41 89 cc 55 48 89 d5 53 <48> 8b 47 48 48 89 f3 48 85 c0 74 64 4c 8b 47 50 e8 ea e8 4a 00 41
Jan 06 02:07:53 kernel: RSP: 0018:ffffb9b7803ebe08 EFLAGS: 00010246
Jan 06 02:07:53 kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
Jan 06 02:07:53 kernel: RDX: 00000001522e2500 RSI: ffff940cc09e30d0 RDI: 0000000000000010
Jan 06 02:07:53 kernel: RBP: 00000001522e2500 R08: ffff940cc12c2b00 R09: 0000000000000050
Jan 06 02:07:53 kernel: R10: ffff940cc021e000 R11: 0000000000000000 R12: 0000000000000000
Jan 06 02:07:53 kernel: R13: ffff940cc005b000 R14: 0000000000000008 R15: 0000000000000000
Jan 06 02:07:53 kernel: FS:  0000000000000000(0000) GS:ffff940db7500000(0000) knlGS:0000000000000000
Jan 06 02:07:53 kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Jan 06 02:07:53 kernel: CR2: 0000000000000058 CR3: 000000010230a000 CR4: 00000000001506e0
Jan 06 02:07:53 kernel: Call Trace:
Jan 06 02:07:53 kernel:  <TASK>
Jan 06 02:07:53 kernel:  amd_iommu_int_thread+0x60c/0x760
Jan 06 02:07:53 kernel:  ? __pfx_irq_thread_fn+0x10/0x10
Jan 06 02:07:53 kernel:  irq_thread_fn+0x1f/0x60
Jan 06 02:07:53 kernel:  irq_thread+0xea/0x1a0
Jan 06 02:07:53 kernel:  ? preempt_count_add+0x6a/0xa0
Jan 06 02:07:53 kernel:  ? __pfx_irq_thread_dtor+0x10/0x10
Jan 06 02:07:53 kernel:  ? __pfx_irq_thread+0x10/0x10
Jan 06 02:07:53 kernel:  kthread+0xe9/0x110
Jan 06 02:07:53 kernel:  ? __pfx_kthread+0x10/0x10
Jan 06 02:07:53 kernel:  ret_from_fork+0x2c/0x50
Jan 06 02:07:53 kernel:  </TASK>

Reported-by: Matt Fagnani <matt.fagnani@bell.net>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 26c96b821fa2..2421acba08c7 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -558,6 +558,9 @@ static void amd_iommu_report_page_fault(struct amd_iommu *iommu,
 		 * prevent logging it.
 		 */
 		if (IS_IOMMU_MEM_TRANSACTION(flags)) {
+			/* Device not attached to domain properly */
+			BUG_ON(dev_data->domain == NULL);
+
 			if (!report_iommu_fault(&dev_data->domain->domain,
 						&pdev->dev, address,
 						IS_WRITE_REQUEST(flags) ?
-- 
2.31.1

Re: [PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Robin Murphy]

On 2023-01-13 13:59, Vasant Hegde wrote:
> If IOMMU domain for device group is not setup properly then we may hit
> IOMMU page fault. Current page fault handler assumes that domain is
> always setup and it will hit NULL pointer dereference (see below sample log).
> 
> This is mostly programming error and difficult to recover from here. Hence
> call BUG_ON().

Is it particularly difficult to skip calling report_iommu_fault()? 
Certainly it didn't seem much of a challenge in 00ef8885a945 
("iommu/mediatek: Fix crash on isr after kexec()"), for example.

Otherwise, even disregarding [1], a BUG_ON() for a condition that's 
guaranteed to crash right there anyway doesn't seem particularly 
valuable. The fact that it crashes already pretty conclusively documents 
the fact that it wasn't expected. report_iommu_fault() is not a big 
complex function, with only one possible callsite related to 
amd_iommu_int_thread(), so even with amd_iommu_report_page_fault() 
inlined I'd hope the stacktrace as below is sufficiently obvious even 
without logging an explicit line number.

Thanks,
Robin.

[1] 
https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on

> 
> Sample log:
> ----------
> Jan 06 02:07:52 kernel: amdgpu 0000:00:01.0: amdgpu: SE 1, SH per SE 1, CU per SH 8, active_cu_number 6
> Jan 06 02:07:52 kernel: BUG: kernel NULL pointer dereference, address: 0000000000000058
> Jan 06 02:07:52 kernel: #PF: supervisor read access in kernel mode
> Jan 06 02:07:53 kernel: #PF: error_code(0x0000) - not-present page
> Jan 06 02:07:53 kernel: PGD 0 P4D 0
> Jan 06 02:07:53 kernel: Oops: 0000 [#1] PREEMPT SMP NOPTI
> Jan 06 02:07:53 kernel: CPU: 2 PID: 56 Comm: irq/24-AMD-Vi Not tainted 6.2.0-rc2+ #89
> Jan 06 02:07:53 kernel: Hardware name: xxx
> Jan 06 02:07:53 kernel: RIP: 0010:report_iommu_fault+0x11/0x90
> Jan 06 02:07:53 kernel: Code: 0f 0b eb cd 0f 1f 44 00 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 55 41 54 41 89 cc 55 48 89 d5 53 <48> 8b 47 48 48 89 f3 48 85 c0 74 64 4c 8b 47 50 e8 ea e8 4a 00 41
> Jan 06 02:07:53 kernel: RSP: 0018:ffffb9b7803ebe08 EFLAGS: 00010246
> Jan 06 02:07:53 kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> Jan 06 02:07:53 kernel: RDX: 00000001522e2500 RSI: ffff940cc09e30d0 RDI: 0000000000000010
> Jan 06 02:07:53 kernel: RBP: 00000001522e2500 R08: ffff940cc12c2b00 R09: 0000000000000050
> Jan 06 02:07:53 kernel: R10: ffff940cc021e000 R11: 0000000000000000 R12: 0000000000000000
> Jan 06 02:07:53 kernel: R13: ffff940cc005b000 R14: 0000000000000008 R15: 0000000000000000
> Jan 06 02:07:53 kernel: FS:  0000000000000000(0000) GS:ffff940db7500000(0000) knlGS:0000000000000000
> Jan 06 02:07:53 kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Jan 06 02:07:53 kernel: CR2: 0000000000000058 CR3: 000000010230a000 CR4: 00000000001506e0
> Jan 06 02:07:53 kernel: Call Trace:
> Jan 06 02:07:53 kernel:  <TASK>
> Jan 06 02:07:53 kernel:  amd_iommu_int_thread+0x60c/0x760
> Jan 06 02:07:53 kernel:  ? __pfx_irq_thread_fn+0x10/0x10
> Jan 06 02:07:53 kernel:  irq_thread_fn+0x1f/0x60
> Jan 06 02:07:53 kernel:  irq_thread+0xea/0x1a0
> Jan 06 02:07:53 kernel:  ? preempt_count_add+0x6a/0xa0
> Jan 06 02:07:53 kernel:  ? __pfx_irq_thread_dtor+0x10/0x10
> Jan 06 02:07:53 kernel:  ? __pfx_irq_thread+0x10/0x10
> Jan 06 02:07:53 kernel:  kthread+0xe9/0x110
> Jan 06 02:07:53 kernel:  ? __pfx_kthread+0x10/0x10
> Jan 06 02:07:53 kernel:  ret_from_fork+0x2c/0x50
> Jan 06 02:07:53 kernel:  </TASK>
> 
> Reported-by: Matt Fagnani <matt.fagnani@bell.net>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>   drivers/iommu/amd/iommu.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 26c96b821fa2..2421acba08c7 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -558,6 +558,9 @@ static void amd_iommu_report_page_fault(struct amd_iommu *iommu,
>   		 * prevent logging it.
>   		 */
>   		if (IS_IOMMU_MEM_TRANSACTION(flags)) {
> +			/* Device not attached to domain properly */
> +			BUG_ON(dev_data->domain == NULL);
> +
>   			if (!report_iommu_fault(&dev_data->domain->domain,
>   						&pdev->dev, address,
>   						IS_WRITE_REQUEST(flags) ?

Re: [PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Vasant Hegde]

Hi Robin,


On 1/13/2023 9:45 PM, Robin Murphy wrote:
> On 2023-01-13 13:59, Vasant Hegde wrote:
>> If IOMMU domain for device group is not setup properly then we may hit
>> IOMMU page fault. Current page fault handler assumes that domain is
>> always setup and it will hit NULL pointer dereference (see below sample log).
>>
>> This is mostly programming error and difficult to recover from here. Hence
>> call BUG_ON().
> 
> Is it particularly difficult to skip calling report_iommu_fault()? Certainly it

Not really. In fact initially I was thinking of logging error message if domain
is not set properly and return. But in this case we hit the issue because setup
was not proper (not like fault handler getting called before initialization).
But since we can't recover it properly I didn't see the point to return.


> didn't seem much of a challenge in 00ef8885a945 ("iommu/mediatek: Fix crash on
> isr after kexec()"), for example.

> 
> Otherwise, even disregarding [1], a BUG_ON() for a condition that's guaranteed

Thanks for reminding the link! I thought explicit BUG_ON is better. But yeah its
not a big function and fairly easy to debug. We can drop this patch.

-Vasant


> to crash right there anyway doesn't seem particularly valuable. The fact that it
> crashes already pretty conclusively documents the fact that it wasn't expected.
> report_iommu_fault() is not a big complex function, with only one possible
> callsite related to amd_iommu_int_thread(), so even with
> amd_iommu_report_page_fault() inlined I'd hope the stacktrace as below is
> sufficiently obvious even without logging an explicit line number.
> 
> Thanks,
> Robin.
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on
> 
>>
>> Sample log:
>> ----------
>> Jan 06 02:07:52 kernel: amdgpu 0000:00:01.0: amdgpu: SE 1, SH per SE 1, CU per
>> SH 8, active_cu_number 6
>> Jan 06 02:07:52 kernel: BUG: kernel NULL pointer dereference, address:
>> 0000000000000058
>> Jan 06 02:07:52 kernel: #PF: supervisor read access in kernel mode
>> Jan 06 02:07:53 kernel: #PF: error_code(0x0000) - not-present page
>> Jan 06 02:07:53 kernel: PGD 0 P4D 0
>> Jan 06 02:07:53 kernel: Oops: 0000 [#1] PREEMPT SMP NOPTI
>> Jan 06 02:07:53 kernel: CPU: 2 PID: 56 Comm: irq/24-AMD-Vi Not tainted
>> 6.2.0-rc2+ #89
>> Jan 06 02:07:53 kernel: Hardware name: xxx
>> Jan 06 02:07:53 kernel: RIP: 0010:report_iommu_fault+0x11/0x90
>> Jan 06 02:07:53 kernel: Code: 0f 0b eb cd 0f 1f 44 00 00 90 90 90 90 90 90 90
>> 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 41 55 41 54 41 89 cc 55 48 89 d5 53
>> <48> 8b 47 48 48 89 f3 48 85 c0 74 64 4c 8b 47 50 e8 ea e8 4a 00 41
>> Jan 06 02:07:53 kernel: RSP: 0018:ffffb9b7803ebe08 EFLAGS: 00010246
>> Jan 06 02:07:53 kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX:
>> 0000000000000000
>> Jan 06 02:07:53 kernel: RDX: 00000001522e2500 RSI: ffff940cc09e30d0 RDI:
>> 0000000000000010
>> Jan 06 02:07:53 kernel: RBP: 00000001522e2500 R08: ffff940cc12c2b00 R09:
>> 0000000000000050
>> Jan 06 02:07:53 kernel: R10: ffff940cc021e000 R11: 0000000000000000 R12:
>> 0000000000000000
>> Jan 06 02:07:53 kernel: R13: ffff940cc005b000 R14: 0000000000000008 R15:
>> 0000000000000000
>> Jan 06 02:07:53 kernel: FS:  0000000000000000(0000) GS:ffff940db7500000(0000)
>> knlGS:0000000000000000
>> Jan 06 02:07:53 kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> Jan 06 02:07:53 kernel: CR2: 0000000000000058 CR3: 000000010230a000 CR4:
>> 00000000001506e0
>> Jan 06 02:07:53 kernel: Call Trace:
>> Jan 06 02:07:53 kernel:  <TASK>
>> Jan 06 02:07:53 kernel:  amd_iommu_int_thread+0x60c/0x760
>> Jan 06 02:07:53 kernel:  ? __pfx_irq_thread_fn+0x10/0x10
>> Jan 06 02:07:53 kernel:  irq_thread_fn+0x1f/0x60
>> Jan 06 02:07:53 kernel:  irq_thread+0xea/0x1a0
>> Jan 06 02:07:53 kernel:  ? preempt_count_add+0x6a/0xa0
>> Jan 06 02:07:53 kernel:  ? __pfx_irq_thread_dtor+0x10/0x10
>> Jan 06 02:07:53 kernel:  ? __pfx_irq_thread+0x10/0x10
>> Jan 06 02:07:53 kernel:  kthread+0xe9/0x110
>> Jan 06 02:07:53 kernel:  ? __pfx_kthread+0x10/0x10
>> Jan 06 02:07:53 kernel:  ret_from_fork+0x2c/0x50
>> Jan 06 02:07:53 kernel:  </TASK>
>>
>> Reported-by: Matt Fagnani <matt.fagnani@bell.net>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   drivers/iommu/amd/iommu.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 26c96b821fa2..2421acba08c7 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -558,6 +558,9 @@ static void amd_iommu_report_page_fault(struct amd_iommu
>> *iommu,
>>            * prevent logging it.
>>            */
>>           if (IS_IOMMU_MEM_TRANSACTION(flags)) {
>> +            /* Device not attached to domain properly */
>> +            BUG_ON(dev_data->domain == NULL);
>> +
>>               if (!report_iommu_fault(&dev_data->domain->domain,
>>                           &pdev->dev, address,
>>                           IS_WRITE_REQUEST(flags) ?

Re: [PATCH 1/3] iommu: Attach device group to old domain in error path

[Vasant Hegde]

Hi Joerg,

Did you get chance to look into this series?

-Vasant


On 1/13/2023 7:29 PM, Vasant Hegde wrote:
> iommu_attach_group() attaches all devices in a group to domain and then
> sets group domain (group->domain). Current code (__iommu_attach_group())
> does not handle error path. This creates problem as devices to domain
> attachment is in inconsistent state.
> 
> To recover from this situation, we need force attaching all devices back
> to the old domain. This patch introduces `force` parameter to
> __iommu_group_set_domain() so that we can attach devices back to old
> domain.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/iommu.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index de91dd88705b..e58683cd3bf7 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -95,7 +95,8 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>  static int __iommu_attach_group(struct iommu_domain *domain,
>  				struct iommu_group *group);
>  static int __iommu_group_set_domain(struct iommu_group *group,
> -				    struct iommu_domain *new_domain);
> +				    struct iommu_domain *new_domain,
> +				    bool force);
>  static int iommu_create_device_direct_mappings(struct iommu_group *group,
>  					       struct device *dev);
>  static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> @@ -1974,7 +1975,7 @@ static void __iommu_group_set_core_domain(struct iommu_group *group)
>  	else
>  		new_domain = group->default_domain;
>  
> -	ret = __iommu_group_set_domain(group, new_domain);
> +	ret = __iommu_group_set_domain(group, new_domain, false);
>  	WARN(ret, "iommu driver failed to attach the default/blocking domain");
>  }
>  
> @@ -2124,8 +2125,16 @@ static int __iommu_attach_group(struct iommu_domain *domain,
>  
>  	ret = __iommu_group_for_each_dev(group, domain,
>  					 iommu_group_do_attach_device);
> -	if (ret == 0)
> +	if (ret == 0) {
>  		group->domain = domain;
> +	} else {
> +		/*
> +		 * To recover from the case when certain device within the
> +		 * group fails to attach to the new domain, we need force
> +		 * attaching all devices back to the old domain.
> +		 */
> +		__iommu_group_set_domain(group, group->domain, true);
> +	}
>  
>  	return ret;
>  }
> @@ -2164,11 +2173,12 @@ static int iommu_group_do_detach_device(struct device *dev, void *data)
>  }
>  
>  static int __iommu_group_set_domain(struct iommu_group *group,
> -				    struct iommu_domain *new_domain)
> +				    struct iommu_domain *new_domain,
> +				    bool force)
>  {
>  	int ret;
>  
> -	if (group->domain == new_domain)
> +	if (!force && group->domain == new_domain)
>  		return 0;
>  
>  	/*
> @@ -3135,7 +3145,7 @@ static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner)
>  	ret = __iommu_group_alloc_blocking_domain(group);
>  	if (ret)
>  		return ret;
> -	ret = __iommu_group_set_domain(group, group->blocking_domain);
> +	ret = __iommu_group_set_domain(group, group->blocking_domain, false);
>  	if (ret)
>  		return ret;
>  
> @@ -3222,7 +3232,7 @@ static void __iommu_release_dma_ownership(struct iommu_group *group)
>  
>  	group->owner_cnt = 0;
>  	group->owner = NULL;
> -	ret = __iommu_group_set_domain(group, group->default_domain);
> +	ret = __iommu_group_set_domain(group, group->default_domain, false);
>  	WARN(ret, "iommu driver failed to attach the default domain");
>  }
>  

Re: [PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Joerg Roedel]

On Fri, Jan 13, 2023 at 01:59:56PM +0000, Vasant Hegde wrote:
> If IOMMU domain for device group is not setup properly then we may hit
> IOMMU page fault. Current page fault handler assumes that domain is
> always setup and it will hit NULL pointer dereference (see below sample log).
> 
> This is mostly programming error and difficult to recover from here. Hence
> call BUG_ON().

I agree with Robin here, a BUG_ON() is no improvement over a NULL-ptr
dereference. Just handle the domain == NULL case so that this code-path
will not crash.

Thanks,

	Joerg

Re: [PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Vasant Hegde]

Hi Joerg,


On 2/3/2023 3:06 PM, Joerg Roedel wrote:
> On Fri, Jan 13, 2023 at 01:59:56PM +0000, Vasant Hegde wrote:
>> If IOMMU domain for device group is not setup properly then we may hit
>> IOMMU page fault. Current page fault handler assumes that domain is
>> always setup and it will hit NULL pointer dereference (see below sample log).
>>
>> This is mostly programming error and difficult to recover from here. Hence
>> call BUG_ON().
> 
> I agree with Robin here, a BUG_ON() is no improvement over a NULL-ptr
> dereference. Just handle the domain == NULL case so that this code-path
> will not crash.

Initially I was thinking of logging error message and return. But it will hit
device faults continuously.  Also we don't have nice way to recover from this
condition.

So I thought crashing here with explicit BUG_ON is better. But I agree with
Robin's comment. We don't really get anything extra with BUG_ON. We should be
able to figureout crash reason based on NULL pointer dereference easily.

Do you want me to just add explicit log here? Otherwise I am fine dropping this
patch (3/3). I think we still need first two patches from this series.

-Vasant

Re: [PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Joerg Roedel]

Hi Vasant

On Fri, Feb 03, 2023 at 04:10:33PM +0530, Vasant Hegde wrote:
> Initially I was thinking of logging error message and return. But it will hit
> device faults continuously.  Also we don't have nice way to recover from this
> condition.
> 
> So I thought crashing here with explicit BUG_ON is better. But I agree with
> Robin's comment. We don't really get anything extra with BUG_ON. We should be
> able to figureout crash reason based on NULL pointer dereference easily.
> 
> Do you want me to just add explicit log here? Otherwise I am fine dropping this
> patch (3/3). I think we still need first two patches from this series.

The best for now is to print a ratelimited error message and just return
from the handler. The ratelimit ensures that no misbehaving device can
flood the kernel log.

Regards,

	Joerg

Re: [PATCH 3/3] iommu/amd: Call BUG_ON in page fault hanlder path if domain is NULL

[Vasant Hegde]

Joerg,


On 2/3/2023 4:42 PM, Joerg Roedel wrote:
> Hi Vasant
> 
> On Fri, Feb 03, 2023 at 04:10:33PM +0530, Vasant Hegde wrote:
>> Initially I was thinking of logging error message and return. But it will hit
>> device faults continuously.  Also we don't have nice way to recover from this
>> condition.
>>
>> So I thought crashing here with explicit BUG_ON is better. But I agree with
>> Robin's comment. We don't really get anything extra with BUG_ON. We should be
>> able to figureout crash reason based on NULL pointer dereference easily.
>>
>> Do you want me to just add explicit log here? Otherwise I am fine dropping this
>> patch (3/3). I think we still need first two patches from this series.
> 
> The best for now is to print a ratelimited error message and just return
> from the handler. The ratelimit ensures that no misbehaving device can
> flood the kernel log.

Sure. Let me fix it and send v2.

-Vasant