Re: [PATCH RFC 07/13] drm/powervr_sgx: Add GEM objects, VMs and dma-buf sharing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential
issues to consider:
- [High] drm/powervr_sgx: unreaped objects across restarts in
  pvrsgx_kobj_reap()
- [High] drm/powervr_sgx: refcount cycle in pvrsgx_kobj_create() and
  pvrsgx_vm_create()
- [High] drm/powervr_sgx: unbounded row loop in range_flush() via
  cache_range_check()
- [High] drm/powervr_sgx: stranded pvrsgx_vm_free items on
  sdev->inval_waits leak drm_device
- [High] drm/powervr_sgx: race between pvrsgx_vm_lookup() and VM context
  teardown
- [Medium] drm/powervr_sgx: imported BOs rejected in
  pvrsgx_ioctl_get_bo_mmap_offset()
- [Medium] drm/powervr_sgx: missing cache flush and GFP flags in
  pvrsgx_ioctl_create_bo()
- [Medium] drm/powervr_sgx: unconditional EDMPROTECT on KDATA in
  krange() faults SGX530
- [Low] drm/powervr_sgx: stale sync_ops comment above
  pvrsgx_ioctl_vm_bind()
--

commit 296c713c959dc9c9e5a4a2d89cd48e4f92b9f9e5
Author: Joseph Telaak <joseph@telaak.dev>

drm/powervr_sgx: Add GEM objects, VMs and dma-buf sharing

This commit adds GEM shmem buffer objects, drm_gpuvm-based VM context and
VM_BIND management, kernel-owned GPU objects with deferred reaping, and
dma-buf export and import support for PowerVR SGX GPUs.

> diff --git a/drivers/gpu/drm/powervr_sgx/pvrsgx_gem.c b/drivers/gpu/drm/powervr_sgx/pvrsgx_gem.c
> new file mode 100644
> index 0000000000000..b22b819b01fd8
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/pvrsgx_gem.c
> @@ -0,0 +1,120 @@

[ ... ]

> +/* Every CPU mapping, the dma-buf's included, needs ALLOW_CPU_ACCESS. */
> +static int pvrsgx_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> +	if (!drm_gem_is_imported(obj) &&
> +	    !(to_pvrsgx_bo(obj)->flags & DRM_POWERVR_SGX_BO_ALLOW_CPU_ACCESS))
> +		return -EINVAL;
> +	return drm_gem_shmem_object_mmap(obj, vma);
> +}

[ ... ]

> +int pvrsgx_ioctl_create_bo(struct drm_device *drm, void *data, struct drm_file *file)
> +{
> +	struct drm_powervr_sgx_ioctl_create_bo_args *args = data;
> +	struct drm_gem_shmem_object *shmem;
> +	int err;
> +
> +	if (args->_padding_c || (args->flags & ~PVRSGX_BO_FLAGS_MASK) ||
> +	    !args->size || !PAGE_ALIGNED(args->size) || args->size > PVRSGX_BO_MAX_SIZE)
> +		return -EINVAL;
> +	if ((args->flags & DRM_POWERVR_SGX_BO_CODE) &&
> +	    (args->flags & DRM_POWERVR_SGX_BO_ALLOW_CPU_ACCESS))
> +		return -EINVAL;
> +
> +	shmem = drm_gem_shmem_create(drm, args->size);
> +	if (IS_ERR(shmem))
> +		return PTR_ERR(shmem);
> +	to_pvrsgx_bo(&shmem->base)->flags = args->flags;
> +	/* Pages the GPU can address. */
> +	mapping_set_gfp_mask(shmem->base.filp->f_mapping,
> +			     (GFP_HIGHUSER & ~__GFP_HIGHMEM) | __GFP_DMA32);

[Severity: Medium]
On ARM, can dirty cache lines from shmem page zeroing overwrite data written
to a BO before its first VM_BIND?

When shmem allocates backing pages, the kernel zeroes them through the
cacheable linear mapping, leaving dirty zero cache lines in the CPU data
cache.  Unlike x86, drm_gem_shmem_get_pages_locked() does not call
set_pages_array_wc() on ARM, so writing to a BO via mmap() or UPLOAD_CODE
before VM_BIND writes through a write-combined mapping to DRAM while those
dirty zero lines remain cached.  When bind_op_check() later calls
drm_gem_shmem_get_pages_sgt() -> dma_map_sgtable(), those dirty zero lines
are flushed to DRAM over the earlier writes.

Also, mapping_set_gfp_mask() replaces the mask set by __drm_gem_shmem_init()
and drops __GFP_RETRY_MAYFAIL | __GFP_NOWARN, so large DMA32 BO allocations
up to PVRSGX_BO_MAX_SIZE (1 GiB) can trigger page allocation failure warnings
in dmesg under memory pressure.

> +
> +	err = drm_gem_handle_create(file, &shmem->base, &args->handle);
> +	drm_gem_object_put(&shmem->base);
> +	return err;
> +}
> +
> +int pvrsgx_ioctl_get_bo_mmap_offset(struct drm_device *drm, void *data, struct drm_file *file)
> +{
> +	struct drm_powervr_sgx_ioctl_get_bo_mmap_offset_args *args = data;
> +	struct drm_gem_object *obj;
> +	int err;
> +
> +	if (args->_padding_4)
> +		return -EINVAL;
> +
> +	obj = drm_gem_object_lookup(file, args->handle);
> +	if (!obj)
> +		return -ENOENT;
> +	if (!(to_pvrsgx_bo(obj)->flags & DRM_POWERVR_SGX_BO_ALLOW_CPU_ACCESS)) {
> +		err = -EINVAL;
> +		goto out;
> +	}

[Severity: Medium]
Does pvrsgx_ioctl_get_bo_mmap_offset() reject every PRIME-imported dma-buf
with -EINVAL?

Imported objects are created via pvrsgx_prime_import_sg_table() with
bo->flags == 0.  pvrsgx_gem_mmap() checks !drm_gem_is_imported(obj) before
requiring DRM_POWERVR_SGX_BO_ALLOW_CPU_ACCESS, but
pvrsgx_ioctl_get_bo_mmap_offset() does not, so userspace can never obtain the
mmap offset to reach pvrsgx_gem_mmap() on an imported BO.

[ ... ]

> diff --git a/drivers/gpu/drm/powervr_sgx/pvrsgx_kobj.c b/drivers/gpu/drm/powervr_sgx/pvrsgx_kobj.c
> new file mode 100644
> index 0000000000000..8b21315e9a135
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/pvrsgx_kobj.c
> @@ -0,0 +1,285 @@

[ ... ]

> +/* Each range in the core's layout (pvrsgx_core.c). */
> +static struct krange krange(const struct pvrsgx_device *sdev, enum pvrsgx_krange r)
> +{
> +	const struct pvrsgx_heap_layout *h = pvrsgx_heaps(sdev);
> +
> +	switch (r) {
> +	case PVRSGX_KRANGE_KDATA:	/* past the firmware's own arenas (pvrsgx_fw.c) */
> +		return (struct krange){ h->kernel_data.base + 0x10000, h->kernel_data.end,
> +					SGX_MMU_PTE_CACHECONSISTENT | SGX_MMU_PTE_EDMPROTECT };

[Severity: Medium]
Should PVRSGX_KRANGE_KDATA omit SGX_MMU_PTE_EDMPROTECT when
sdev->family->data_no_edmprotect is set, the way fw_arenas() does in
pvrsgx_fw.c?

On SGX530 (data_no_edmprotect = true), microkernel stores to pages mapped
with SGX_MMU_PTE_EDMPROTECT trigger BIF page faults, so microkernel writes to
context CCB control words, RT dataset status words, and render details in
PVRSGX_KRANGE_KDATA will fault and reset the GPU.

[ ... ]

> +	if (per_vm) {
> +		o->vm = pvrsgx_vm_get(vm);
> +	} else if (vm) {
> +		err = pvrsgx_mmu_map_pages(vm->mmu, o->node.start, o->km.pages, o->km.npages, 0);
> +		if (err)
> +			goto err_kmem;
> +		o->vm = pvrsgx_vm_get(vm);
> +	}

[Severity: High]
Does holding o->vm on graveyard objects create a reference cycle with
sdev->base that prevents pvrsgx_device_release() and pvrsgx_kobj_fini() from
ever running?

In pvrsgx_vm_create(), drm_gpuvm_init() takes a drm_dev_get(&sdev->base)
reference that is only dropped in vm_release() -> drm_gpuvm_put().  Each
VM-associated pvrsgx_kobj holds pvrsgx_vm_get(vm) until kobj_free(), while
pvrsgx_kobj_fini() is only called from the drmm callback
pvrsgx_device_release() when sdev->base.ref reaches 0.  If any VM-associated
kobj is sitting in sdev->graveyard or sdev->kobj_unmapped when the device is
unplugged (or is released when a DRM fd closes after unplug), o->vm keeps
vm->ref > 0, which keeps sdev->base.ref > 0 so pvrsgx_device_release() never
runs to reap the graveyard.

[ ... ]

> +void pvrsgx_kobj_reap(struct pvrsgx_device *sdev, u64 renders_done)
> +{
> +	struct pvrsgx_kobj *o, *tmp;
> +	bool all = renders_done == U64_MAX;
> +	u32 restarts = READ_ONCE(sdev->restarts);
> +	u64 seq = 0, consumed;
> +	LIST_HEAD(dead);
> +
> +	mutex_lock(&sdev->kobj_lock);
> +	list_for_each_entry_safe(o, tmp, &sdev->graveyard, grave) {\n> +		/* A restart forgets everything the old run pointed at. */
> +		if (all || o->grave_restarts != restarts ||
> +		    (o->grave_seq < renders_done && !o->until_restart))
> +			list_move_tail(&o->grave, &dead);
> +	}
> +	mutex_unlock(&sdev->kobj_lock);
> +
> +	if (!list_empty(&dead)) {
> +		list_for_each_entry(o, &dead, grave)
> +			kobj_unmap(o);
> +		/* The next command carries the invalidate. */
> +		mutex_lock(&sdev->ccb_lock);
> +		sdev->fw.cache_control |= PVRSGX_CC_INVAL_BIF_PT | PVRSGX_CC_INVAL_BIF_PD;
> +		seq = sdev->ccb_issued + 1;
> +		mutex_unlock(&sdev->ccb_lock);
> +	}
> +
> +	mutex_lock(&sdev->kobj_lock);
> +	list_for_each_entry(o, &dead, grave)
> +		o->flush_seq = seq;
> +	list_splice_tail_init(&dead, &sdev->kobj_unmapped);
> +	/*
> +	 * After a restart the ring starts over empty, so everything sent before it
> +	 * counts as consumed: the GPU was reset, and the first command after the
> +	 * microkernel's start carries an invalidate of its own.
> +	 */
> +	consumed = all ? U64_MAX : pvrsgx_ccb_consumed(sdev);
> +	list_for_each_entry_safe(o, tmp, &sdev->kobj_unmapped, grave) {
> +		if (o->flush_seq <= consumed)
> +			list_move_tail(&o->grave, &dead);
> +	}

[Severity: High]
Does assigning o->flush_seq = sdev->ccb_issued + 1 leave objects reaped
across a microkernel restart stranded in sdev->kobj_unmapped?

When pvrsgx_fw_start() restarts the microkernel, it resets the CCB ring
offsets to 0 without advancing sdev->ccb_issued and queues sdev->reap_work.
pvrsgx_kobj_reap() moves graveyard objects with o->grave_restarts != restarts
(including until_restart parameter buffer descriptors) to dead, sets
o->flush_seq = sdev->ccb_issued + 1, and compares it against
pvrsgx_ccb_consumed(sdev), which returns sdev->ccb_issued.  Because
o->flush_seq > consumed, neither those objects nor any objects already in
sdev->kobj_unmapped waiting for sdev->ccb_issued + 1 before the reset are
freed until another CCB command is issued and reaped.

Also, unlike pvrsgx_ccb_inval_reap(), consumed does not check
READ_ONCE(sdev->lost), so objects in sdev->kobj_unmapped remain unfreed when
the device is marked lost.

[ ... ]

> diff --git a/drivers/gpu/drm/powervr_sgx/pvrsgx_prime.c b/drivers/gpu/drm/powervr_sgx/pvrsgx_prime.c
> new file mode 100644
> index 0000000000000..e6aca114515dd
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/pvrsgx_prime.c
> @@ -0,0 +1,203 @@

[ ... ]

> +/* A range's flags, and its rows inside an object of obj_size bytes. */
> +static int cache_range_check(const struct drm_powervr_sgx_cache_range *r, u64 obj_size)
> +{
> +	u32 rows = max(r->rows, 1u);
> +	u64 span, end;
> +
> +	if (!r->flags || (r->flags & ~CACHE_RANGE_FLAGS) || !r->size)
> +		return -EINVAL;
> +	if (rows > 1 && r->stride < r->size)
> +		return -EINVAL;
> +	if (check_mul_overflow((u64)(rows - 1), (u64)r->stride, &span) ||
> +	    check_add_overflow(span, r->size, &span) ||
> +	    check_add_overflow(r->offset, span, &end) || end > obj_size)
> +		return -EINVAL;
> +	return 0;
> +}

[ ... ]

> +static u64 range_flush(const struct pvrsgx_cache_range *r)
> +{
> +	const struct iosys_map *map = &to_pvrsgx_bo(r->obj)->import_map;
> +
> +	/* I/O memory isn't cached. */
> +	if (!r->import || map->is_iomem)
> +		return 0;
> +	if (r->rows == 1 || r->stride == r->size) {
> +		drm_clflush_virt_range(map->vaddr + r->offset, r->size * r->rows);
> +	} else {
> +		for (u32 y = 0; y < r->rows; y++)
> +			drm_clflush_virt_range(map->vaddr + r->offset + (u64)y * r->stride,
> +					       r->size);
> +	}
> +	return r->size * r->rows;
> +}

[Severity: High]
Can a user submit cache ranges with a tiny size and stride (such as size = 1,
stride = 2, and rows = obj_size / 2) to stall the CPU in range_flush()?

On a 1 GiB imported dma-buf, cache_range_check() allows up to 2^29 rows per
range across 64 ranges.  Because r->stride != r->size, range_flush() calls
drm_clflush_virt_range() in a tight non-preemptible loop hundreds of millions
of times—each issuing two full memory barriers and two clflushopt
instructions—both in pvrsgx_run_job() and inside the fence-signaling section
in pvrsgx_job_process_done().

[ ... ]

> diff --git a/drivers/gpu/drm/powervr_sgx/pvrsgx_vm.c b/drivers/gpu/drm/powervr_sgx/pvrsgx_vm.c
> new file mode 100644
> index 0000000000000..b12d390622cfc
> --- /dev/null
> +++ b/drivers/gpu/drm/powervr_sgx/pvrsgx_vm.c
> @@ -0,0 +1,799 @@

[ ... ]

> +static void vm_free_work(struct work_struct *work)
> +{
> +	struct pvrsgx_vm_free *f = container_of(work, struct pvrsgx_vm_free, work);
> +	struct pvrsgx_device *sdev = to_pvrsgx_device(f->drm);
> +
> +	f->inval.done = vm_free_done;
> +	if (f->teardown) {
> +		mutex_lock(&f->vm->lock);
> +		vm_unmap_all(f->vm, &f->vas);
> +		mutex_unlock(&f->vm->lock);
> +	}
> +	/*
> +	 * The BIF caches walks only of a directory a job ran on, and a restart or
> +	 * power-off drops them: then there is nothing to wait for.
> +	 */
> +	if (!READ_ONCE(sdev->powered) ||
> +	    READ_ONCE(f->vm->ran_gen) != READ_ONCE(sdev->fw_gen) + 1) {
> +		vm_free_done(&f->inval);
> +		return;
> +	}
> +	pvrsgx_ccb_after_invalidate(sdev, &f->inval, PVRSGX_CC_INVAL_BIF_PT |
> +				    PVRSGX_CC_INVAL_BIF_PD | PVRSGX_CC_INVAL_DATA);
> +}

[Severity: High]
Can f->inval be queued onto sdev->inval_waits and never reaped across power-off
or unplug, leaking f->drm and deadlocking drmm teardown?

CPU0 (vm_free_work)               CPU1 (unplug or CCB completion)
vm_free_work()
  READ_ONCE(sdev->powered) == true
  pvrsgx_ccb_after_invalidate()
    mutex_lock(&sdev->ccb_lock)
    w->seq = sdev->ccb_issued + 1
    mutex_unlock(&sdev->ccb_lock)
                                  pvrsgx_job_unplug()
                                    sdev->lost = true
                                    pvrsgx_fw_stop()
                                    pvrsgx_ccb_inval_reap(sdev, true)
    spin_lock_irqsave(&sdev->inval_lock)
    list_add_tail(&w->node, &sdev->inval_waits)  <-- never reaped

In pvrsgx_ccb.c, pvrsgx_ccb_after_invalidate() drops sdev->ccb_lock before
taking sdev->inval_lock, and checks sdev->powered / sdev->lost without
holding sdev->inval_lock:

drivers/gpu/drm/powervr_sgx/pvrsgx_ccb.c:pvrsgx_ccb_after_invalidate() {
	if (READ_ONCE(sdev->lost)) {
		w->done(w);
		return;
	}
	mutex_lock(&sdev->ccb_lock);
	sdev->fw.cache_control |= cache_control;
	w->seq = sdev->ccb_issued + 1;
	mutex_unlock(&sdev->ccb_lock);
	spin_lock_irqsave(&sdev->inval_lock, flags);
	list_add_tail(&w->node, &sdev->inval_waits);
	spin_unlock_irqrestore(&sdev->inval_lock, flags);
	pvrsgx_ccb_inval_reap(sdev, false);
}

If pvrsgx_job_unplug() (or another CPU completing command w->seq) runs
pvrsgx_ccb_inval_reap() in that window, w->node is queued after the reap has
already finished.  Likewise, if vm_free_work() queues f->inval while the GPU
is idle before runtime suspend, pvrsgx_fw_stop() powers off the GPU without
calling pvrsgx_ccb_inval_reap().  Because vm_free_after_jobs() holds
drm_dev_get(f->drm) until vm_free_done(), a stranded entry on
sdev->inval_waits prevents sdev->base.ref from reaching 0, so the drmm
callback pvrsgx_device_release() never runs.

[ ... ]

> +void pvrsgx_vm_user_get(struct pvrsgx_vm *vm)
> +{
> +	atomic_inc(&vm->users);
> +}

[ ... ]

> +struct pvrsgx_vm *pvrsgx_vm_lookup(struct pvrsgx_file *sfile, u32 handle)
> +{
> +	struct pvrsgx_vm *vm;
> +
> +	xa_lock(&sfile->vms);
> +	vm = xa_load(&sfile->vms, handle);
> +	if (vm)
> +		pvrsgx_vm_get(vm);
> +	xa_unlock(&sfile->vms);
> +	return vm;
> +}

[Severity: High]
Can pvrsgx_ioctl_destroy_vm_context() race with a concurrent
pvrsgx_vm_lookup() user to map objects into an already torn-down VM or
resurrect vm->users from 0?

CPU0                              CPU1
pvrsgx_ioctl_vm_bind()
  vm = pvrsgx_vm_lookup()
                                  pvrsgx_ioctl_destroy_vm_context()
                                    xa_erase(&sfile->vms, handle)
                                    pvrsgx_vm_user_put(vm)
                                      atomic_dec_and_test(&vm->users)
                                      vm_free_work()
                                        vm_unmap_all(vm, ...)
  mutex_lock(&vm->lock)
  bind_apply(vm, ...)             <-- maps BOs into torn-down VM

pvrsgx_vm_lookup() only increments vm->ref under xa_lock(&sfile->vms),
without checking or incrementing vm->users, and pvrsgx_ioctl_vm_bind() never
checks whether vm->users has already dropped to 0 after acquiring vm->lock.
New mappings added after vm_unmap_all() stay pinned until vm_release() (and if
a context creation path calls pvrsgx_vm_user_get() after lookup, atomic_inc()
resurrects vm->users from 0 to 1 and triggers a second teardown when that
context is destroyed).

[ ... ]

> +/* Wait, interruptibly, for every wait op; find every signal op's syncobj. */
> +int pvrsgx_ioctl_vm_bind(struct drm_device *drm, void *data, struct drm_file *file)
> +{
> +	struct drm_powervr_sgx_ioctl_vm_bind_args *args = data;
> +	struct pvrsgx_vm_free *f = NULL;
> +	struct bind_op *ops = NULL;
> +	struct drm_powervr_sgx_vm_bind_op *in;
> +	struct pvrsgx_vm *vm;
> +	struct drm_exec exec;
> +	u32 n = args->ops.count;
> +	int err;
> +
> +	/* A synchronous bind takes no fences (drm-vm-bind-async.rst). */
> +	if (args->flags != DRM_POWERVR_SGX_VM_BIND_FLAG_SYNC || !n || args->sync_ops.count)
> +		return -EINVAL;

[Severity: Low]
This isn't a bug, but the comment above pvrsgx_ioctl_vm_bind() describes
waiting on and signaling sync_ops, whereas pvrsgx_ioctl_vm_bind() only
supports synchronous binds and rejects non-zero args->sync_ops.count.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-powervr-sgx-v1-0-c69b84bac011@telaak.dev?part=7