Re: [PATCH 13/20] drm/xe/eudebug: Add vm open/pread/pwrite

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/xe/eudebug: unlocked vm->size reads in
  __xe_eudebug_vm_access()
- [Low] drm/xe/eudebug: detach race in __xe_eudebug_vm_access() returns
  -EINVAL
--

commit a3a16f65fbfea0ccc25672bcf54427e90a80cace
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>

drm/xe/eudebug: Add vm open/pread/pwrite

This commit adds the DRM_XE_EUDEBUG_IOCTL_VM_OPEN ioctl and file operations
for reading, writing, and syncing a target client's BO-backed VM mappings.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug_vm.c b/drivers/gpu/drm/xe/xe_eudebug_vm.c
> new file mode 100644
> index 0000000000000..385fcc7e8f5c2
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_eudebug_vm.c

[ ... ]

> +static ssize_t __xe_eudebug_vm_access(struct file *file,
> +				      char __user *r_buffer,
> +				      const char __user *w_buffer,
> +				      size_t count, loff_t *__pos)
> +{
> +	struct vm_file *vmf = file->private_data;
> +	struct xe_eudebug * const d = vmf->debugger;
> +	struct xe_device * const xe = d->xe;
> +	const bool write = !!w_buffer;
> +	struct xe_vm *vm;
> +	ssize_t copied = 0;
> +	ssize_t bytes_left;
> +	ssize_t ret;
> +	unsigned long alloc_len;
> +	loff_t pos = *__pos;
> +	void *k_buffer;
> +
> +	if (XE_IOCTL_DBG(d->xe, xe_eudebug_is_detached(d)))
> +		return -ENOTCONN;
> +
> +	if (XE_IOCTL_DBG(xe, write && r_buffer))
> +		return -EINVAL;
> +
> +	vm = xe_eudebug_vm_get(d, vmf->vm_handle);
> +	if (XE_IOCTL_DBG(xe, !vm))
> +		return -EINVAL;

[Severity: Low]
Can __xe_eudebug_vm_access() return -EINVAL instead of -ENOTCONN if the
target detaches between xe_eudebug_is_detached() and xe_eudebug_vm_get()?

CPU0                              CPU1
__xe_eudebug_vm_access()
  xe_eudebug_is_detached(d)
    -> false
                                  xe_eudebug_detach()
                                    d->target.xef = NULL
                                    xe_eudebug_resources_clear(d)
  xe_eudebug_vm_get(d, ...)
    -> NULL
  return -EINVAL;  <-- expected -ENOTCONN

Checking xe_eudebug_is_detached(d) when !vm, as xe_eudebug_vm_open_ioctl()
does, would return -ENOTCONN as documented in the UAPI header.

> +
> +	if (XE_IOCTL_DBG(xe, vm != vmf->vm)) {
> +		eu_warn(d, "vm_access(%s): vm handle mismatch vm_handle=%llu, flags=0x%llx, pos=%llu, count=%zu\n",
> +			str_write_read(write),
> +			vmf->vm_handle, vmf->flags, pos, count);
> +		xe_vm_put(vm);
> +		return -EINVAL;
> +	}
> +
> +	if (!count) {
> +		xe_vm_put(vm);
> +		return 0;
> +	}
> +
> +	if (pos >= vm->size) {
> +		xe_vm_put(vm);
> +		return write ? -ENOSPC : 0;
> +	}
> +
> +	count = min_t(u64, count, vm->size - pos);
> +	bytes_left = count;
> +
> +	alloc_len = min_t(unsigned long, ALIGN(count, PAGE_SIZE), 64 * SZ_1M);
> +	do  {
> +		k_buffer = __vmalloc(alloc_len, GFP_KERNEL_ACCOUNT | __GFP_NOWARN);

[Severity: Medium]
Can reading vm->size twice without holding vm->lock race with xe_vm_close()
zeroing vm->size?

drivers/gpu/drm/xe/xe_vm.c:xe_vm_close() {
	down_write(&vm->lock);
	xe_vm_lock(vm, false);

	if (xe_vm_in_fault_mode(vm))
		xe_svm_notifier_lock(vm);

	vm->size = 0;
    ...
}

CPU0                              CPU1
__xe_eudebug_vm_access()
  vm = xe_eudebug_vm_get(...)
                                  xe_vm_close(vm)
                                    down_write(&vm->lock)
  if (pos >= vm->size)
    -> false (pos == 0)
                                    vm->size = 0
                                    up_write(&vm->lock)
  count = min_t(u64, count,
                vm->size - pos)
    -> 0
  alloc_len = 0
  __vmalloc(0, ...)               <-- hits WARN_ON_ONCE(!size)

If pos > 0 when vm->size becomes 0 between the two reads, vm->size - pos
underflows u64; if vm->size is already 0 at the first read, pread() returns
0 (EOF) and pwrite() returns -ENOSPC instead of -EINVAL for a closed VM.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109135549.4128280-1-mika.kuoppala@linux.intel.com?part=13