Re: [PATCH v15 11/19] drm/etnaviv: Add etnaviv_gem_obj_remove() helper

[Sui Jingfeng <sui.jingfeng@linux.dev>]

Hi,

On 2024/10/1 22:21, Lucas Stach wrote:
> Am Sonntag, dem 08.09.2024 um 17:43 +0800 schrieb Sui Jingfeng:
>> Which is corresonding to the etnaviv_gem_obj_add()
>>
> While symmetry is nice,


Thanks a lot for understanding and review my patch.


> it's still not really symmetric,

patch 0016 will try try to make it symmetric.
It will do this uniformly for all etnaviv GEM buffer objects.


> as this
> function isn't exported into the PRIME parts of the driver like
> etnaviv_gem_obj_add().

Yes, exactly.

> Given that I don't really see how this patch
> improves code legibility.

When the reference counter of a GEM buffer object decrease to 0,
the drm_gem_object_free() will be get called. which in turn,
etnaviv_gem_free_object() will get called.

The etnaviv_gem_free_object() will remove the GEM BO node
from the 'priv->gem_list' without checking if it has been
added into the list.

The data field of the struct etnaviv_gem_object::gem_node
will be all ZERO under such a case.

When drm/etnaviv import a shared buffer from an another driver.
etnaviv_gem_prime_import_sg_table() will be get called. But it
could fails before the "etnaviv_gem_obj_add(dev, &etnaviv_obj->base)"
get executed. The fails might either due to out of memory or
drm_prime_sg_to_page_array() failed.


Those fails will lead to NULL pointer de-reference, as we will
use uninitialized data member(say the 'gem_node') of an GEM
buffer object.

This is also the reason why we want to add it into the
etnaviv_drm_private::gem_list immediately after an etnaviv
GEM buffer object is successfully created.

> Regards,
> Lucas
>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index 39cfece67b90..3732288ff530 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -19,6 +19,8 @@
>>   static struct lock_class_key etnaviv_shm_lock_class;
>>   static struct lock_class_key etnaviv_userptr_lock_class;
>>   
>> +static void etnaviv_gem_obj_remove(struct drm_gem_object *obj);
>> +
>>   static void etnaviv_gem_scatter_map(struct etnaviv_gem_object *etnaviv_obj)
>>   {
>>   	struct drm_device *dev = etnaviv_obj->base.dev;
>> @@ -555,15 +557,12 @@ void etnaviv_gem_free_object(struct drm_gem_object *obj)
>>   {
>>   	struct drm_device *drm = obj->dev;
>>   	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
>> -	struct etnaviv_drm_private *priv = obj->dev->dev_private;
>>   	struct etnaviv_vram_mapping *mapping, *tmp;
>>   
>>   	/* object should not be active */
>>   	drm_WARN_ON(drm, is_active(etnaviv_obj));
>>   
>> -	mutex_lock(&priv->gem_lock);
>> -	list_del(&etnaviv_obj->gem_node);
>> -	mutex_unlock(&priv->gem_lock);
>> +	etnaviv_gem_obj_remove(obj);
>>   
>>   	list_for_each_entry_safe(mapping, tmp, &etnaviv_obj->vram_list,
>>   				 obj_node) {
>> @@ -595,6 +594,16 @@ void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
>>   	mutex_unlock(&priv->gem_lock);
>>   }
>>   
>> +static void etnaviv_gem_obj_remove(struct drm_gem_object *obj)
>> +{
>> +	struct etnaviv_drm_private *priv = to_etnaviv_priv(obj->dev);
>> +	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
>> +
>> +	mutex_lock(&priv->gem_lock);
>> +	list_del(&etnaviv_obj->gem_node);
>> +	mutex_unlock(&priv->gem_lock);
>> +}
>> +
>>   static const struct vm_operations_struct vm_ops = {
>>   	.fault = etnaviv_gem_fault,
>>   	.open = drm_gem_vm_open,

-- 
Best regards,
Sui