[PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Artem Mygaiev]

vgic_vcpu_pending_irq() uses find_next_bit() library function with 
single 'unsigned long' variable, while it is designed to work with 
memory regions. Nothing wrong is happening since 'offset' is set to 0 
(other values could lead to memory corruption), but it would be more 
correct to use the find_first_bit() function instead.

Coverity Scan issue 1469342

Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index d831b35525..fd63906e9b 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -362,7 +362,7 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
      ASSERT(v == current);

      mask_priority = gic_hw_ops->read_vmcr_priority();
-    active_priority = find_next_bit(&apr, 32, 0);
+    active_priority = find_first_bit(&apr, 32);

      spin_lock_irqsave(&v->arch.vgic.lock, flags);


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Julien Grall]

Hi Artem,

Thank you for the report.

On 24/05/18 14:20, Artem Mygaiev wrote:
> vgic_vcpu_pending_irq() uses find_next_bit() library function with 
> single 'unsigned long' variable, while it is designed to work with 
> memory regions. Nothing wrong is happening since 'offset' is set to 0 
> (other values could lead to memory corruption), but it would be more 
> correct to use the find_first_bit() function instead.

I don't understand the commit message. It is fine to use other offset 
than 0 in find_next_bit as long as it is smaller than 32. There would be 
no corruption happening.

Furthermore, find_first_bit(&apr, 32, 0) and find_next_bit(&apr, 32) are 
equivalent because the former is just a macro using the latter (see 
include/asm-arm/arm64/bitops.h).

So as it is the patch is not solving anything. However, I think this is 
just a false positive. Coverity should be able to guess that it will not 
go past the array (BITOP_WORD will turned into 0).

> 
> Coverity Scan issue 1469342

For future reference, please use the tag: "Coverity-ID: 1469342".

> 
> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index d831b35525..fd63906e9b 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -362,7 +362,7 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
>       ASSERT(v == current);
> 
>       mask_priority = gic_hw_ops->read_vmcr_priority();
> -    active_priority = find_next_bit(&apr, 32, 0);
> +    active_priority = find_first_bit(&apr, 32);
> 
>       spin_lock_irqsave(&v->arch.vgic.lock, flags);
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Artem Mygaiev]

Hi Julien

On 24.05.18 16:49, Julien Grall wrote:
> Hi Artem,
> 
> Thank you for the report.
> 
> On 24/05/18 14:20, Artem Mygaiev wrote:
>> vgic_vcpu_pending_irq() uses find_next_bit() library function with 
>> single 'unsigned long' variable, while it is designed to work with 
>> memory regions. Nothing wrong is happening since 'offset' is set to 0 
>> (other values could lead to memory corruption), but it would be more 
>> correct to use the find_first_bit() function instead.
> 
> I don't understand the commit message. It is fine to use other offset 
> than 0 in find_next_bit as long as it is smaller than 32. There would be 
> no corruption happening.
> 
> Furthermore, find_first_bit(&apr, 32, 0) and find_next_bit(&apr, 32) are 
> equivalent because the former is just a macro using the latter (see 
> include/asm-arm/arm64/bitops.h).
> 
> So as it is the patch is not solving anything. However, I think this is 
> just a false positive. Coverity should be able to guess that it will not 
> go past the array (BITOP_WORD will turned into 0).
> 
Absolutely agree with you. Probably my message was not clear enough - 
with this particular patch I am not trying to fix a memory corruption, 
there is no memory corruption in the code now. It is just the use of 
functions: find_first_bit() is a better fit since the 
vgic_vcpu_pending_irq() function does not need to go over memory region 
and checks only one 32-bit variable. I have mentioned Coverity issue 
here because this was a false positive detected after today's test run.

>>
>> Coverity Scan issue 1469342
> 
> For future reference, please use the tag: "Coverity-ID: 1469342".
> 
Thanks, will do.

>>
>> Signed-off-by: Artem Mygaiev <artem_mygaiev@epam.com>
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index d831b35525..fd63906e9b 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -362,7 +362,7 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
>>       ASSERT(v == current);
>>
>>       mask_priority = gic_hw_ops->read_vmcr_priority();
>> -    active_priority = find_next_bit(&apr, 32, 0);
>> +    active_priority = find_first_bit(&apr, 32);
>>
>>       spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>
> 
> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Julien Grall]

On 24/05/18 15:07, Artem Mygaiev wrote:
> Hi Julien

