Re: [PATCH v2 2/3] CI_ONLY Mark VMs as closing before queue cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/xe: NULL pointer dereference in xe_pagefault_print() on
  ack_fault failure
--

commit 213c218fbf049faac8f39a23293d5c3f7bb580b5
Author: Arvind Yadav <arvind.yadav@intel.com>

CI_ONLY Mark VMs as closing before queue cleanup

This commit introduces an XE_VM_FLAG_CLOSING state set before asynchronous
queue teardown to block new VM work and serialize cached page-fault
acknowledgments against VM closure.

> diff --git a/drivers/gpu/drm/xe/xe_pagefault.c b/drivers/gpu/drm/xe/xe_pagefault.c
> index 98cce0f12b123..ac05e7b6315de 100644
> --- a/drivers/gpu/drm/xe/xe_pagefault.c
> +++ b/drivers/gpu/drm/xe/xe_pagefault.c

[ ... ]

> @@ -678,32 +678,35 @@ static void xe_pagefault_queue_work(struct work_struct *w)
>  
>  	while (xe_pagefault_queue_pop(pf_queue, &pf, pf_work->id)) {
>  		const struct xe_pagefault_ops *ops = pf->producer.ops;
> +		struct xe_vm *ack_vm = NULL;
>  		void *private = pf->producer.private;
>  		struct xe_gt *gt = pf->gt;
>  		u32 asid = FIELD_GET(XE_PAGEFAULT_ASID_MASK, pf->consumer.id);
> +		struct xe_pagefault original = {
> +			.gt = gt,
> +			.consumer = {
> +				.page_addr = pf->consumer.page_addr,
> +				.access_type = pf->consumer.access_type,
> +				.fault_type_level = pf->consumer.fault_type_level,
> +				.engine_class_instance =
> +					pf->consumer.engine_class_instance,
> +				.id = pf->consumer.id,
> +			},
> +		};
> +		struct xe_pagefault *report_pf = pf;
>  		int err = 0;
>  		bool invalidated = false;
> +		bool cache_hit = false;
>  
>  		/* Last fault same address, ack immediately */
>  		if (xe_pagefault_match(pf, cache_start, cache_end, cache_asid)) {
> -			xe_gt_stats_incr(gt, XE_GT_STATS_ID_LAST_PAGEFAULT_COUNT, 1);
> +			cache_hit = true;
>  			goto ack_fault;
>  		}
>  
>  		err = xe_pagefault_service(pf);
>  
> -		if (err) {
> -			if (!(pf->consumer.access_type & XE_PAGEFAULT_ACCESS_PREFETCH)) {
> -				xe_pagefault_save_to_vm(gt_to_xe(gt), pf);
> -				xe_pagefault_cache_start_invalidate(cache_start);
> -				xe_pagefault_print(pf);
> -				xe_log_err_info(pf->gt, PAGEFAULT, err, "Unsuccessful response\n");
> -			} else {
> -				xe_gt_stats_incr(pf->gt, XE_GT_STATS_ID_INVALID_PREFETCH_PAGEFAULT_COUNT, 1);
> -				xe_gt_dbg(pf->gt, "Prefetch Fault response: Unsuccessful %pe\n",
> -					  ERR_PTR(err));
> -			}
> -		} else {
> +		if (!err) {
>  			/* Cache valid fault locally */
>  			cache_start = xe_pagefault_start_addr(pf);
>  			cache_end = xe_pagefault_end_addr(pf);
> @@ -711,6 +714,55 @@ static void xe_pagefault_queue_work(struct work_struct *w)
>  		}
>  
>  ack_fault:
> +		/*
> +		 * Cache hits bypass fault service. Serialize successful replies
> +		 * with VM close, including replies to matching chained faults.
> +		 * Take vm->lock before ack_fault_begin() takes the CT lock and
> +		 * hold it until ack_fault_end() flushes the reply batch.
> +		 */
> +		if (!err) {
> +			/*
> +			 * Successful service may replace the fault fields with
> +			 * the resolved range. Use the original fields to report
> +			 * a failure detected here. Do not restore the live entry,
> +			 * which also contains queue state and chain links.
> +			 */
> +			report_pf = &original;
> +			ack_vm = xe_pagefault_asid_to_vm(report_pf, asid);
> +			if (IS_ERR(ack_vm)) {
> +				err = PTR_ERR(ack_vm);
> +				ack_vm = NULL;
> +			} else {
> +				down_read(&ack_vm->lock);
> +				if (xe_vm_is_closed_or_banned(ack_vm)) {
> +					xe_pagefault_set_error(report_pf,
> +							       XE_PAGEFAULT_ERROR_VM_CLOSED);
> +					err = -ENOENT;
> +					up_read(&ack_vm->lock);
> +					xe_vm_put(ack_vm);
> +					ack_vm = NULL;
> +				}
> +			}
> +		}
> +
> +		if (err) {
> +			xe_pagefault_cache_start_invalidate(cache_start);
> +			if (!(report_pf->consumer.access_type &
> +			      XE_PAGEFAULT_ACCESS_PREFETCH)) {
> +				xe_pagefault_save_to_vm(xe, report_pf);
> +				xe_pagefault_print(report_pf);

[Severity: High]
Does xe_pagefault_print(report_pf) dereference a NULL pf->producer.ops
pointer when the post-service ASID lookup or VM state check fails?

original only initializes .gt and .consumer, leaving original.producer
zero-initialized.  When report_pf is set to &original and the check at
ack_fault sets err, xe_pagefault_print(report_pf) dereferences
pf->producer.ops->print:

drivers/gpu/drm/xe/xe_pagefault.c:xe_pagefault_print() {
	enum xe_pagefault_error err = xe_pagefault_get_error(pf);

	if (WARN_ON_ONCE(!pf->producer.ops->print))
		return;

	pf->producer.ops->print(pf, xe_pagefault_error_to_str(err));
}

In addition, xe_guc_pagefault_print() reads pf->producer.msg to decode the
fault details, so original also needs .producer = pf->producer.

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