Re: [PATCH 1/3] drm/xe: Read scheduler message opcode under lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe: data race on msg->opcode in tracepoint
- [Medium] drm/xe: memory leak of dynamically allocated scheduler messages
--

commit f8887f43a6b200067866b162e2ad500eb6e16175
Author: Matthew Brost <matthew.brost@intel.com>

drm/xe: Read scheduler message opcode under lock

For static scheduler messages, the opcode is only guaranteed to remain
stable while the message is on the pending message list. Read the opcode
under the message lock before removing the message from the pending list,
and pass it through the processing pipeline.

> diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> index 9c8004d5dd915..d656db758cb3c 100644
> --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c

[ ... ]

> @@ -42,13 +46,14 @@ static void xe_sched_process_msg_work(struct work_struct *w)
>  	struct xe_gpu_scheduler *sched =
>  		container_of(w, struct xe_gpu_scheduler, work_process_msg);
>  	struct xe_sched_msg *msg;
> +	unsigned int opcode;
>  
>  	if (drm_sched_is_stopped(&sched->base))
>  		return;

[Severity: Medium]
When the scheduler is stopped, does this early return leak dynamically
allocated messages?

If untrusted userspace triggers an error path by queuing property change
messages (like SET_SCHED_PROPS) and then causes the queue to hang, wedge, or
close during teardown, this worker exits without draining or freeing pending
dynamically allocated messages.

During queue teardown, xe_sched_fini() and other cleanup paths fail to
traverse sched->msgs and kfree() dynamically allocated messages before the
list and lock are destroyed.

> -	msg = xe_sched_get_msg(sched);
> +	msg = xe_sched_get_msg(sched, &opcode);
>  	if (msg) {
> -		sched->ops->process_msg(msg);
> +		sched->ops->process_msg(msg, opcode);
>  
>  		xe_sched_process_msg_queue_if_ready(sched);
>  	}

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 7ce73489c35fc..c4ef3e9bf7a18 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2056,16 +2056,17 @@ static void __guc_exec_queue_process_msg_cgp_sync(struct xe_sched_msg *msg,
>  #define MSG_HEAD	BIT(9)
>  #define MSG_PM_REF	BIT(10)
>  
> -static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
> +static void guc_exec_queue_process_msg(struct xe_sched_msg *msg,
> +				       unsigned int opcode)
>  {
>  	struct xe_device *xe = guc_to_xe(exec_queue_to_guc(msg->private_data));
>  	int idx;
> -	bool pm_ref = !!(msg->opcode & MSG_PM_REF);
> +	bool pm_ref = !!(opcode & MSG_PM_REF);
>  	bool bound = drm_dev_enter(&xe->drm, &idx);
>  
>  	trace_xe_sched_msg_recv(msg);

[Severity: Medium]
Does this tracepoint still contain a data race by reading the unstable
msg->opcode?

The patch addresses a data race by extracting msg->opcode into a local
variable under msg_lock, because static messages can be concurrently
re-enqueued modifying their opcode outside the lock. However,
trace_xe_sched_msg_recv(msg) continues to read msg->opcode directly via the
xe_sched_msg event class:

drivers/gpu/drm/xe/xe_trace.h:DECLARE_EVENT_CLASS(xe_sched_msg) {
    ...
    __entry->opcode = msg->opcode;
    ...
}

Because trace_xe_sched_msg_recv() is called here with the message pointer,
rather than the stable extracted opcode, this leaves a data race where the
tracepoint may log an incorrect opcode or trigger KCSAN warnings.

> -	switch (msg->opcode & OPCODE_MASK) {
> +	switch (opcode & OPCODE_MASK) {
>  	case CLEANUP:
>  		__guc_exec_queue_process_msg_cleanup(msg, bound);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013023442.206915-1-daniel.charles@intel.com?part=1