[PATCH] drm/nouveau: fix error handling in core/core object creation functions

[PATCH] drm/nouveau: fix error handling in core/core object creation functions

[Marcin Slusarz]

Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
This patch relies on "drm/nouveau: remove >1 sclass support from nouveau_parent_create_".

There are *many* *more* code paths without proper error handling - I counted
at least 106 in 41 functions. If someone would like to do a bit of janitorial
work I marked those code paths and uploaded "patch" here:
http://people.freedesktop.org/~mslusarz/0001-codepaths-without-error-handling.patch
(Please let me know if you are going to fix those to not duplicate work)
---
 drivers/gpu/drm/nouveau/core/core/engine.c | 1 +
 drivers/gpu/drm/nouveau/core/core/gpuobj.c | 9 ++++++---
 drivers/gpu/drm/nouveau/core/core/parent.c | 4 +++-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/core/core/engine.c b/drivers/gpu/drm/nouveau/core/core/engine.c
index 09b3bd5..4319854 100644
--- a/drivers/gpu/drm/nouveau/core/core/engine.c
+++ b/drivers/gpu/drm/nouveau/core/core/engine.c
@@ -44,6 +44,7 @@ nouveau_engine_create_(struct nouveau_object *parent,
 		return ret;
 
 	if (!nouveau_boolopt(device->cfgopt, iname, enable)) {
+		nouveau_subdev_destroy(&engine->base);
 		if (!enable)
 			nv_warn(engine, "disabled, %s=1 to enable\n", iname);
 		return -ENODEV;
diff --git a/drivers/gpu/drm/nouveau/core/core/gpuobj.c b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
index c2a7608..6254d52 100644
--- a/drivers/gpu/drm/nouveau/core/core/gpuobj.c
+++ b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
@@ -40,7 +40,7 @@ nouveau_gpuobj_destroy(struct nouveau_gpuobj *gpuobj)
 	}
 
 	if (gpuobj->heap.block_size)
-		nouveau_mm_fini(&gpuobj->heap);
+		WARN_ON(nouveau_mm_fini(&gpuobj->heap));
 
 	nouveau_object_destroy(&gpuobj->base);
 }
@@ -113,7 +113,7 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
 		ret = nouveau_mm_head(heap, 1, size, size,
 				      max(align, (u32)1), &gpuobj->node);
 		if (ret)
-			return ret;
+			goto err;
 
 		gpuobj->addr += gpuobj->node->offset;
 	}
