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

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

[Kristian Høgsberg]

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@bitplanet.net>
---
 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@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

> ---
>  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@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

Re: [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@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@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Dec 03, 2013 at 08:33:46AM -0800, Kristian Høgsberg wrote:
> On Tue, Dec 3, 2013 at 7:26 AM, Daniel Vetter <daniel@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@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().

Too many nightmares around lifetime rules, I've imagined some monster that
isn't ther and stand corrected.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch