Re: patch drm-radeon-kms-fix-null-pointer-dereference-if-memory-allocation-failed.patch added to 2.6.33-stable tree

Re: patch drm-radeon-kms-fix-null-pointer-dereference-if-memory-allocation-failed.patch added to 2.6.33-stable tree

[Stefan Lippers-Hollmann]

Hi

On Tuesday 06 April 2010, gregkh@suse.de wrote:
> This is a note to let you know that we have just queued up the patch titled
> 
>     Subject: drm/radeon/kms: Fix NULL pointer dereference if memory allocation failed.
> 
> to the 2.6.33-stable tree.  Its filename is
> 
>     drm-radeon-kms-fix-null-pointer-dereference-if-memory-allocation-failed.patch
> 
> A git repo of this tree can be found at 
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> 
> From fcbc451ba1948fba967198bd150ecbd10bbb7075 Mon Sep 17 00:00:00 2001
> From: Pauli Nieminen <suokkos@gmail.com>
> Date: Fri, 19 Mar 2010 07:44:33 +0000
> Subject: drm/radeon/kms: Fix NULL pointer dereference if memory allocation failed.
> 
> From: Pauli Nieminen <suokkos@gmail.com>
> 
> commit fcbc451ba1948fba967198bd150ecbd10bbb7075 upstream.
> 
> When there is allocation failure in radeon_cs_parser_relocs parser->nrelocs
> is not cleaned. This causes NULL pointer defeference in radeon_cs_parser_fini
> when clean up code is trying to loop over the relocation array and free the
> objects.
> 
> Fix adds a check for a possible NULL pointer in clean up code.
[...]

This patch breaks compiling kernel 2.6.33 + the current stable queue:

  CC [M]  drivers/gpu/drm/radeon/radeon_cs.o
/tmp/buildd/linux-sidux-2.6-2.6.33/debian/build/source_amd64_none/drivers/gpu/drm/radeon/radeon_cs.c: In function 'radeon_cs_parser_fini':
/tmp/buildd/linux-sidux-2.6-2.6.33/debian/build/source_amd64_none/drivers/gpu/drm/radeon/radeon_cs.c:200: error: implicit declaration of function 'drm_gem_object_unreference_unlocked'
make[6]: *** [drivers/gpu/drm/radeon/radeon_cs.o] Error 1

as it depends on the introduction of drm_gem_object_unreference_unlocked()
in:

Commit:     c3ae90c099bb62387507e86da7cf799850444b08
Author:     Luca Barbieri <luca@luca-barbieri.com>
AuthorDate: Tue Feb 9 05:49:11 2010 +0000

    drm: introduce drm_gem_object_[handle_]unreference_unlocked
    
    This patch introduces the drm_gem_object_unreference_unlocked
    and drm_gem_object_handle_unreference_unlocked functions that
    do not require holding struct_mutex.
    
    drm_gem_object_unreference_unlocked calls the new
    ->gem_free_object_unlocked entry point if available, and
    otherwise just takes struct_mutex and just calls ->gem_free_object

which in turn suggests:

Commit:     bc9025bdc4e2b591734cca17697093845007b63d
Author:     Luca Barbieri <luca@luca-barbieri.com>
AuthorDate: Tue Feb 9 05:49:12 2010 +0000

    Use drm_gem_object_[handle_]unreference_unlocked where possible
    
    Mostly obvious simplifications.
    
    The i915 pread/pwrite ioctls, intel_overlay_put_image and
    nouveau_gem_new were incorrectly using the locked versions
    without locking: this is also fixed in this patch.

which don't really look like candidates for 2.6.33-stable.

