[PATCH] drm: Don't reference objects in the flink name idr

[Kristian Høgsberg]

On Tue, Dec 3, 2013 at 7:26 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Mon, Dec 02, 2013 at 05:36:17PM -0800, Kristian Høgsberg wrote:
>> There's no reason to keep a reference to objects in the name idr.  Each
>> handle to an object has a reference to the object and just before we
>> destroy the last handle we take the object out of the name idr.  Thus,
>> if an object is in the name idr, there's at least one reference to the
>> object.
>>
>> Or to put it another way, the name idr reference will never keep the
>> object alive.  It just looks like it, which is confusing.
>>
>> Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
>
> I expect this to blow up when you race gem_close ioctl calls with flink
> open. i-g-t/gem_flink_close tests actually have been written specifically
> to exercise these races.
> -Daniel

Can you be more specific about what race you see?  The one thing that
could go wrong is that the last handle is delete after we enter
drm_gem_flink_ioctl() and look up the object but before taking the
object_name_lock, but that's handled by checking obj->handle_count
under the lock.  Deleting handles and removing the name is always done
under the object_name_lock from
drm_gem_object_handle_unreference_unlocked().

Kristian

>> ---
>>  drivers/gpu/drm/drm_gem.c | 15 ---------------
>>  1 file changed, 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 4761ade..3ff39bb 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -175,11 +175,6 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
>>       mutex_unlock(&filp->prime.lock);
>>  }
>>
>> -static void drm_gem_object_ref_bug(struct kref *list_kref)
>> -{
>> -     BUG();
>> -}
>> -
>>  /**
>>   * Called after the last handle to the object has been closed
>>   *
>> @@ -195,13 +190,6 @@ static void drm_gem_object_handle_free(struct drm_gem_object *obj)
>>       if (obj->name) {
>>               idr_remove(&dev->object_name_idr, obj->name);
>>               obj->name = 0;
>> -             /*
>> -              * The object name held a reference to this object, drop
>> -              * that now.
>> -             *
>> -             * This cannot be the last reference, since the handle holds one too.
>> -              */
>> -             kref_put(&obj->refcount, drm_gem_object_ref_bug);
>>       }
>>  }
>>
>> @@ -602,9 +590,6 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
>>                       goto err;
>>
>>               obj->name = ret;
>> -
>> -             /* Allocate a reference for the name table.  */
>> -             drm_gem_object_reference(obj);
>>       }
>>
>>       args->name = (uint64_t) obj->name;
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch