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;
> +}
