Re: [PATCH v10 05/10] drm/xe/exec_queue: Introduce xe_exec_queue_reinit()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A local denial of service (kernel panic) is possible when a user space application sets the DRM_XE_EXEC_QUEUE_SET_HANG_REPLAY_STATE property on platforms where default_lrc is not populated.
--

--- Patch [5]: [PATCH v10 05/10] drm/xe/exec_queue: Introduce xe_exec_queue_reinit() ---
commit a8c85ce636fedba930156524ab6e248c40456eee
Author: Raag Jadav <[email protected]>

drm/xe/exec_queue: Introduce xe_exec_queue_reinit()

In preparation of usecases which require re-initializing exec queue
after PCIe FLR, introduce xe_exec_queue_reinit() helper. All the exec
queue LCRs already exist but the context is lost on PCIe FLR and needs
re-initialization.

> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index 35b4e8289b5fe..79b363978ea49 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c

[ ... ]

> @@ -1486,8 +1486,19 @@ void xe_lrc_set_multi_queue_priority(struct xe_lrc *lrc, enum xe_multi_queue_pri
>  	lrc->desc |= FIELD_PREP(LRC_PRIORITY, xe_multi_queue_prio_to_lrc(lrc, priority));
>  }
>  
> -static int xe_lrc_ctx_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, struct xe_vm *vm,
> -			   void *replay_state, u16 msix_vec, u32 init_flags)
> +/**
> + * xe_lrc_ctx_init() - Initialize LRC with context details
> + * @lrc: Pointer to the LRC
> + * @hwe: Hardware Engine
> + * @vm: The VM (address space)
> + * @replay_state: GPU hang replay state
> + * @msix_vec: MSI-X interrupt vector (for platforms that support it)
> + * @init_flags: LRC initialization flags
> + *
> + * Returns: 0 on success, negative error code otherwise.
> + */
> +int xe_lrc_ctx_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, struct xe_vm *vm,
> +		    void *replay_state, u16 msix_vec, u32 init_flags)
>  {
>  	struct xe_gt *gt = hwe->gt;
>  	struct xe_tile *tile = gt_to_tile(gt);

[Severity: High]
This is a pre-existing issue, but can this code cause a NULL pointer
dereference when replay_state is set?

If a user space application calls the xe_exec_queue_create IOCTL with the
DRM_XE_EXEC_QUEUE_SET_HANG_REPLAY_STATE extension, q->replay_state is
populated. Inside xe_lrc_ctx_init(), the code checks:

    if (gt->default_lrc[hwe->class] || replay_state) {
        xe_map_memset(xe, &map, 0, 0, LRC_PPHWSP_SIZE);
        xe_map_memcpy_to(xe, &map, LRC_PPHWSP_SIZE,
                         gt->default_lrc[hwe->class] + LRC_PPHWSP_SIZE,
                         lrc->size - LRC_PPHWSP_SIZE);

If the queue is created on a platform where GuC submission is disabled,
gt->default_lrc is not populated during initialization and remains NULL.

Because replay_state is true, the condition passes, and we unconditionally
evaluate gt->default_lrc[hwe->class] + LRC_PPHWSP_SIZE as the source pointer
for xe_map_memcpy_to(), leading to a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5