Re: [PATCH] drm/xe/vm: Reject invalid prefetch region for non-SVM VMA

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Jul 14, 2026 at 09:02:52AM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 14-07-2026 02:31, Matthew Brost wrote:
> > On Fri, Jul 10, 2026 at 02:17:00AM +0000, Shuicheng Lin wrote:
> > > DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC (-1) is only valid on a
> > > CPU-address-mirror (SVM) VMA. On a regular VMA the value is used as
> > > an index into region_to_mem_type[], causing an out-of-bounds access:
> > > 
> > >    UBSAN: array-index-out-of-bounds in drivers/gpu/drm/xe/xe_vm.c:3260:28
> > >    index 4294967295 is out of range for type 'u32 [3]'
> > >    Call Trace:
> > >     __ubsan_handle_out_of_bounds+0xa7/0xf0
> > >     vm_bind_ioctl_ops_execute+0x9b0/0x9d0 [xe]
> > >     xe_vm_bind_ioctl+0x19f1/0x1b10 [xe]
> > > 
> > > Three related changes:
> > > 
> > > - vm_bind_ioctl_ops_create(): For a non-CPU-address-mirror VMA, reject
> > >    both DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC and out-of-range prefetch
> > >    regions with -EINVAL. This is the primary fix for the OOB.
> > > 
> > > - op_lock_and_prep(): Tighten the xe_assert() to
> > >    'region < ARRAY_SIZE(region_to_mem_type)'. The
> > >    DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC exemption is no longer needed
> > >    since the value is rejected earlier, and '<=' was an off-by-one
> > >    bound (valid indices are 0..ARRAY_SIZE-1).
> > > 
> > > - xe_drm.h: Document the CPU-address-mirror constraint on the
> > >    DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC UAPI value.
> > > 
> > > Fixes: c1bb69a2e8e2 ("drm/xe/svm: Consult madvise preferred location in prefetch")
> > > Assisted-by: Claude:claude-opus-4.7
> > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > 
> > I think Himal at least fixed the memory safety problem later in the
> > pipeline here [1]. I'm unsure if he merged that one yet, but I'm
> > inclined to say this is a better solution.
> > 
> > What do you think Himal?
> 
> Hi Matt,
> 
> IMO -EINVAL isn't the right behaviour here. Rejecting fails the whole ioctl
> and prefetches nothing — including the SVM VMAs in the range that were
> always valid.
> 
> AFAIU prefetching a range that covers mixed VMAs is a perfectly valid use
> case. Today:
> 
> region 0 (system) works
> region 1 (devmem) works
> for -1 (CONSULT_MEM_ADVISE_PREF_LOC) we should prefetch each SVM VMA to its
> preferred location, and for a BO VMA try its preferred location (which is
> effectively always local devmem); if the BO has no VRAM placement, fall back
> to PL_TT — but no ioctl failure.
> 
> If we do decide -EINVAL is the right way, then we'd effectively be telling
> UMD that range-based prefetch only works with system/devmem, and that a
> CONSULT_MEM_ADVISE_PREF_LOC prefetch range must not contain any BO VMAs.
> That's a much more awkward contract to put on UMD, since a range can span
> both VMA types, so I'm not aligned with the EINVAL approach.
> 

Thanks for input here, I'm going to have to agree with Himal's line of
thinking here.

> Patch [1] is not merged yet, once you confirm will go ahead with it.
>

Go ahead merge.

Matt

> BR
> Himal
> 
> 
> > 
> > Matt
> > 
> > [1] https://patchwork.freedesktop.org/series/168913/
> > 
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Shuicheng Lin <shuicheng.lin@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_vm.c | 12 ++++++++++--
> > >   include/uapi/drm/xe_drm.h  |  4 +++-
> > >   2 files changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 080c2fff0e95..9430b2be18e4 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -2495,6 +2495,15 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> > >   			u32 i;
> > >   			if (!xe_vma_is_cpu_addr_mirror(vma)) {
> > > +				if (XE_IOCTL_DBG(vm->xe,
> > > +						 prefetch_region ==
> > > +						 DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC) ||
> > > +				    XE_IOCTL_DBG(vm->xe,
> > > +						 prefetch_region >=
> > > +						 ARRAY_SIZE(region_to_mem_type))) {
> > > +					err = -EINVAL;
> > > +					goto unwind_prefetch_ops;
> > > +				}
> > >   				op->prefetch.region = prefetch_region;
> > >   				break;
> > >   			}
> > > @@ -3236,8 +3245,7 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
> > >   		if (!xe_vma_is_cpu_addr_mirror(vma)) {
> > >   			region = op->prefetch.region;
> > > -			xe_assert(vm->xe, region == DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC ||
> > > -				  region <= ARRAY_SIZE(region_to_mem_type));
> > > +			xe_assert(vm->xe, region < ARRAY_SIZE(region_to_mem_type));
> > >   		}
> > >   		/*
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 509202a7b13e..e159c44e380a 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -1075,7 +1075,9 @@ struct drm_xe_vm_destroy {
> > >    *
> > >    * The @prefetch_mem_region_instance for %DRM_XE_VM_BIND_OP_PREFETCH can also be:
> > >    *  - %DRM_XE_CONSULT_MEM_ADVISE_PREF_LOC, which ensures prefetching occurs in
> > > - *    the memory region advised by madvise.
> > > + *    the memory region advised by madvise. Only valid when the target VMA
> > > + *    was created with %DRM_XE_VM_BIND_FLAG_CPU_ADDR_MIRROR; rejected with
> > > + *    -EINVAL otherwise.
> > >    */
> > >   struct drm_xe_vm_bind_op {
> > >   	/** @extensions: Pointer to the first extension struct, if any */
> > > -- 
> > > 2.43.0
> > > 
>