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