[PATCH] irqdomain: check for NULL before dereferencing

[PATCH] irqdomain: check for NULL before dereferencing

[Dan Carpenter]

I reversed these two checks so we check "root_irq_data" for NULL before
dereferencing it.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index b9c688944429..e84b7056bb08 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1505,10 +1505,10 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
 		return -EINVAL;
 
-	if (domain->parent != root_irq_data->domain)
+	if (!root_irq_data)
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (domain->parent != root_irq_data->domain)
 		return -EINVAL;
 
 	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,

Re: [PATCH] irqdomain: check for NULL before dereferencing

[Thomas Gleixner]

On Fri, 25 Aug 2017, Dan Carpenter wrote:

> I reversed these two checks so we check "root_irq_data" for NULL before
> dereferencing it.

Can you please start to write coherent changelogs? See
Documentation/process/submitting-patches.rst

The above describes the change you made which is completely pointless
because we can see the change in the patch.

What's missing is WHY this is required and WHAT the problem with the
existing code is.

Thanks,

	tglx

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index b9c688944429..e84b7056bb08 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1505,10 +1505,10 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
>  	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
>  		return -EINVAL;
>  
> -	if (domain->parent != root_irq_data->domain)
> +	if (!root_irq_data)
>  		return -EINVAL;
>  
> -	if (!root_irq_data)
> +	if (domain->parent != root_irq_data->domain)
>  		return -EINVAL;
>  
>  	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
> 

[PATCH v2] irqdomain: potential Oops in irq_domain_push_irq()

[Dan Carpenter]

This code generates a Smatch warning:

    kernel/irq/irqdomain.c:1511 irq_domain_push_irq()
    warn: variable dereferenced before check 'root_irq_data' (see line 1508)

irq_get_irq_data() does sometimes return NULL pointers so it does look
like this code could Oops.  I've moved the check for NULL earlier to
fix this bug.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: redo the commit message

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index b9c688944429..e84b7056bb08 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1505,10 +1505,10 @@ int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
 		return -EINVAL;
 
-	if (domain->parent != root_irq_data->domain)
+	if (!root_irq_data)
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (domain->parent != root_irq_data->domain)
 		return -EINVAL;
 
 	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,

Re: [PATCH v2] irqdomain: potential Oops in irq_domain_push_irq()

[Thomas Gleixner]

On Fri, 25 Aug 2017, Dan Carpenter wrote:

> This code generates a Smatch warning:
> 
>     kernel/irq/irqdomain.c:1511 irq_domain_push_irq()
>     warn: variable dereferenced before check 'root_irq_data' (see line 1508)
> 
> irq_get_irq_data() does sometimes return NULL pointers so it does look
> like this code could Oops.  I've moved the check for NULL earlier to
> fix this bug.

>From Documentation/process/submitting-patches.rst:

 "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour."

Is it that hard to read _and_ understand what those instructions mean?

Thanks,

	tglx