This patch fixes the below Coverity warning:
*** CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
/drivers/cpufreq/powernv-cpufreq.c: 1008 in powernv_fast_switch()
1002 unsigned int target_freq)
1003 {
1004 int index;
1005 struct powernv_smp_call_data freq_data;
1006
1007 index = cpufreq_table_find_index_dl(policy, target_freq);
>>> CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>>> Using variable "index" as an index to array "powernv_freqs".
1008 freq_data.pstate_id = powernv_freqs[index].driver_data;
1009 freq_data.gpstate_id = powernv_freqs[index].driver_data;
1010 set_pstate(&freq_data);
1011
1012 return powernv_freqs[index].frequency;
1013 }
Signed-off-by: Shilpasri G Bhat <[email protected]>
---
drivers/cpufreq/powernv-cpufreq.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 29cdec1..69edfe9 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
struct powernv_smp_call_data freq_data;
index = cpufreq_table_find_index_dl(policy, target_freq);
+ if (unlikely(index < 0))
+ index = get_nominal_index();
+
freq_data.pstate_id = powernv_freqs[index].driver_data;
freq_data.gpstate_id = powernv_freqs[index].driver_data;
set_pstate(&freq_data);
--
1.8.3.1
Hi,
On 02/12/2018 03:59 PM, Viresh Kumar wrote:
> On 12-02-18, 15:51, Shilpasri G Bhat wrote:
>> This patch fixes the below Coverity warning:
>>
>> *** CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>> /drivers/cpufreq/powernv-cpufreq.c: 1008 in powernv_fast_switch()
>> 1002 unsigned int target_freq)
>> 1003 {
>> 1004 int index;
>> 1005 struct powernv_smp_call_data freq_data;
>> 1006
>> 1007 index = cpufreq_table_find_index_dl(policy, target_freq);
>>>>> CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>>>>> Using variable "index" as an index to array "powernv_freqs".
>> 1008 freq_data.pstate_id = powernv_freqs[index].driver_data;
>> 1009 freq_data.gpstate_id = powernv_freqs[index].driver_data;
>> 1010 set_pstate(&freq_data);
>> 1011
>> 1012 return powernv_freqs[index].frequency;
>> 1013 }
>>
>> Signed-off-by: Shilpasri G Bhat <[email protected]>
>> ---
>> drivers/cpufreq/powernv-cpufreq.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index 29cdec1..69edfe9 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
>> struct powernv_smp_call_data freq_data;
>>
>> index = cpufreq_table_find_index_dl(policy, target_freq);
>> + if (unlikely(index < 0))
>> + index = get_nominal_index();
>> +
>
> AFAICT, you will get -1 here only if the freq table had no valid
> frequencies (or the freq table is empty). Why would that happen ?
I agree too. There is no way we can get -1 with initialized cpu frequency table.
We don't initialize powernv-cpufreq if we don't have valid CPU frequency
entries. Is there any other way to suppress the Coverity tool warning apart from
ignoring it?
Thanks and Regards,
Shilpa
>
>> freq_data.pstate_id = powernv_freqs[index].driver_data;
>> freq_data.gpstate_id = powernv_freqs[index].driver_data;
>> set_pstate(&freq_data);
>> --
>> 1.8.3.1
>
On 12-02-18, 15:51, Shilpasri G Bhat wrote:
> This patch fixes the below Coverity warning:
>
> *** CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
> /drivers/cpufreq/powernv-cpufreq.c: 1008 in powernv_fast_switch()
> 1002 unsigned int target_freq)
> 1003 {
> 1004 int index;
> 1005 struct powernv_smp_call_data freq_data;
> 1006
> 1007 index = cpufreq_table_find_index_dl(policy, target_freq);
> >>> CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
> >>> Using variable "index" as an index to array "powernv_freqs".
> 1008 freq_data.pstate_id = powernv_freqs[index].driver_data;
> 1009 freq_data.gpstate_id = powernv_freqs[index].driver_data;
> 1010 set_pstate(&freq_data);
> 1011
> 1012 return powernv_freqs[index].frequency;
> 1013 }
>
> Signed-off-by: Shilpasri G Bhat <[email protected]>
> ---
> drivers/cpufreq/powernv-cpufreq.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 29cdec1..69edfe9 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
> struct powernv_smp_call_data freq_data;
>
> index = cpufreq_table_find_index_dl(policy, target_freq);
> + if (unlikely(index < 0))
> + index = get_nominal_index();
> +
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
> freq_data.pstate_id = powernv_freqs[index].driver_data;
> freq_data.gpstate_id = powernv_freqs[index].driver_data;
> set_pstate(&freq_data);
> --
> 1.8.3.1
--
viresh
On 12-02-18, 16:03, Shilpasri G Bhat wrote:
> I agree too. There is no way we can get -1 with initialized cpu frequency table.
> We don't initialize powernv-cpufreq if we don't have valid CPU frequency
> entries. Is there any other way to suppress the Coverity tool warning apart from
> ignoring it?
So IIUC, this warning is generated by an external tool after static
analysis of the code ?
If yes, then just ignore the warning. We shouldn't try fixing the
kernel because a tool isn't smart enough to catch intentional
ignorance of the return value here.
--
viresh
Viresh Kumar <[email protected]> writes:
> On 12-02-18, 15:51, Shilpasri G Bhat wrote:
>> This patch fixes the below Coverity warning:
>>
>> *** CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>> /drivers/cpufreq/powernv-cpufreq.c: 1008 in powernv_fast_switch()
>> 1002 unsigned int target_freq)
>> 1003 {
>> 1004 int index;
>> 1005 struct powernv_smp_call_data freq_data;
>> 1006
>> 1007 index = cpufreq_table_find_index_dl(policy, target_freq);
>> >>> CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>> >>> Using variable "index" as an index to array "powernv_freqs".
>> 1008 freq_data.pstate_id = powernv_freqs[index].driver_data;
>> 1009 freq_data.gpstate_id = powernv_freqs[index].driver_data;
>> 1010 set_pstate(&freq_data);
>> 1011
>> 1012 return powernv_freqs[index].frequency;
>> 1013 }
>>
>> Signed-off-by: Shilpasri G Bhat <[email protected]>
>> ---
>> drivers/cpufreq/powernv-cpufreq.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index 29cdec1..69edfe9 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
>> struct powernv_smp_call_data freq_data;
>>
>> index = cpufreq_table_find_index_dl(policy, target_freq);
>> + if (unlikely(index < 0))
>> + index = get_nominal_index();
>> +
>
> AFAICT, you will get -1 here only if the freq table had no valid
> frequencies (or the freq table is empty). Why would that happen ?
Bugs?
Or if you ask for a target_freq that is higher than anything in the
table.
Or the API changes, and we forget to update this call site.
If you're saying that cpufreq_table_find_index_dl() can NEVER fail, then
write it so that it can never fail and change it to return unsigned int.
Having it potentially return -1, which is then used to index an array
and not handling that is just asking for bugs to happen.
cheers
On 21-02-18, 16:39, Michael Ellerman wrote:
> Viresh Kumar <[email protected]> writes:
> > AFAICT, you will get -1 here only if the freq table had no valid
> > frequencies (or the freq table is empty). Why would that happen ?
>
> Bugs?
The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).
> Or if you ask for a target_freq that is higher than anything in the
> table.
You will still get a valid index in that case.
There is only once case where we return -1, when the cpufreq table
doesn't have any valid frequencies.
> Or the API changes, and we forget to update this call site.
I am not sure we can do much about that right now.
> If you're saying that cpufreq_table_find_index_dl() can NEVER fail,
Yes, if we have at least one valid frequency in the table, otherwise
the cpufreq driver shouldn't have registered itself.
> then
> write it so that it can never fail and change it to return unsigned int.
But what should we do when there is no frequency in the cpufreq table?
Just in case where a driver is buggy and tries to call this routine
for an invalid table.
> Having it potentially return -1, which is then used to index an array
> and not handling that is just asking for bugs to happen.
I understand what you are trying to say here, but I don't know what
can be done to prevent this here.
What we can do is change the return type to void and pass a int
pointer to the routine, but that wouldn't change anything at all. That
pointers variable can still have -1 in it.
--
viresh
On Wed, Feb 21, 2018 at 11:02 AM, Viresh Kumar <[email protected]> wrote:
> On 21-02-18, 10:27, Rafael J. Wysocki wrote:
>> To be precise, ->init() should fail as that's where the table is
>> created. The registration fails as a result then.
>>
>> But what if the bug is that ->init() doesn't fail when it should?
>>
>> I guess the core could double check the frequency table after ->init()
>> if ->target_index is not NULL.
>>
>> The overall point here is that if you get a negative index in
>> ->fast_switch(), that's way too late anyway and we should be able to
>> catch that error much earlier.
>
> I don't want to end up doing double checking as some of it is already
> done at init, but let me check on what can be done.
The driver is expected to call cpufreq_table_validate_and_show() at
->init() time and fail ->init() if that fails.
That's kind of fragile, because it depends on the driver to do the right thing.
On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar <[email protected]> wrote:
> On 21-02-18, 16:39, Michael Ellerman wrote:
>> Viresh Kumar <[email protected]> writes:
>
>> > AFAICT, you will get -1 here only if the freq table had no valid
>> > frequencies (or the freq table is empty). Why would that happen ?
>>
>> Bugs?
>
> The cupfreq driver shouldn't have registered itself in that case (i.e.
> if the cpufreq table is empty).
To be precise, ->init() should fail as that's where the table is
created. The registration fails as a result then.
But what if the bug is that ->init() doesn't fail when it should?
I guess the core could double check the frequency table after ->init()
if ->target_index is not NULL.
The overall point here is that if you get a negative index in
->fast_switch(), that's way too late anyway and we should be able to
catch that error much earlier.
On 21-02-18, 10:27, Rafael J. Wysocki wrote:
> To be precise, ->init() should fail as that's where the table is
> created. The registration fails as a result then.
>
> But what if the bug is that ->init() doesn't fail when it should?
>
> I guess the core could double check the frequency table after ->init()
> if ->target_index is not NULL.
>
> The overall point here is that if you get a negative index in
> ->fast_switch(), that's way too late anyway and we should be able to
> catch that error much earlier.
I don't want to end up doing double checking as some of it is already
done at init, but let me check on what can be done.
--
viresh
On 21-02-18, 11:17, Rafael J. Wysocki wrote:
> The driver is expected to call cpufreq_table_validate_and_show() at
> ->init() time and fail ->init() if that fails.
>
> That's kind of fragile, because it depends on the driver to do the right thing.
That's exactly what I am trying to explore here, i.e. Call
validate/show from core instead of drivers.
--
viresh
"Rafael J. Wysocki" <[email protected]> writes:
> On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar <[email protected]> wrote:
>> On 21-02-18, 16:39, Michael Ellerman wrote:
>>> Viresh Kumar <[email protected]> writes:
>>
>>> > AFAICT, you will get -1 here only if the freq table had no valid
>>> > frequencies (or the freq table is empty). Why would that happen ?
>>>
>>> Bugs?
>>
>> The cupfreq driver shouldn't have registered itself in that case (i.e.
>> if the cpufreq table is empty).
>
> To be precise, ->init() should fail as that's where the table is
> created. The registration fails as a result then.
>
> But what if the bug is that ->init() doesn't fail when it should?
>
> I guess the core could double check the frequency table after ->init()
> if ->target_index is not NULL.
>
> The overall point here is that if you get a negative index in
> ->fast_switch(), that's way too late anyway and we should be able to
> catch that error much earlier.
OK.
Still it's one thing for the driver to print a warning and bail out,
it's another to access off the front of an array and keep running using
some junk values, or oops (though not in this case because the array
happens to be static).
cheers
On Wed, Feb 21, 2018 at 2:13 PM, Michael Ellerman <[email protected]> wrote:
> "Rafael J. Wysocki" <[email protected]> writes:
>
>> On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar <[email protected]> wrote:
>>> On 21-02-18, 16:39, Michael Ellerman wrote:
>>>> Viresh Kumar <[email protected]> writes:
>>>
>>>> > AFAICT, you will get -1 here only if the freq table had no valid
>>>> > frequencies (or the freq table is empty). Why would that happen ?
>>>>
>>>> Bugs?
>>>
>>> The cupfreq driver shouldn't have registered itself in that case (i.e.
>>> if the cpufreq table is empty).
>>
>> To be precise, ->init() should fail as that's where the table is
>> created. The registration fails as a result then.
>>
>> But what if the bug is that ->init() doesn't fail when it should?
>>
>> I guess the core could double check the frequency table after ->init()
>> if ->target_index is not NULL.
>>
>> The overall point here is that if you get a negative index in
>> ->fast_switch(), that's way too late anyway and we should be able to
>> catch that error much earlier.
>
> OK.
>
> Still it's one thing for the driver to print a warning and bail out,
> it's another to access off the front of an array and keep running using
> some junk values, or oops (though not in this case because the array
> happens to be static).
Well, let me rephrase. If ->fast_switch() runs, then it must not be
possible to get a negative index in it. That has to be guaranteed by
the core.
On Mon, Feb 12, 2018 at 11:33 AM, Shilpasri G Bhat
<[email protected]> wrote:
> Hi,
>
> On 02/12/2018 03:59 PM, Viresh Kumar wrote:
>> On 12-02-18, 15:51, Shilpasri G Bhat wrote:
>>> This patch fixes the below Coverity warning:
>>>
>>> *** CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>>> /drivers/cpufreq/powernv-cpufreq.c: 1008 in powernv_fast_switch()
>>> 1002 unsigned int target_freq)
>>> 1003 {
>>> 1004 int index;
>>> 1005 struct powernv_smp_call_data freq_data;
>>> 1006
>>> 1007 index = cpufreq_table_find_index_dl(policy, target_freq);
>>>>>> CID 182816: Memory - illegal accesses (NEGATIVE_RETURNS)
>>>>>> Using variable "index" as an index to array "powernv_freqs".
>>> 1008 freq_data.pstate_id = powernv_freqs[index].driver_data;
>>> 1009 freq_data.gpstate_id = powernv_freqs[index].driver_data;
>>> 1010 set_pstate(&freq_data);
>>> 1011
>>> 1012 return powernv_freqs[index].frequency;
>>> 1013 }
>>>
>>> Signed-off-by: Shilpasri G Bhat <[email protected]>
>>> ---
>>> drivers/cpufreq/powernv-cpufreq.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>>> index 29cdec1..69edfe9 100644
>>> --- a/drivers/cpufreq/powernv-cpufreq.c
>>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>>> @@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
>>> struct powernv_smp_call_data freq_data;
>>>
>>> index = cpufreq_table_find_index_dl(policy, target_freq);
>>> + if (unlikely(index < 0))
>>> + index = get_nominal_index();
>>> +
>>
>> AFAICT, you will get -1 here only if the freq table had no valid
>> frequencies (or the freq table is empty). Why would that happen ?
>
> I agree too. There is no way we can get -1 with initialized cpu frequency table.
> We don't initialize powernv-cpufreq if we don't have valid CPU frequency
> entries. Is there any other way to suppress the Coverity tool warning apart from
> ignoring it?
In principle you could use BUG_ON(something_impossible) to annotate
that kind of thing to the static analysis tools, but that would
generate extra code.