Re: [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Integer truncation on 32-bit platforms breaks infinite waits.
- [High] xa_alloc_cyclic() error path causes data corruption by erasing valid fences.
- [High] GFP_KERNEL memory allocation under dma_resv locks causes deadlocks.
- [Medium] Legitimate negative error codes from dma_fence_wait_timeout are masked to -ETIME.
--

--- Patch [6]: [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking ---
commit fcd8c204d7bfbab0abf15f0e12e9b8db05263bcc
Author: Maíra Canal <mcanal@igalia.com>

    drm/vc4: Add per-file descriptor seqno tracking
    
    Implement per-file descriptor seqno tracking using an xarray, allowing
    userspace to wait on specific job completions via vc4_wait_seqno_ioctl.

> diff --git a/drivers/gpu/drm/vc4/vc4_submit.c b/drivers/gpu/drm/vc4/vc4_submit.c
> index 5e839db2ba4d3..0e43f31fb813f 100644
> --- a/drivers/gpu/drm/vc4/vc4_submit.c
> +++ b/drivers/gpu/drm/vc4/vc4_submit.c
[ ... ]
> @@ -232,6 +232,57 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
>  	return ret;
>  }
>  
> +int
> +vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
> +		     struct drm_file *file_priv)
> +{
> +	struct vc4_file *vc4_priv = file_priv->driver_priv;
> +	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct drm_vc4_wait_seqno *args = data;
> +	unsigned long timeout_jiffies = nsecs_to_jiffies(args->timeout_ns);

[Severity: High]
On 32-bit architectures, could this truncate the 64-bit timeout_ns?

If userspace passes ~0ULL for an infinite wait, this would truncate to a
32-bit unsigned long, which then evaluates to a negative signed long when
passed to dma_fence_wait_timeout(), potentially causing the wait to fail
instantly.

> +	unsigned long start = jiffies;
> +	struct dma_fence *fence;
> +	long ret;
> +
[ ... ]
> +	if (!fence)
> +		return 0;
> +
> +	trace_vc4_wait_for_seqno_begin(dev, args->seqno, args->timeout_ns);
> +	ret = dma_fence_wait_timeout(fence, true, timeout_jiffies);
> +	trace_vc4_wait_for_seqno_end(dev, args->seqno);
> +
> +	dma_fence_put(fence);
> +
> +	if (ret == -ERESTARTSYS) {
> +		u64 delta = jiffies_to_nsecs(jiffies - start);
> +
> +		if (args->timeout_ns >= delta)
> +			args->timeout_ns -= delta;
> +		else
> +			args->timeout_ns = 0;
> +
> +		return ret;
> +	}
> +
> +	return ret > 0 ? 0 : -ETIME;

[Severity: Medium]
Does this mask legitimate negative error codes returned by
dma_fence_wait_timeout()?

If dma_fence_wait_timeout() returns an error like -EINVAL for invalid
timeouts or -EDEADLK, this translates it to -ETIME, which might mislead
userspace debugging and error handling.

> +}
> +
[ ... ]
> @@ -290,6 +341,10 @@ vc4_render_job_free(struct kref *ref)
>  	vc4->bin_alloc_used &= ~job->bin_slots;
>  	spin_unlock_irqrestore(&vc4->job_lock, irqflags);
>  
> +	if (job->seqno)
> +		xa_erase(&job->file->seqno_xa, job->seqno);

[Severity: High]
Could this unconditionally erase a valid fence belonging to a concurrent
submission if xa_alloc_cyclic() previously failed?

In vc4_submit_cl_ioctl(), xa_alloc_cyclic() can populate render->seqno with
a proposed ID before returning -ENOMEM. The error path would then call this
function, which erases the ID from the xarray. If another thread successfully
used that same ID, its completion fence would be incorrectly removed.

> +
> +	vc4_file_put(job->file);
>  	vc4_job_free(ref);
>  }
>  
[ ... ]
> @@ -547,11 +611,20 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
>  		goto fail_exec;
>  
>  	scoped_guard(mutex, &vc4->sched_lock) {
> -		ret = vc4_push_jobs(&exec);
> +		ret = xa_alloc_cyclic(&vc4_priv->seqno_xa, &render->seqno,
> +				      NULL, xa_limit_32b, &vc4_priv->next_seqno,
> +				      GFP_KERNEL);

[Severity: High]
Is it safe to use GFP_KERNEL here and in xa_store() while dma_resv locks are
held?

vc4_submit_cl_ioctl() calls vc4_lock_bo_reservations() before reaching this
code. Using GFP_KERNEL allows direct memory reclaim, which could invoke DRM
shrinkers. If a shrinker attempts to acquire the same dma_resv lock to evict
a buffer object, it could result in an AB-BA deadlock.

> +		if (ret < 0)
> +			goto fail_exec;
> +
> +		ret = vc4_push_jobs(vc4_priv, &exec);
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=6