Hi Artem,

> 
> On 24.05.18 16:49, Julien Grall wrote:
>> Hi Artem,
>>
>> Thank you for the report.
>>
>> On 24/05/18 14:20, Artem Mygaiev wrote:
>>> vgic_vcpu_pending_irq() uses find_next_bit() library function with 
>>> single 'unsigned long' variable, while it is designed to work with 
>>> memory regions. Nothing wrong is happening since 'offset' is set to 0 
>>> (other values could lead to memory corruption), but it would be more 
>>> correct to use the find_first_bit() function instead.
>>
>> I don't understand the commit message. It is fine to use other offset 
>> than 0 in find_next_bit as long as it is smaller than 32. There would 
>> be no corruption happening.
>>
>> Furthermore, find_first_bit(&apr, 32, 0) and find_next_bit(&apr, 32) 
>> are equivalent because the former is just a macro using the latter 
>> (see include/asm-arm/arm64/bitops.h).
>>
>> So as it is the patch is not solving anything. However, I think this 
>> is just a false positive. Coverity should be able to guess that it 
>> will not go past the array (BITOP_WORD will turned into 0).
>>
> Absolutely agree with you. Probably my message was not clear enough - 
> with this particular patch I am not trying to fix a memory corruption, 
> there is no memory corruption in the code now. It is just the use of 
> functions: find_first_bit() is a better fit since the 
> vgic_vcpu_pending_irq() function does not need to go over memory region 
> and checks only one 32-bit variable. I have mentioned Coverity issue 
> here because this was a false positive detected after today's test run.

Feel free to resend this patch as a clean-up. I always found the use of 
find_next_bit confusing over find_first_bit.

However, I don't think coverity should be mention in the commit message 
or title. This is making thing very confusing as, IHMO, this is a false 
positive. We tend to only mention coverity when this is either going to 
silent coverity or fix the defect.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] arm: Coverity 1469342 correct find_*_bit() functions use

[Artem Mygaiev]

Hi,

On 24.05.18 17:22, Julien Grall wrote:
> 
> 
> On 24/05/18 15:07, Artem Mygaiev wrote:
>> Hi Julien
> 
> Hi Artem,
> 
>>
>> On 24.05.18 16:49, Julien Grall wrote:
>>> Hi Artem,
>>>
>>> Thank you for the report.
>>>
>>> On 24/05/18 14:20, Artem Mygaiev wrote:
>>>> vgic_vcpu_pending_irq() uses find_next_bit() library function with 
>>>> single 'unsigned long' variable, while it is designed to work with 
>>>> memory regions. Nothing wrong is happening since 'offset' is set to 
>>>> 0 (other values could lead to memory corruption), but it would be 
>>>> more correct to use the find_first_bit() function instead.
>>>
>>> I don't understand the commit message. It is fine to use other offset 
>>> than 0 in find_next_bit as long as it is smaller than 32. There would 
>>> be no corruption happening.
>>>
>>> Furthermore, find_first_bit(&apr, 32, 0) and find_next_bit(&apr, 32) 
>>> are equivalent because the former is just a macro using the latter 
>>> (see include/asm-arm/arm64/bitops.h).
>>>
>>> So as it is the patch is not solving anything. However, I think this 
>>> is just a false positive. Coverity should be able to guess that it 
>>> will not go past the array (BITOP_WORD will turned into 0).
>>>
>> Absolutely agree with you. Probably my message was not clear enough - 
>> with this particular patch I am not trying to fix a memory corruption, 
>> there is no memory corruption in the code now. It is just the use of 
>> functions: find_first_bit() is a better fit since the 
>> vgic_vcpu_pending_irq() function does not need to go over memory 
>> region and checks only one 32-bit variable. I have mentioned Coverity 
>> issue here because this was a false positive detected after today's 
>> test run.
> 
> Feel free to resend this patch as a clean-up. I always found the use of 
> find_next_bit confusing over find_first_bit.
> 
> However, I don't think coverity should be mention in the commit message 
> or title. This is making thing very confusing as, IHMO, this is a false 
> positive. We tend to only mention coverity when this is either going to 
> silent coverity or fix the defect.
> 
Ok, I'll adjust the subject & commit message, and re-send. JFYI, I've 
marked this one as False Positive in Coverity; there were three more 
suspects produced in today's run, but nothing really worth attention.

> Cheers,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel