| Message ID | 1518430876-24464-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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 >
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 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
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.
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.
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.
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.
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.
"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
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.
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.
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);
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(+)