Re: [PATCH] drm/xe: Don't wedge shared engine on stale faults from torn-down VMs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional `drm_info` logging in the page fault handler causes dmesg spam and potential DoS.
- [High] Setting identical start and end addresses breaks fault chaining, leading to O(N^2) requeuing loops.
- [Medium] Incomplete implementation of the commit message claim regarding ASIDs mapped to non-fault-capable VMs.
--

commit cb9b15da640021d0a9305672434a725603f3f8a3
Author: Sanjay Yadav <sanjay.kumar.yadav@intel.com>

drm/xe: Don't wedge shared engine on stale faults from torn-down VMs

This commit modifies the page fault handler to drain stale page faults 
associated with closed VMs or removed ASIDs rather than returning an error. 
This prevents shared engines from wedging when handling unserviceable 
faults from abruptly terminated processes.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> index aeb56ff5d58e5..18687e19bceed 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c
> @@ -238,8 +238,10 @@ static struct xe_vm *xe_pagefault_asid_to_vm(struct xe_device *xe, u32 asid)
>  	vm = xa_load(&xe->usm.asid_to_vm, asid);
>  	if (vm && xe_vm_in_fault_mode(vm))
>  		xe_vm_get(vm);
> -	else
> +	else if (vm)
>  		vm = ERR_PTR(-EINVAL);
> +	else
> +		vm = ERR_PTR(-ENOENT);
>  	up_read(&xe->usm.lock);
>  
>  	return vm;
> @@ -260,13 +262,27 @@ static int xe_pagefault_service(struct xe_pagefault *pf)
>  		return -EFAULT;
>  
>  	vm = xe_pagefault_asid_to_vm(xe, asid);
> -	if (IS_ERR(vm))
> +	if (IS_ERR(vm)) {
> +		if (PTR_ERR(vm) == -ENOENT) {

[Severity: Medium]
The commit message mentions we should drain faults for ASIDs that no longer
map to a fault-capable VM. However, xe_pagefault_asid_to_vm() returns
-EINVAL for that case, while this check in xe_pagefault_service() only
intercepts -ENOENT. Will this cause non-fault-capable VM errors to still
wedge the shared engine?

> +			drm_info(&xe->drm,
> +				 "xe_pf_debug: drain stale fault (no VM) asid=%u addr=0x%llx\n",
> +				 asid, pf->consumer.page_addr);

[Severity: High]
Does this unconditional drm_info() log in xe_pagefault_service() allow
userspace to spam dmesg? Since any userspace process can create a VM,
schedule GPU work, and abruptly exit, it seems this could generate a flood
of log messages. Would a debug log or rate-limited output be more
appropriate?

> +			xe_pagefault_set_start_addr(pf, pf->consumer.page_addr);
> +			xe_pagefault_set_end_addr(pf, pf->consumer.page_addr);

[Severity: High]
Does setting start and end to the same address break fault chaining?
When xe_pagefault_queue_pop() processes subsequent faults, it appears to
use xe_pagefault_match() to check if the address falls within the exclusive
bounds (page_addr >= start && page_addr < end). Since start equals end here,
creating a zero-length range, the condition can never be met. Can this lead
to O(N^2) unchained requeuing?

> +			return 0;
> +		}
>  		return PTR_ERR(vm);
> +	}
>  
>  	down_read(&vm->lock);
>  
>  	if (xe_vm_is_closed(vm)) {
> -		err = -ENOENT;
> +		drm_info(&xe->drm,
> +			 "xe_pf_debug: drain stale fault (closed VM) asid=%u addr=0x%llx\n",
> +			 asid, pf->consumer.page_addr);

[Severity: High]
Similar to the no VM case above, could this unconditional drm_info() log
be abused by userspace to trigger dmesg spam by creating and closing VMs
with pending work?

> +		xe_pagefault_set_start_addr(pf, pf->consumer.page_addr);
> +		xe_pagefault_set_end_addr(pf, pf->consumer.page_addr);

[Severity: High]
As with the previous block, setting the start and end to the same address
creates a zero-length range, which might break the exclusive bounds check
in xe_pagefault_match(). Can this lead to subsequent identical faults not
being coalesced properly?

> +		err = 0;
>  		goto unlock_vm;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911192300.1561972-2-sanjay.kumar.yadav@intel.com?part=1