[PATCH] drm/mm: avoid possible null pointer dereference

[PATCH] drm/mm: avoid possible null pointer dereference

[Heinrich Schuchardt]

Do not dereference node before the check if node is NULL.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/gpu/drm/drm_mm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 04de6fd..cb39f45 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
 int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
 {
 	struct drm_mm_node *hole;
-	u64 end = node->start + node->size;
+	u64 end;
 	u64 hole_start;
 	u64 hole_end;
 
 	BUG_ON(node == NULL);
 
+	end = node->start + node->size;
+
 	/* Find the relevant hole to add our node to */
 	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
 		if (hole_start > node->start || hole_end < end)
-- 
2.1.4

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Eric Engestrom]

On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> Do not dereference node before the check if node is NULL.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/gpu/drm/drm_mm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index 04de6fd..cb39f45 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
>  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
>  {
>  	struct drm_mm_node *hole;
> -	u64 end = node->start + node->size;
> +	u64 end;
>  	u64 hole_start;
>  	u64 hole_end;
>  
>  	BUG_ON(node == NULL);
>  
> +	end = node->start + node->size;
> +
>  	/* Find the relevant hole to add our node to */
>  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
>  		if (hole_start > node->start || hole_end < end)
> -- 
> 2.1.4

Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Eric Engestrom]

On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> Do not dereference node before the check if node is NULL.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/gpu/drm/drm_mm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index 04de6fd..cb39f45 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
>  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
>  {
>  	struct drm_mm_node *hole;
> -	u64 end = node->start + node->size;
> +	u64 end;
>  	u64 hole_start;
>  	u64 hole_end;
>  
>  	BUG_ON(node == NULL);
>  
> +	end = node->start + node->size;
> +
>  	/* Find the relevant hole to add our node to */
>  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
>  		if (hole_start > node->start || hole_end < end)
> -- 
> 2.1.4

Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Daniel Vetter]

On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > Do not dereference node before the check if node is NULL.
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  drivers/gpu/drm/drm_mm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > index 04de6fd..cb39f45 100644
> > --- a/drivers/gpu/drm/drm_mm.c
> > +++ b/drivers/gpu/drm/drm_mm.c
> > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> >  {
> >  	struct drm_mm_node *hole;
> > -	u64 end = node->start + node->size;
> > +	u64 end;
> >  	u64 hole_start;
> >  	u64 hole_end;
> >  
> >  	BUG_ON(node == NULL);
> >  
> > +	end = node->start + node->size;
> > +
> >  	/* Find the relevant hole to add our node to */
> >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> >  		if (hole_start > node->start || hole_end < end)
> > -- 
> > 2.1.4
> 
> Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

Applied to drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Daniel Vetter]

On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > Do not dereference node before the check if node is NULL.
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> >  drivers/gpu/drm/drm_mm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > index 04de6fd..cb39f45 100644
> > --- a/drivers/gpu/drm/drm_mm.c
> > +++ b/drivers/gpu/drm/drm_mm.c
> > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> >  {
> >  	struct drm_mm_node *hole;
> > -	u64 end = node->start + node->size;
> > +	u64 end;
> >  	u64 hole_start;
> >  	u64 hole_end;
> >  
> >  	BUG_ON(node == NULL);
> >  
> > +	end = node->start + node->size;
> > +
> >  	/* Find the relevant hole to add our node to */
> >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> >  		if (hole_start > node->start || hole_end < end)
> > -- 
> > 2.1.4
> 
> Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

Applied to drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Chris Wilson]

On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > Do not dereference node before the check if node is NULL.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > ---
> > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > index 04de6fd..cb39f45 100644
> > > --- a/drivers/gpu/drm/drm_mm.c
> > > +++ b/drivers/gpu/drm/drm_mm.c
> > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > >  {
> > >  	struct drm_mm_node *hole;
> > > -	u64 end = node->start + node->size;
> > > +	u64 end;
> > >  	u64 hole_start;
> > >  	u64 hole_end;
> > >  
> > >  	BUG_ON(node == NULL);
> > >  
> > > +	end = node->start + node->size;
> > > +
> > >  	/* Find the relevant hole to add our node to */
> > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > >  		if (hole_start > node->start || hole_end < end)
> > > -- 
> > > 2.1.4
> > 
> > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>

Remove useless check instead?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Eric Engestrom]

On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > Do not dereference node before the check if node is NULL.
> > > > 
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > ---
> > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > index 04de6fd..cb39f45 100644
> > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > >  {
> > > >  	struct drm_mm_node *hole;
> > > > -	u64 end = node->start + node->size;
> > > > +	u64 end;
> > > >  	u64 hole_start;
> > > >  	u64 hole_end;
> > > >  
> > > >  	BUG_ON(node == NULL);
> > > >  
> > > > +	end = node->start + node->size;
> > > > +
> > > >  	/* Find the relevant hole to add our node to */
> > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > >  		if (hole_start > node->start || hole_end < end)
> > > > -- 
> > > > 2.1.4
> > > 
> > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> 
> Remove useless check instead?
> -Chris

I tend to prefer erring on the side of caution and have (too) many checks,
especially simple ones like avoiding null dereferences.

That said, BUG() might be too extreme. I'm not all that familiar with
the code, but it doesn't seem like it would be an unrecoverable failure.
WARN() instead?

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Eric Engestrom]

