[Public]
It seems reasonable to introduce a unified helper that encapsulates fetching
and validating the raw MQD data for user queues, so that this logic is not
duplicated across call sites.
Regards,
Prike
> -----Original Message-----
> From: Junrui Luo <[email protected]>
> Sent: Monday, March 16, 2026 6:10 PM
> To: Liang, Prike <[email protected]>; [email protected]
> Cc: Deucher, Alexander <[email protected]>; Koenig, Christian
> <[email protected]>; David Airlie <[email protected]>; Simona Vetter
> <[email protected]>; [email protected];
> [email protected];
> [email protected]; Yuhao Jiang <[email protected]>;
> [email protected]
> Subject: Re: [PATCH] drm/amdgpu/userq: fix memory leak in MQD creation error
> paths
>
> On Sun, Mar 15, 2026 at 10:50:44AM +0100, Markus Elfring wrote:
> > If you would like to stick to the usage of goto labels so far, I see
> > further possibilities to avoid also duplicate source code for the
> > affected implementation of the function “mes_userq_mqd_create”.
> > https://elixir.bootlin.com/linux/v7.0-rc3/source/drivers/gpu/drm/amd/a
> > mdgpu/mes_userqueue.c#L275-L434
>
>
> On Mon, Mar 16, 2026 at 03:32:34AM +0000, Liang, Prike wrote:
> > Thanks for the fix. We could further refine this by wrapping a unified
> > helper for
> fetching and validating the userq MQD raw data.
>
> Thanks for the review and suggestions.
>
> I'm thinking of a follow-up patch that splits the branches into separate
> helper
> functions. Each function would use __free(kfree) to manage the memdup lifetime
> internally. The main function would only dispatch and forward errors to the
> existing
> goto chain.
>
> For instance:
>
> static void *mes_userq_mqd_read(struct drm_amdgpu_userq_in *mqd_user,
> size_t size, const char *ip_name)
> {
> void *mqd;
>
> if (mqd_user->mqd_size != size || !mqd_user->mqd) {
> DRM_ERROR("Invalid %s MQD\n", ip_name);
> return ERR_PTR(-EINVAL);
> }
>
> mqd = memdup_user(u64_to_user_ptr(mqd_user->mqd), size);
> if (IS_ERR(mqd)) {
> DRM_ERROR("Failed to read %s user MQD\n", ip_name);
> return ERR_PTR(-ENOMEM);
> }
>
> return mqd;
> }
>
> static int mes_userq_mqd_init_compute(struct amdgpu_device *adev,
> struct amdgpu_usermode_queue *queue,
> struct drm_amdgpu_userq_in *mqd_user,
> struct amdgpu_mqd_prop *userq_props) {
> struct drm_amdgpu_userq_mqd_compute_gfx11 *mqd __free(kfree) = NULL;
> int r;
>
> mqd = mes_userq_mqd_read(mqd_user, sizeof(*mqd), "compute");
> if (IS_ERR(mqd))
> return PTR_ERR(mqd);
>
> r = amdgpu_userq_input_va_validate(adev, queue, mqd->eop_va, 2048);
> if (r)
> return r;
>
> userq_props->eop_gpu_addr = mqd->eop_va;
> userq_props->hqd_pipe_priority = AMDGPU_GFX_PIPE_PRIO_NORMAL;
> userq_props->hqd_queue_priority =
> AMDGPU_GFX_QUEUE_PRIORITY_MINIMUM;
> userq_props->hqd_active = false;
> userq_props->tmz_queue =
> mqd_user->flags &
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE;
> return 0;
> }
>
> /* similarly for mes_userq_mqd_init_gfx/sdma */
>
> Then in mes_userq_mqd_create():
>
> if (queue->queue_type == AMDGPU_HW_IP_COMPUTE)
> r = mes_userq_mqd_init_compute(adev, queue, mqd_user,
> userq_props);
> else if (queue->queue_type == AMDGPU_HW_IP_GFX)
> r = mes_userq_mqd_init_gfx(adev, queue, mqd_user, userq_props); else if
> (queue->queue_type == AMDGPU_HW_IP_DMA)
> r = mes_userq_mqd_init_sdma(adev, queue, mqd_user, userq_props);
>
> if (r)
> goto free_mqd;
>
> Would this direction be acceptable as a follow-up?
>
> Thanks,
> Junrui Luo