@@ -121,7 +121,7 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
 	if (gpuobj->flags & NVOBJ_FLAG_HEAP) {
 		ret = nouveau_mm_init(&gpuobj->heap, 0, gpuobj->size, 1);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	if (flags & NVOBJ_FLAG_ZERO_ALLOC) {
@@ -130,6 +130,9 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
 	}
 
 	return ret;
+err:
+	nouveau_gpuobj_destroy(gpuobj);
+	return ret;
 }
 
 struct nouveau_gpuobj_class {
diff --git a/drivers/gpu/drm/nouveau/core/core/parent.c b/drivers/gpu/drm/nouveau/core/core/parent.c
index 1a48e58..d2ea7c2 100644
--- a/drivers/gpu/drm/nouveau/core/core/parent.c
+++ b/drivers/gpu/drm/nouveau/core/core/parent.c
@@ -87,8 +87,10 @@ nouveau_parent_create_(struct nouveau_object *parent,
 
 	if (sclass && sclass->ofuncs) {
 		nclass = kzalloc(sizeof(*nclass), GFP_KERNEL);
-		if (!nclass)
+		if (!nclass) {
+			nouveau_parent_destroy(object);
 			return -ENOMEM;
+		}
 
 		nclass->sclass = object->sclass;
 		object->sclass = nclass;
-- 
1.7.12

Re: [PATCH] drm/nouveau: fix error handling in core/core object creation functions

[Marcin Slusarz]

On Mon, Oct 08, 2012 at 12:49:31AM +0200, Marcin Slusarz wrote:
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> This patch relies on "drm/nouveau: remove >1 sclass support from nouveau_parent_create_".
> 
> There are *many* *more* code paths without proper error handling - I counted
> at least 106 in 41 functions. If someone would like to do a bit of janitorial
> work I marked those code paths and uploaded "patch" here:
> http://people.freedesktop.org/~mslusarz/0001-codepaths-without-error-handling.patch
> (Please let me know if you are going to fix those to not duplicate work)
> ---
>  drivers/gpu/drm/nouveau/core/core/engine.c | 1 +
>  drivers/gpu/drm/nouveau/core/core/gpuobj.c | 9 ++++++---
>  drivers/gpu/drm/nouveau/core/core/parent.c | 4 +++-
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/core/core/engine.c b/drivers/gpu/drm/nouveau/core/core/engine.c
> index 09b3bd5..4319854 100644
> --- a/drivers/gpu/drm/nouveau/core/core/engine.c
> +++ b/drivers/gpu/drm/nouveau/core/core/engine.c
> @@ -44,6 +44,7 @@ nouveau_engine_create_(struct nouveau_object *parent,
>  		return ret;
>  
>  	if (!nouveau_boolopt(device->cfgopt, iname, enable)) {
> +		nouveau_subdev_destroy(&engine->base);
>  		if (!enable)
>  			nv_warn(engine, "disabled, %s=1 to enable\n", iname);
>  		return -ENODEV;
> diff --git a/drivers/gpu/drm/nouveau/core/core/gpuobj.c b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
> index c2a7608..6254d52 100644
> --- a/drivers/gpu/drm/nouveau/core/core/gpuobj.c
> +++ b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
> @@ -40,7 +40,7 @@ nouveau_gpuobj_destroy(struct nouveau_gpuobj *gpuobj)
>  	}
>  
>  	if (gpuobj->heap.block_size)
> -		nouveau_mm_fini(&gpuobj->heap);
> +		WARN_ON(nouveau_mm_fini(&gpuobj->heap));

Ha! This is triggering on channel close. Maybe it's the leak which is
preventing piglit from finishing...

>  
>  	nouveau_object_destroy(&gpuobj->base);
>  }
> @@ -113,7 +113,7 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
>  		ret = nouveau_mm_head(heap, 1, size, size,
>  				      max(align, (u32)1), &gpuobj->node);
>  		if (ret)
> -			return ret;
> +			goto err;
>  
>  		gpuobj->addr += gpuobj->node->offset;
>  	}
> @@ -121,7 +121,7 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
>  	if (gpuobj->flags & NVOBJ_FLAG_HEAP) {
>  		ret = nouveau_mm_init(&gpuobj->heap, 0, gpuobj->size, 1);
>  		if (ret)
> -			return ret;
> +			goto err;
>  	}
>  
>  	if (flags & NVOBJ_FLAG_ZERO_ALLOC) {
> @@ -130,6 +130,9 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
>  	}
>  
>  	return ret;
> +err:
> +	nouveau_gpuobj_destroy(gpuobj);
> +	return ret;
>  }
>  
>  struct nouveau_gpuobj_class {
> diff --git a/drivers/gpu/drm/nouveau/core/core/parent.c b/drivers/gpu/drm/nouveau/core/core/parent.c
> index 1a48e58..d2ea7c2 100644
> --- a/drivers/gpu/drm/nouveau/core/core/parent.c
> +++ b/drivers/gpu/drm/nouveau/core/core/parent.c
> @@ -87,8 +87,10 @@ nouveau_parent_create_(struct nouveau_object *parent,
>  
>  	if (sclass && sclass->ofuncs) {
>  		nclass = kzalloc(sizeof(*nclass), GFP_KERNEL);
> -		if (!nclass)
> +		if (!nclass) {
> +			nouveau_parent_destroy(object);
>  			return -ENOMEM;
> +		}
>  
>  		nclass->sclass = object->sclass;
>  		object->sclass = nclass;
> -- 
> 1.7.12
> 

Re: [PATCH] drm/nouveau: fix error handling in core/core object creation functions

[Ben Skeggs]

On Mon, Oct 08, 2012 at 12:49:31AM +0200, Marcin Slusarz wrote:
> Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> This patch relies on "drm/nouveau: remove >1 sclass support from nouveau_parent_create_".
> 
> There are *many* *more* code paths without proper error handling -
This is *not* a bug.  An object's constructor should be called via
nouveau_object_ctor(), which has the semantics that the constructor
returns and error *and* a pointer returned via pobject, then the
class's destructor will be called to cleanup.

Ben.

> I counted
> at least 106 in 41 functions. If someone would like to do a bit of janitorial
> work I marked those code paths and uploaded "patch" here:
> http://people.freedesktop.org/~mslusarz/0001-codepaths-without-error-handling.patch
> (Please let me know if you are going to fix those to not duplicate work)
> ---
>  drivers/gpu/drm/nouveau/core/core/engine.c | 1 +
>  drivers/gpu/drm/nouveau/core/core/gpuobj.c | 9 ++++++---
>  drivers/gpu/drm/nouveau/core/core/parent.c | 4 +++-
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/core/core/engine.c b/drivers/gpu/drm/nouveau/core/core/engine.c
> index 09b3bd5..4319854 100644
> --- a/drivers/gpu/drm/nouveau/core/core/engine.c
> +++ b/drivers/gpu/drm/nouveau/core/core/engine.c
> @@ -44,6 +44,7 @@ nouveau_engine_create_(struct nouveau_object *parent,
>  		return ret;
>  
>  	if (!nouveau_boolopt(device->cfgopt, iname, enable)) {
> +		nouveau_subdev_destroy(&engine->base);
>  		if (!enable)
>  			nv_warn(engine, "disabled, %s=1 to enable\n", iname);
>  		return -ENODEV;
> diff --git a/drivers/gpu/drm/nouveau/core/core/gpuobj.c b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
> index c2a7608..6254d52 100644
> --- a/drivers/gpu/drm/nouveau/core/core/gpuobj.c
> +++ b/drivers/gpu/drm/nouveau/core/core/gpuobj.c
> @@ -40,7 +40,7 @@ nouveau_gpuobj_destroy(struct nouveau_gpuobj *gpuobj)
>  	}
>  
>  	if (gpuobj->heap.block_size)
> -		nouveau_mm_fini(&gpuobj->heap);
> +		WARN_ON(nouveau_mm_fini(&gpuobj->heap));
>  
>  	nouveau_object_destroy(&gpuobj->base);
>  }
> @@ -113,7 +113,7 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
>  		ret = nouveau_mm_head(heap, 1, size, size,
>  				      max(align, (u32)1), &gpuobj->node);
>  		if (ret)
> -			return ret;
> +			goto err;
>  
>  		gpuobj->addr += gpuobj->node->offset;
>  	}
> @@ -121,7 +121,7 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
>  	if (gpuobj->flags & NVOBJ_FLAG_HEAP) {
>  		ret = nouveau_mm_init(&gpuobj->heap, 0, gpuobj->size, 1);
>  		if (ret)
> -			return ret;
> +			goto err;
>  	}
>  
>  	if (flags & NVOBJ_FLAG_ZERO_ALLOC) {
> @@ -130,6 +130,9 @@ nouveau_gpuobj_create_(struct nouveau_object *parent,
>  	}
>  
>  	return ret;
> +err:
> +	nouveau_gpuobj_destroy(gpuobj);
> +	return ret;
>  }
>  
>  struct nouveau_gpuobj_class {
> diff --git a/drivers/gpu/drm/nouveau/core/core/parent.c b/drivers/gpu/drm/nouveau/core/core/parent.c
> index 1a48e58..d2ea7c2 100644
> --- a/drivers/gpu/drm/nouveau/core/core/parent.c
> +++ b/drivers/gpu/drm/nouveau/core/core/parent.c
> @@ -87,8 +87,10 @@ nouveau_parent_create_(struct nouveau_object *parent,
>  
>  	if (sclass && sclass->ofuncs) {
>  		nclass = kzalloc(sizeof(*nclass), GFP_KERNEL);
> -		if (!nclass)
> +		if (!nclass) {
> +			nouveau_parent_destroy(object);
>  			return -ENOMEM;
> +		}
>  
>  		nclass->sclass = object->sclass;
>  		object->sclass = nclass;
> -- 
> 1.7.12
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH] drm/nouveau: fix error handling in core/core object creation functions

[Marcin Slusarz]

On Mon, Oct 08, 2012 at 11:05:33AM +1000, Ben Skeggs wrote:
> On Mon, Oct 08, 2012 at 12:49:31AM +0200, Marcin Slusarz wrote:
> > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > This patch relies on "drm/nouveau: remove >1 sclass support from nouveau_parent_create_".
> > 
> > There are *many* *more* code paths without proper error handling -
> This is *not* a bug.  An object's constructor should be called via
> nouveau_object_ctor(), which has the semantics that the constructor
> returns and error *and* a pointer returned via pobject, then the
> class's destructor will be called to cleanup.

That's... clever, and crazy, and unlike anything in kernel land...

Please put a comment near ctor field in nouveau_ofuncs to make this
information easier to discover, without need to review the whole call chain...

Marcin

Re: [PATCH] drm/nouveau: fix error handling in core/core object creation functions

[Ben Skeggs]

On Tue, Oct 09, 2012 at 12:50:59AM +0200, Marcin Slusarz wrote:
> On Mon, Oct 08, 2012 at 11:05:33AM +1000, Ben Skeggs wrote:
> > On Mon, Oct 08, 2012 at 12:49:31AM +0200, Marcin Slusarz wrote:
> > > Signed-off-by: Marcin Slusarz <marcin.slusarz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > ---
> > > This patch relies on "drm/nouveau: remove >1 sclass support from nouveau_parent_create_".
> > > 
> > > There are *many* *more* code paths without proper error handling -
> > This is *not* a bug.  An object's constructor should be called via
> > nouveau_object_ctor(), which has the semantics that the constructor
> > returns and error *and* a pointer returned via pobject, then the
> > class's destructor will be called to cleanup.
> 
> That's... clever, and crazy, and unlike anything in kernel land...
Yep, I know.  It made sense to me, rather than having two copies of the
destructor code, one of them filled with gotos :)

> 
> Please put a comment near ctor field in nouveau_ofuncs to make this
> information easier to discover, without need to review the whole call chain...
Alright, I'll do something to that effect.  I have been adding comments
as I go to some WIP stuff I have going on too.

Ben.
> 
> Marcin