[Public]
> -----Original Message-----
> From: Alex Deucher <alexdeuc...@gmail.com>
> Sent: Saturday, May 31, 2025 5:32 AM
> To: Liang, Prike <prike.li...@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; Deucher, Alexander
> <alexander.deuc...@amd.com>; Koenig, Christian <christian.koe...@amd.com>;
> Lazar, Lijo <lijo.la...@amd.com>
> Subject: Re: [PATCH 1/9] drm/amdgpu: validate userq input args
>
> On Fri, May 30, 2025 at 4:05 AM Prike Liang <prike.li...@amd.com> wrote:
> >
> > This will help on validating the userq input args, and rejecting for
> > the invalidate userq request at the IOCTLs first place.
> >
> > Signed-off-by: Prike Liang <prike.li...@amd.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 80
> > +++++++++++++++------- drivers/gpu/drm/amd/amdgpu/mes_userqueue.c |
> > 7 --
> > 2 files changed, 55 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > index 295e7186e156..f67969312c39 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> > @@ -359,27 +359,10 @@ amdgpu_userq_create(struct drm_file *filp, union
> drm_amdgpu_userq *args)
> > (args->in.flags &
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK) >>
> > AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_SHIFT;
> >
> > - /* Usermode queues are only supported for GFX IP as of now */
> > - if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
> > - args->in.ip_type != AMDGPU_HW_IP_DMA &&
> > - args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
> > - drm_file_err(uq_mgr->file, "Usermode queue doesn't support
> > IP
> type %u\n",
> > - args->in.ip_type);
> > - return -EINVAL;
> > - }
> > -
> > r = amdgpu_userq_priority_permit(filp, priority);
> > if (r)
> > return r;
> >
> > - if ((args->in.flags &
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE) &&
> > - (args->in.ip_type != AMDGPU_HW_IP_GFX) &&
> > - (args->in.ip_type != AMDGPU_HW_IP_COMPUTE) &&
> > - !amdgpu_is_tmz(adev)) {
> > - drm_file_err(uq_mgr->file, "Secure only supported on
> > GFX/Compute
> queues\n");
> > - return -EINVAL;
> > - }
> > -
> > r = pm_runtime_get_sync(adev_to_drm(adev)->dev);
> > if (r < 0) {
> > drm_file_err(uq_mgr->file, "pm_runtime_get_sync()
> > failed for userqueue create\n"); @@ -485,22 +468,44 @@
> amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
> > return r;
> > }
> >
> > -int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
> > - struct drm_file *filp)
> > +static int amdgpu_userq_input_args_validate(struct drm_device *dev,
> > + union drm_amdgpu_userq *args,
> > + struct drm_file *filp)
> > {
> > - union drm_amdgpu_userq *args = data;
> > - int r;
> > + struct amdgpu_device *adev = drm_to_adev(dev);
> >
> > switch (args->in.op) {
> > case AMDGPU_USERQ_OP_CREATE:
> > if (args->in.flags &
> ~(AMDGPU_USERQ_CREATE_FLAGS_QUEUE_PRIORITY_MASK |
> >
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE))
> > return -EINVAL;
> > - r = amdgpu_userq_create(filp, args);
> > - if (r)
> > - drm_file_err(filp, "Failed to create usermode
> > queue\n");
> > - break;
> > + /* Usermode queues are only supported for GFX IP as of now
> > */
> > + if (args->in.ip_type != AMDGPU_HW_IP_GFX &&
> > + args->in.ip_type != AMDGPU_HW_IP_DMA &&
> > + args->in.ip_type != AMDGPU_HW_IP_COMPUTE) {
> > + drm_file_err(filp, "Usermode queue doesn't support
> > IP type %u\n",
> > + args->in.ip_type);
> > + return -EINVAL;
> > + }
> > +
> > + if ((args->in.flags &
> AMDGPU_USERQ_CREATE_FLAGS_QUEUE_SECURE) &&
> > + (args->in.ip_type != AMDGPU_HW_IP_GFX) &&
> > + (args->in.ip_type != AMDGPU_HW_IP_COMPUTE) &&
> > + !amdgpu_is_tmz(adev)) {
> > + drm_file_err(filp, "Secure only supported on
> > GFX/Compute
> queues\n");
> > + return -EINVAL;
> > + }
> >
> > + if (args->in.queue_va == AMDGPU_BO_INVALID_OFFSET ||
> > + args->in.queue_size == 0) {
> > + drm_file_err(filp, "invalidate userq queue va or
> > size\n");
> > + return -EINVAL;
> > + }
> > + if (!args->in.wptr_va || !args->in.rptr_va) {
> > + drm_file_err(filp, "invalidate userq queue rptr or
> > wptr\n");
> > + return -EINVAL;
> > + }
> > + break;
> > case AMDGPU_USERQ_OP_FREE:
> > if (args->in.ip_type ||
> > args->in.doorbell_handle || @@ -514,6 +519,31 @@
> > int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
> > args->in.mqd ||
> > args->in.mqd_size)
> > return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
> > + struct drm_file *filp) {
> > + union drm_amdgpu_userq *args = data;
> > + int r;
> > +
> > + if (amdgpu_userq_input_args_validate(dev, args, filp) < 0)
> > + return -EINVAL;
> > +
> > + switch (args->in.op) {
> > + case AMDGPU_USERQ_OP_CREATE:
> > + r = amdgpu_userq_create(filp, args);
> > + if (r)
> > + drm_file_err(filp, "Failed to create usermode
> > queue\n");
> > + break;
> > +
> > + case AMDGPU_USERQ_OP_FREE:
> > r = amdgpu_userq_destroy(filp, args->in.queue_id);
> > if (r)
> > drm_file_err(filp, "Failed to destroy usermode
> > queue\n"); diff --git a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > index d6f50b13e2ba..1457fb49a794 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/mes_userqueue.c
> > @@ -215,13 +215,6 @@ static int mes_userq_mqd_create(struct
> amdgpu_userq_mgr *uq_mgr,
> > return -ENOMEM;
> > }
> >
> > - if (!mqd_user->wptr_va || !mqd_user->rptr_va ||
> > - !mqd_user->queue_va || mqd_user->queue_size == 0) {
> > - DRM_ERROR("Invalid MQD parameters for userqueue\n");
> > - r = -EINVAL;
> > - goto free_props;
> > - }
>
> I would keep this check here as this is MES specific at this point.
OK, previously I just want to validate the userq input parameters at the useq
IOCTL early time to avoid allocating unnecessary resource.
> Alex
>
> > -
> > r = amdgpu_userq_create_object(uq_mgr, &queue->mqd, mqd_hw_default-
> >mqd_size);
> > if (r) {
> > DRM_ERROR("Failed to create MQD object for
> > userqueue\n");
> > --
> > 2.34.1
> >
