Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/qaic: UAF and double free in create_slice_jobs error path - [Medium] drm/qaic: sequence number collision breaks DRM scheduler dependencies - [Medium] drm/qaic: silent failure propagation due to clobbered error code --
commit 0016fc8438fc80359a88bc33d3c9d6dd42769622 Author: Carl Vanderlip <[email protected]> accel/qaic: Split oversized BO requests into multiple jobs This commit updates the job submission logic to handle oversized buffer objects by dividing them into multiple jobs that fit within the hardware FIFO. It modifies job creation and scheduling to accommodate multiple chained requests for a single slice. > diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c > index 63ca4a489a9a3..7287370e4fab7 100644 > --- a/drivers/accel/qaic/qaic_data.c > +++ b/drivers/accel/qaic/qaic_data.c [ ... ] > @@ -1169,11 +1166,11 @@ static inline void qaic_dbc_put_job(struct > dma_bridge_chan *dbc, struct qaic_job > static int create_slice_jobs(struct drm_file *file_priv, struct bo_slice > *slice, bool partial, > u64 resize, struct list_head *tmp_list) > { > + unsigned int nents_offset, nents_xfer; > struct dbc_req *reqs = slice->reqs; > - struct qaic_job *job = NULL; > + struct qaic_job *job, *job_i; > unsigned int job_count = 0; > - unsigned int nents_xfer; > - u64 last_bytes; > + u64 last_bytes, seq_no; > u32 first_n; > > /* > @@ -1194,16 +1191,32 @@ static int create_slice_jobs(struct drm_file > *file_priv, struct bo_slice *slice, > else > nents_xfer = slice->nents; > > - job = qaic_create_job(file_priv, slice, nents_xfer, 0, tmp_list); > - if (IS_ERR(job)) > - return PTR_ERR(job); > - job_count++; > + nents_offset = 0; > + seq_no = 0; > + job = NULL; > + while (nents_xfer > nents_offset) { > + job = qaic_create_job(file_priv, slice, nents_offset, > nents_xfer, > + seq_no++, tmp_list); [Severity: Medium] When a BO has multiple slices, does this reset the sequence number for every slice? This appears to create duplicate sequence numbers on the same shared BO fence context. Because drm_sched_job_add_dependency() deduplicates fences using dma_fence_is_later(), the explicit intra-BO job dependencies might be silently dropped if the new seq_no is not greater than older sequence numbers on the same context. > + if (IS_ERR(job)) > + goto free_jobs; > + > + job_count++; > + nents_offset += job->num_req; > + } > > /* job points to the last job for this slice, only valid for partial > execute ioctl */ > job->partial_size = last_bytes; > job->partial = partial; > > return job_count; > + > +free_jobs: > + list_for_each_entry_safe_reverse(job, job_i, tmp_list, queue) > + if (job_count--) > + qaic_cleanup_job(job); [Severity: High] Does this error path leave dangling pointers in tmp_list? By calling qaic_cleanup_job() without removing the job from tmp_list (e.g., via list_del()), the freed jobs remain in the list. If send_bo_list_to_sched() is the caller and this returns an error (or 0), the error path there iterates over tmp_list again: drivers/accel/qaic/qaic_data.c:send_bo_list_to_sched() { ... list_for_each_entry_safe_reverse(job_i, tmp, &tmp_list, queue) qaic_cleanup_job(job_i); ... } Would this result in a double free of the jobs? [Severity: Medium] Will this error path clobber the original error code from qaic_create_job()? The job iterator here shadows the job variable containing the ERR_PTR, and the function returns the post-decremented job_count (which is 0 or -1). When this propagates up to send_bo_list_to_sched(), it sees a return of 0: drivers/accel/qaic/qaic_data.c:send_bo_list_to_sched() { ... ret = create_slice_jobs(file_priv, slice, false, 0, &tmp_list); if (ret <= 0) goto unlock_bo; ... return ret; } Could this mask the failure and return 0 (success) to userspace, causing a hang when userspace waits for BO fences that will never be signaled? > + else > + break; > + return job_count; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