On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > Do not dereference node before the check if node is NULL.
> > > > 
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > ---
> > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > index 04de6fd..cb39f45 100644
> > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > >  {
> > > >  	struct drm_mm_node *hole;
> > > > -	u64 end = node->start + node->size;
> > > > +	u64 end;
> > > >  	u64 hole_start;
> > > >  	u64 hole_end;
> > > >  
> > > >  	BUG_ON(node == NULL);
> > > >  
> > > > +	end = node->start + node->size;
> > > > +
> > > >  	/* Find the relevant hole to add our node to */
> > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > >  		if (hole_start > node->start || hole_end < end)
> > > > -- 
> > > > 2.1.4
> > > 
> > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> 
> Remove useless check instead?
> -Chris

I tend to prefer erring on the side of caution and have (too) many checks,
especially simple ones like avoiding null dereferences.

That said, BUG() might be too extreme. I'm not all that familiar with
the code, but it doesn't seem like it would be an unrecoverable failure.
WARN() instead?

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Chris Wilson]

On Mon, May 23, 2016 at 02:38:29PM +0100, Eric Engestrom wrote:
> On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> > On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > > Do not dereference node before the check if node is NULL.
> > > > > 
> > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > > index 04de6fd..cb39f45 100644
> > > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > > >  {
> > > > >  	struct drm_mm_node *hole;
> > > > > -	u64 end = node->start + node->size;
> > > > > +	u64 end;
> > > > >  	u64 hole_start;
> > > > >  	u64 hole_end;
> > > > >  
> > > > >  	BUG_ON(node == NULL);
> > > > >  
> > > > > +	end = node->start + node->size;
> > > > > +
> > > > >  	/* Find the relevant hole to add our node to */
> > > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > > >  		if (hole_start > node->start || hole_end < end)
> > > > > -- 
> > > > > 2.1.4
> > > > 
> > > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > 
> > Remove useless check instead?
> > -Chris
> 
> I tend to prefer erring on the side of caution and have (too) many checks,
> especially simple ones like avoiding null dereferences.

BUG(node == NULL) is no more informative than hitting a GFP with *node.

> That said, BUG() might be too extreme. I'm not all that familiar with
> the code, but it doesn't seem like it would be an unrecoverable failure.
> WARN() instead?

It's a programming error, just as would be passing in mm == NULL. Mark up
the function as requiring non-NULL parameters.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Chris Wilson]

On Mon, May 23, 2016 at 02:38:29PM +0100, Eric Engestrom wrote:
> On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> > On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > > Do not dereference node before the check if node is NULL.
> > > > > 
> > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > > index 04de6fd..cb39f45 100644
> > > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > > >  {
> > > > >  	struct drm_mm_node *hole;
> > > > > -	u64 end = node->start + node->size;
> > > > > +	u64 end;
> > > > >  	u64 hole_start;
> > > > >  	u64 hole_end;
> > > > >  
> > > > >  	BUG_ON(node == NULL);
> > > > >  
> > > > > +	end = node->start + node->size;
> > > > > +
> > > > >  	/* Find the relevant hole to add our node to */
> > > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > > >  		if (hole_start > node->start || hole_end < end)
> > > > > -- 
> > > > > 2.1.4
> > > > 
> > > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > 
> > Remove useless check instead?
> > -Chris
> 
> I tend to prefer erring on the side of caution and have (too) many checks,
> especially simple ones like avoiding null dereferences.

BUG(node == NULL) is no more informative than hitting a GFP with *node.

> That said, BUG() might be too extreme. I'm not all that familiar with
> the code, but it doesn't seem like it would be an unrecoverable failure.
> WARN() instead?

It's a programming error, just as would be passing in mm == NULL. Mark up
the function as requiring non-NULL parameters.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Eric Engestrom]

