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

[Sarah Walker]

On Fri, 2023-08-18 at 00:42 +0200, Jann Horn wrote:
> *** CAUTION: This email originates from a source not known to Imagination Technologies. Think before you click a link or open an attachment ***
> 
> 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()?

Yes, I think you're correct here. I don't think there's anything in the current
patch set that can actually trigger this, but it definitely needs fixing.

Thanks,
Sarah

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