[PATCH] irqchip/gic: check return value of of_address_to_resource

[PATCH] irqchip/gic: check return value of of_address_to_resource

[Bo Yan]

The of_address_to_resource returns 0 if successful. gic_check_eoimode
calls it without checking the return value. This induces Coverity
warning: "Unchecked return value".

Return false from gic_check_eoimode if of_address_to_resource returns
non-0 value.

Signed-off-by: Bo Yan <byan@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ced10c4..0bceb10 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1284,7 +1284,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 {
 	struct resource cpuif_res;
 
-	of_address_to_resource(node, 1, &cpuif_res);
+	if (of_address_to_resource(node, 1, &cpuif_res))
+		return false;
 
 	if (!is_hyp_mode_available())
 		return false;
-- 
2.7.4

Re: [PATCH] irqchip/gic: check return value of of_address_to_resource

[Marc Zyngier]

Hi Bo,

On Thu, 5 Jul 2018 11:20:59 -0700
Bo Yan <byan@nvidia.com> wrote:

> The of_address_to_resource returns 0 if successful. gic_check_eoimode
> calls it without checking the return value. This induces Coverity
> warning: "Unchecked return value".
> 
> Return false from gic_check_eoimode if of_address_to_resource returns
> non-0 value.
> 
> Signed-off-by: Bo Yan <byan@nvidia.com>
> ---
>  drivers/irqchip/irq-gic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index ced10c4..0bceb10 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1284,7 +1284,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>  {
>  	struct resource cpuif_res;
>  
> -	of_address_to_resource(node, 1, &cpuif_res);
> +	if (of_address_to_resource(node, 1, &cpuif_res))
> +		return false;

We've just done an of_iomap() on this resource, which succeeded. How
can the same thing now fail? It would mean that the device tree has
been pulled from under our feet...

And if it could happen, why is returning false the right thing to do?
Why would we say we want EOImode==0 instead of 1?

>  
>  	if (!is_hyp_mode_available())
>  		return false;

As it stands, I'm not taking such a patch. It either papers over a
bigger problem, or just keeps a warning quiet for the sake of it.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] irqchip/gic: check return value of of_address_to_resource

[Bo Yan]

Marc,

I'm also wondering if of_address_to_resource can really fail in this 
particular case?

What if we just explicitly discard the return value like this:

(void)of_address_to_resource(node, 1, &cpuif_res);

This suppresses Coverity warning by explicitly stating we are 100% sure 
the function call will always return success.

On 07/05/2018 12:13 PM, Marc Zyngier wrote:
> Hi Bo,
> 
> On Thu, 5 Jul 2018 11:20:59 -0700
> Bo Yan <byan@nvidia.com> wrote:
> 
>> The of_address_to_resource returns 0 if successful. gic_check_eoimode
>> calls it without checking the return value. This induces Coverity
>> warning: "Unchecked return value".
>>
>> Return false from gic_check_eoimode if of_address_to_resource returns
>> non-0 value.
>>
>> Signed-off-by: Bo Yan <byan@nvidia.com>
>> ---
>>   drivers/irqchip/irq-gic.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>> index ced10c4..0bceb10 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -1284,7 +1284,8 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>>   {
>>   	struct resource cpuif_res;
>>   
>> -	of_address_to_resource(node, 1, &cpuif_res);
>> +	if (of_address_to_resource(node, 1, &cpuif_res))
>> +		return false;
> 
> We've just done an of_iomap() on this resource, which succeeded. How
> can the same thing now fail? It would mean that the device tree has
> been pulled from under our feet...
> 
> And if it could happen, why is returning false the right thing to do?
> Why would we say we want EOImode==0 instead of 1?
> 
>>   
>>   	if (!is_hyp_mode_available())
>>   		return false;
> 
> As it stands, I'm not taking such a patch. It either papers over a
> bigger problem, or just keeps a warning quiet for the sake of it.
> 
> Thanks,
> 
> 	M.
> 

Re: [PATCH] irqchip/gic: check return value of of_address_to_resource

[Bo Yan]

Marc,

Sorry for the previous reply. My email settings were not correct, so it 
inserted those confidentiality text, which was not what I intended.

This is what I think:

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ced10c4..0b60bb0 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1284,7 +1284,7 @@ static bool gic_check_eoimode(struct device_node 
*node, void __iomem **base)
  {
         struct resource cpuif_res;

-       of_address_to_resource(node, 1, &cpuif_res);
+       (void)of_address_to_resource(node, 1, &cpuif_res);

         if (!is_hyp_mode_available())
                 return false;

We are 100% sure of_address_to_resource will succeed in this particular 
case, so "(void)" will help suppress Coverity warning.

On 07/05/2018 12:18 PM, Bo Yan wrote:
> Marc,
> 
> I'm also wondering if of_address_to_resource can really fail in this 
> particular case?
> 
> What if we just explicitly discard the return value like this:
> 
> (void)of_address_to_resource(node, 1, &cpuif_res);
> 
> This suppresses Coverity warning by explicitly stating we are 100% sure 
> the function call will always return success.
> 
> On 07/05/2018 12:13 PM, Marc Zyngier wrote:
>> Hi Bo,
>>
>> On Thu, 5 Jul 2018 11:20:59 -0700
>> Bo Yan <byan@nvidia.com> wrote:
>>
>>> The of_address_to_resource returns 0 if successful. gic_check_eoimode
>>> calls it without checking the return value. This induces Coverity
>>> warning: "Unchecked return value".
>>>
>>> Return false from gic_check_eoimode if of_address_to_resource returns
>>> non-0 value.
>>>
>>> Signed-off-by: Bo Yan <byan@nvidia.com>
>>> ---
>>>   drivers/irqchip/irq-gic.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index ced10c4..0bceb10 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -1284,7 +1284,8 @@ static bool gic_check_eoimode(struct 
>>> device_node *node, void __iomem **base)
>>>   {
>>>       struct resource cpuif_res;
>>> -    of_address_to_resource(node, 1, &cpuif_res);
>>> +    if (of_address_to_resource(node, 1, &cpuif_res))
>>> +        return false;
>>
>> We've just done an of_iomap() on this resource, which succeeded. How
>> can the same thing now fail? It would mean that the device tree has
>> been pulled from under our feet...
>>
>> And if it could happen, why is returning false the right thing to do?
>> Why would we say we want EOImode==0 instead of 1?
>>
>>>       if (!is_hyp_mode_available())
>>>           return false;
>>
>> As it stands, I'm not taking such a patch. It either papers over a
>> bigger problem, or just keeps a warning quiet for the sake of it.
>>
>> Thanks,
>>
>>     M.
>>

Re: [PATCH] irqchip/gic: check return value of of_address_to_resource

[Marc Zyngier]

On Thu, 5 Jul 2018 12:32:22 -0700
Bo Yan <byan@nvidia.com> wrote:

> Marc,
> 
> Sorry for the previous reply. My email settings were not correct, so it inserted those confidentiality text, which was not what I intended.
> 
> This is what I think:
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index ced10c4..0b60bb0 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1284,7 +1284,7 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>   {
>          struct resource cpuif_res;
> 
> -       of_address_to_resource(node, 1, &cpuif_res);
> +       (void)of_address_to_resource(node, 1, &cpuif_res);
> 
>          if (!is_hyp_mode_available())
>                  return false;
> 
> We are 100% sure of_address_to_resource will succeed in this particular case, so "(void)" will help suppress Coverity warning.

In all honesty. I don't see the point of patching the kernel to silence
a warning when we know that this is a false positive. I'm sure you can
flag that one as "false positive" in Coverity.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.