[PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Shilpasri G Bhat]

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 <shilpa.bhat@linux.vnet.ibm.com>
---
 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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Viresh Kumar]

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 <shilpa.bhat@linux.vnet.ibm.com>
> ---
>  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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Shilpasri G Bhat]

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 <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>>  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
> 

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Viresh Kumar]

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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Rafael J. Wysocki]

On Mon, Feb 12, 2018 at 11:33 AM, Shilpasri G Bhat
<shilpa.bhat@linux.vnet.ibm.com> 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 <shilpa.bhat@linux.vnet.ibm.com>
>>> ---
>>>  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.

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Michael Ellerman]

Viresh Kumar <viresh.kumar@linaro.org> 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 <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>>  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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Viresh Kumar]

On 21-02-18, 16:39, Michael Ellerman wrote:
> Viresh Kumar <viresh.kumar@linaro.org> 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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Rafael J. Wysocki]

On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 21-02-18, 16:39, Michael Ellerman wrote:
>> Viresh Kumar <viresh.kumar@linaro.org> 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.

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Viresh Kumar]

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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Rafael J. Wysocki]

On Wed, Feb 21, 2018 at 11:02 AM, Viresh Kumar <viresh.kumar@linaro.org> 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.

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Viresh Kumar]

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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Michael Ellerman]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> On 21-02-18, 16:39, Michael Ellerman wrote:
>>> Viresh Kumar <viresh.kumar@linaro.org> 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

Re: [PATCH] cpufreq: powernv: Check negative value returned by cpufreq_table_find_index_dl()

[Rafael J. Wysocki]

On Wed, Feb 21, 2018 at 2:13 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
>> On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>> On 21-02-18, 16:39, Michael Ellerman wrote:
>>>> Viresh Kumar <viresh.kumar@linaro.org> 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.