On Sat, Feb 1, 2020 at 5:26 PM Dan Carpenter dan.carpenter@oracle.com wrote:
On Sat, Feb 01, 2020 at 05:02:47PM +0800, Hillf Danton wrote:
On Sat, 1 Feb 2020 09:17:57 +0300 Dan Carpenter wrote:
On Sat, Feb 01, 2020 at 12:32:09PM +0800, Hillf Danton wrote:
Release obj in error path.
--- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -196,10 +196,10 @@ static struct drm_gem_object *vgem_gem_c return ERR_CAST(obj);
ret = drm_gem_handle_create(file, &obj->base, handle);
- drm_gem_object_put_unlocked(&obj->base);
- if (ret)
- if (ret) {
drm_gem_object_put_unlocked(&obj->base); return ERR_PTR(ret);
- } return &obj->base;
Oh yeah. It's weird that we never noticed the success path was broken. It's been that way for three years and no one noticed at all. Very strange.
Anyway, it already gets freed on error in drm_gem_handle_create() so we should just delete the drm_gem_object_put_unlocked() here it looks like.
There's two refcounts here, one is the handle_count, and the other is the underlying object refcount. I think the code is correct, except if you race with a 2nd thread which destroys the object (through the handle) while we still try to read gem_object->size in the caller of this. So correct fix (I think at least) is to shuffle that temporary reference on the object (not the handle) we hold while constructing it around a bit, so there's no use-after free anymore in the case of a race. I'm typing a patch for this.
Cheers, Daniel
Good catch, Dan :P Would you please post a patch sometime convenient next week?
Sure. Will do.
regards, dan carpenter