On Mon, May 23, 2016 at 02:46:59PM +0100, Chris Wilson wrote:
> On Mon, May 23, 2016 at 02:38:29PM +0100, Eric Engestrom wrote:
> > On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> > > On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > > > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > > > Do not dereference node before the check if node is NULL.
> > > > > > 
> > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > > > index 04de6fd..cb39f45 100644
> > > > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > > > >  {
> > > > > >  	struct drm_mm_node *hole;
> > > > > > -	u64 end = node->start + node->size;
> > > > > > +	u64 end;
> > > > > >  	u64 hole_start;
> > > > > >  	u64 hole_end;
> > > > > >  
> > > > > >  	BUG_ON(node == NULL);
> > > > > >  
> > > > > > +	end = node->start + node->size;
> > > > > > +
> > > > > >  	/* Find the relevant hole to add our node to */
> > > > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > > > >  		if (hole_start > node->start || hole_end < end)
> > > > > > -- 
> > > > > > 2.1.4
> > > > > 
> > > > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > > 
> > > Remove useless check instead?
> > > -Chris
> > 
> > I tend to prefer erring on the side of caution and have (too) many checks,
> > especially simple ones like avoiding null dereferences.
> 
> BUG(node == NULL) is no more informative than hitting a GFP with *node.

Good point.

> 
> > That said, BUG() might be too extreme. I'm not all that familiar with
> > the code, but it doesn't seem like it would be an unrecoverable failure.
> > WARN() instead?
> 
> It's a programming error, just as would be passing in mm == NULL. Mark up
> the function as requiring non-NULL parameters.
> -Chris

Using `__attribute__((nonnull))`? I approve of that idea, as I find
those attributes extremely useful and try to get them used as much as
possible at $DAYJOB.

I have noticed however that this particular attribute isn't used much in
the kernel (4 times that I could find with a simple grep), to the point
of not even having a `__nonnull`. Any reason you know of?

Cheers,
  Eric

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Eric Engestrom]

On Mon, May 23, 2016 at 02:46:59PM +0100, Chris Wilson wrote:
> On Mon, May 23, 2016 at 02:38:29PM +0100, Eric Engestrom wrote:
> > On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> > > On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > > > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > > > Do not dereference node before the check if node is NULL.
> > > > > > 
> > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > > > index 04de6fd..cb39f45 100644
> > > > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > > > >  {
> > > > > >  	struct drm_mm_node *hole;
> > > > > > -	u64 end = node->start + node->size;
> > > > > > +	u64 end;
> > > > > >  	u64 hole_start;
> > > > > >  	u64 hole_end;
> > > > > >  
> > > > > >  	BUG_ON(node == NULL);
> > > > > >  
> > > > > > +	end = node->start + node->size;
> > > > > > +
> > > > > >  	/* Find the relevant hole to add our node to */
> > > > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > > > >  		if (hole_start > node->start || hole_end < end)
> > > > > > -- 
> > > > > > 2.1.4
> > > > > 
> > > > > Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
> > > 
> > > Remove useless check instead?
> > > -Chris
> > 
> > I tend to prefer erring on the side of caution and have (too) many checks,
> > especially simple ones like avoiding null dereferences.
> 
> BUG(node == NULL) is no more informative than hitting a GFP with *node.

Good point.

> 
> > That said, BUG() might be too extreme. I'm not all that familiar with
> > the code, but it doesn't seem like it would be an unrecoverable failure.
> > WARN() instead?
> 
> It's a programming error, just as would be passing in mm == NULL. Mark up
> the function as requiring non-NULL parameters.
> -Chris

Using `__attribute__((nonnull))`? I approve of that idea, as I find
those attributes extremely useful and try to get them used as much as
possible at $DAYJOB.

I have noticed however that this particular attribute isn't used much in
the kernel (4 times that I could find with a simple grep), to the point
of not even having a `__nonnull`. Any reason you know of?

Cheers,
  Eric
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Chris Wilson]

On Mon, May 23, 2016 at 04:35:53PM +0100, Eric Engestrom wrote:
> On Mon, May 23, 2016 at 02:46:59PM +0100, Chris Wilson wrote:
> > It's a programming error, just as would be passing in mm == NULL. Mark up
> > the function as requiring non-NULL parameters.
> 
> Using `__attribute__((nonnull))`? I approve of that idea, as I find
> those attributes extremely useful and try to get them used as much as
> possible at $DAYJOB.
> 
> I have noticed however that this particular attribute isn't used much in
> the kernel (4 times that I could find with a simple grep), to the point
> of not even having a `__nonnull`. Any reason you know of?

Not that I know of. Even more confusing is its use under tools but not
being added to the main kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/mm: avoid possible null pointer dereference

[Chris Wilson]

On Mon, May 23, 2016 at 04:35:53PM +0100, Eric Engestrom wrote:
> On Mon, May 23, 2016 at 02:46:59PM +0100, Chris Wilson wrote:
> > It's a programming error, just as would be passing in mm == NULL. Mark up
> > the function as requiring non-NULL parameters.
> 
> Using `__attribute__((nonnull))`? I approve of that idea, as I find
> those attributes extremely useful and try to get them used as much as
> possible at $DAYJOB.
> 
> I have noticed however that this particular attribute isn't used much in
> the kernel (4 times that I could find with a simple grep), to the point
> of not even having a `__nonnull`. Any reason you know of?

Not that I know of. Even more confusing is its use under tools but not
being added to the main kernel.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel