Re: [PATCH v5 13/17] drm/imagination: Implement context creation/destruction ioctls

[Jann Horn]

On Wed, Aug 16, 2023 at 10:25 AM Sarah Walker <sarah.walker@xxxxxxxxxx> wrote:
>
> Implement ioctls for the creation and destruction of contexts. Contexts are
> used for job submission and each is associated with a particular job type.
[...]
> +/**
> + * pvr_context_create() - Create a context.
> + * @pvr_file: File to attach the created context to.
> + * @args: Context creation arguments.
> + *
> + * Return:
> + *  * 0 on success, or
> + *  * A negative error code on failure.
> + */
> +int pvr_context_create(struct pvr_file *pvr_file, struct drm_pvr_ioctl_create_context_args *args)
> +{
> +       struct pvr_device *pvr_dev = pvr_file->pvr_dev;
> +       struct pvr_context *ctx;
> +       int ctx_size;
> +       int err;
> +
> +       /* Context creation flags are currently unused and must be zero. */
> +       if (args->flags)
> +               return -EINVAL;
> +
> +       ctx_size = get_fw_obj_size(args->type);
> +       if (ctx_size < 0)
> +               return ctx_size;
> +
> +       ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +       if (!ctx)
> +               return -ENOMEM;
> +
> +       ctx->data_size = ctx_size;
> +       ctx->type = args->type;
> +       ctx->flags = args->flags;
> +       ctx->pvr_dev = pvr_dev;
> +       kref_init(&ctx->ref_count);
> +
> +       err = remap_priority(pvr_file, args->priority, &ctx->priority);
> +       if (err)
> +               goto err_free_ctx;
> +
> +       ctx->vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle);
> +       if (IS_ERR(ctx->vm_ctx)) {
> +               err = PTR_ERR(ctx->vm_ctx);
> +               goto err_free_ctx;
> +       }
> +
> +       ctx->data = kzalloc(ctx_size, GFP_KERNEL);
> +       if (!ctx->data) {
> +               err = -ENOMEM;
> +               goto err_put_vm;
> +       }
> +
> +       err = init_fw_objs(ctx, args, ctx->data);
> +       if (err)
> +               goto err_free_ctx_data;
> +
> +       err = pvr_fw_object_create(pvr_dev, ctx_size, PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
> +                                  ctx_fw_data_init, ctx, &ctx->fw_obj);
> +       if (err)
> +               goto err_free_ctx_data;
> +
> +       err = xa_alloc(&pvr_dev->ctx_ids, &ctx->ctx_id, ctx, xa_limit_32b, GFP_KERNEL);
> +       if (err)
> +               goto err_destroy_fw_obj;
> +
> +       err = xa_alloc(&pvr_file->ctx_handles, &args->handle, ctx, xa_limit_32b, GFP_KERNEL);
> +       if (err)
> +               goto err_release_id;

This bailout looks a bit dodgy. We have already inserted ctx into
&pvr_dev->ctx_ids, and now we just take it out again. If someone could
concurrently call pvr_context_lookup_id() on the ID we just allocated
(I don't understand enough about what's going on here at a high level
to be able to tell if that's possible), I think they would be able to
elevate the ctx->ref_count from 1 to 2, and then on the bailout path
we'll just free the ctx without looking at the refcount.

If this can't happen, it might be a good idea to add a comment
explaining why. If it can happen, I guess one way to fix it would be
to replace this last bailout with a call to pvr_context_put()?


> +
> +       return 0;
> +
> +err_release_id:
> +       xa_erase(&pvr_dev->ctx_ids, ctx->ctx_id);
> +
> +err_destroy_fw_obj:
> +       pvr_fw_object_destroy(ctx->fw_obj);
> +
> +err_free_ctx_data:
> +       kfree(ctx->data);
> +
> +err_put_vm:
> +       pvr_vm_context_put(ctx->vm_ctx);
> +
> +err_free_ctx:
> +       kfree(ctx);
> +       return err;
> +}