> --- a/drivers/gpu/drm/radeon/radeon_cs.c
> +++ b/drivers/gpu/drm/radeon/radeon_cs.c
> @@ -193,11 +193,13 @@ static void radeon_cs_parser_fini(struct
>  		radeon_bo_list_fence(&parser->validated, parser->ib->fence);
>  	}
>  	radeon_bo_list_unreserve(&parser->validated);
> -	for (i = 0; i < parser->nrelocs; i++) {
> -		if (parser->relocs[i].gobj) {
> -			mutex_lock(&parser->rdev->ddev->struct_mutex);
> -			drm_gem_object_unreference(parser->relocs[i].gobj);
> -			mutex_unlock(&parser->rdev->ddev->struct_mutex);
> +	if (parser->relocs != NULL) {
   	^ the only important part, the rest merely covers the new indentation 
  	  level

> +		for (i = 0; i < parser->nrelocs; i++) {
> +			if (parser->relocs[i].gobj) {
> +				mutex_lock(&parser->rdev->ddev->struct_mutex);
> +				drm_gem_object_unreference_unlocked(parser->relocs[i].gobj);
  				^ drm_gem_object_unreference_unlocked() doesn't exist in 2.6.33, yet
  				  we can use drm_gem_object_unreference() instead.

> +				mutex_unlock(&parser->rdev->ddev->struct_mutex);
> +			}
>  		}
>  	}
>  	kfree(parser->track);

As a consequence, I'd suggest to merely backport the NULL pointer check,
while ignoring the simplification of using the newly introduced 
drm_gem_object_unreference_unlocked() from 2.6.34:

Signed-off-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>

--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -193,11 +193,13 @@ static void radeon_cs_parser_fini(struct
 		radeon_bo_list_fence(&parser->validated, parser->ib->fence);
 	}
 	radeon_bo_list_unreserve(&parser->validated);
-	for (i = 0; i < parser->nrelocs; i++) {
-		if (parser->relocs[i].gobj) {
-			mutex_lock(&parser->rdev->ddev->struct_mutex);
-			drm_gem_object_unreference(parser->relocs[i].gobj);
-			mutex_unlock(&parser->rdev->ddev->struct_mutex);
+	if (parser->relocs != NULL) {
+		for (i = 0; i < parser->nrelocs; i++) {
+			if (parser->relocs[i].gobj) {
+				mutex_lock(&parser->rdev->ddev->struct_mutex);
+				drm_gem_object_unreference(parser->relocs[i].gobj);
+				mutex_unlock(&parser->rdev->ddev->struct_mutex);
+			}
 		}
 	}
 	kfree(parser->track);

Re: patch drm-radeon-kms-fix-null-pointer-dereference-if-memory-allocation-failed.patch added to 2.6.33-stable tree

[Greg KH]

On Tue, Apr 06, 2010 at 11:45:38PM +0200, Stefan Lippers-Hollmann wrote:
> Hi
> 
> On Tuesday 06 April 2010, gregkh@suse.de wrote:
> > This is a note to let you know that we have just queued up the patch titled
> > 
> >     Subject: drm/radeon/kms: Fix NULL pointer dereference if memory allocation failed.
> > 
> > to the 2.6.33-stable tree.  Its filename is
> > 
> >     drm-radeon-kms-fix-null-pointer-dereference-if-memory-allocation-failed.patch
> > 
> > A git repo of this tree can be found at 
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > 
> > From fcbc451ba1948fba967198bd150ecbd10bbb7075 Mon Sep 17 00:00:00 2001
> > From: Pauli Nieminen <suokkos@gmail.com>
> > Date: Fri, 19 Mar 2010 07:44:33 +0000
> > Subject: drm/radeon/kms: Fix NULL pointer dereference if memory allocation failed.
> > 
> > From: Pauli Nieminen <suokkos@gmail.com>
> > 
> > commit fcbc451ba1948fba967198bd150ecbd10bbb7075 upstream.
> > 
> > When there is allocation failure in radeon_cs_parser_relocs parser->nrelocs
> > is not cleaned. This causes NULL pointer defeference in radeon_cs_parser_fini
> > when clean up code is trying to loop over the relocation array and free the
> > objects.
> > 
> > Fix adds a check for a possible NULL pointer in clean up code.
> [...]
> 
> This patch breaks compiling kernel 2.6.33 + the current stable queue:
> 
>   CC [M]  drivers/gpu/drm/radeon/radeon_cs.o
> /tmp/buildd/linux-sidux-2.6-2.6.33/debian/build/source_amd64_none/drivers/gpu/drm/radeon/radeon_cs.c: In function 'radeon_cs_parser_fini':
> /tmp/buildd/linux-sidux-2.6-2.6.33/debian/build/source_amd64_none/drivers/gpu/drm/radeon/radeon_cs.c:200: error: implicit declaration of function 'drm_gem_object_unreference_unlocked'
> make[6]: *** [drivers/gpu/drm/radeon/radeon_cs.o] Error 1
> 
> as it depends on the introduction of drm_gem_object_unreference_unlocked()
> in:
> 
> Commit:     c3ae90c099bb62387507e86da7cf799850444b08
> Author:     Luca Barbieri <luca@luca-barbieri.com>
> AuthorDate: Tue Feb 9 05:49:11 2010 +0000
> 
>     drm: introduce drm_gem_object_[handle_]unreference_unlocked
>     
>     This patch introduces the drm_gem_object_unreference_unlocked
>     and drm_gem_object_handle_unreference_unlocked functions that
>     do not require holding struct_mutex.
>     
>     drm_gem_object_unreference_unlocked calls the new
>     ->gem_free_object_unlocked entry point if available, and
>     otherwise just takes struct_mutex and just calls ->gem_free_object
> 
> which in turn suggests:
> 
> Commit:     bc9025bdc4e2b591734cca17697093845007b63d
> Author:     Luca Barbieri <luca@luca-barbieri.com>
> AuthorDate: Tue Feb 9 05:49:12 2010 +0000
> 
>     Use drm_gem_object_[handle_]unreference_unlocked where possible
>     
>     Mostly obvious simplifications.
>     
>     The i915 pread/pwrite ioctls, intel_overlay_put_image and
>     nouveau_gem_new were incorrectly using the locked versions
>     without locking: this is also fixed in this patch.
> 
> which don't really look like candidates for 2.6.33-stable.
> 
> > --- a/drivers/gpu/drm/radeon/radeon_cs.c
> > +++ b/drivers/gpu/drm/radeon/radeon_cs.c
> > @@ -193,11 +193,13 @@ static void radeon_cs_parser_fini(struct
> >  		radeon_bo_list_fence(&parser->validated, parser->ib->fence);
> >  	}
> >  	radeon_bo_list_unreserve(&parser->validated);
> > -	for (i = 0; i < parser->nrelocs; i++) {
> > -		if (parser->relocs[i].gobj) {
> > -			mutex_lock(&parser->rdev->ddev->struct_mutex);
> > -			drm_gem_object_unreference(parser->relocs[i].gobj);
> > -			mutex_unlock(&parser->rdev->ddev->struct_mutex);
> > +	if (parser->relocs != NULL) {
>    	^ the only important part, the rest merely covers the new indentation 
>   	  level
> 
> > +		for (i = 0; i < parser->nrelocs; i++) {
> > +			if (parser->relocs[i].gobj) {
> > +				mutex_lock(&parser->rdev->ddev->struct_mutex);
> > +				drm_gem_object_unreference_unlocked(parser->relocs[i].gobj);
>   				^ drm_gem_object_unreference_unlocked() doesn't exist in 2.6.33, yet
>   				  we can use drm_gem_object_unreference() instead.
> 
> > +				mutex_unlock(&parser->rdev->ddev->struct_mutex);
> > +			}
> >  		}
> >  	}
> >  	kfree(parser->track);
> 
> As a consequence, I'd suggest to merely backport the NULL pointer check,
> while ignoring the simplification of using the newly introduced 
> drm_gem_object_unreference_unlocked() from 2.6.34:
> 
> Signed-off-by: Stefan Lippers-Hollmann <s.l-h@gmx.de>

Very cool, thanks for doing this, I just noticed it failing to build
here as well when you sent this.

I've queued up your version as it makes more sense.

thanks,

greg k-h