| Message ID | 20240603110741.24818-1-abelova@astralinux.ru (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value | expand |
On 6/3/2024 06:07, Anastasia Belova wrote: > cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it > and return in case of error. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Anastasia Belova <abelova@astralinux.ru> Thank you! Acked-by: Mario Limonciello <mario.limonciello@amd.com> > --- > drivers/cpufreq/amd-pstate.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 1b7e82a0ad2e..672cb6c280a4 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu, > unsigned long max_perf, min_perf, des_perf, > cap_perf, lowest_nonlinear_perf, max_freq; > struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > + if (!policy) > + return; > struct amd_cpudata *cpudata = policy->driver_data; > unsigned int target_freq; > > @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata) > static void amd_pstate_update_limits(unsigned int cpu) > { > struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > + if (!policy) > + return; > struct amd_cpudata *cpudata = policy->driver_data; > u32 prev_high = 0, cur_high = 0; > int ret;
Hello, On Mon, Jun 03, 2024 at 02:07:41PM +0300, Anastasia Belova wrote: > cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it > and return in case of error. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Signed-off-by: Anastasia Belova <abelova@astralinux.ru> Thank you for the patch. Indeed we should be checking if the policy is valid before dereferencing it. > --- > drivers/cpufreq/amd-pstate.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c > index 1b7e82a0ad2e..672cb6c280a4 100644 > --- a/drivers/cpufreq/amd-pstate.c > +++ b/drivers/cpufreq/amd-pstate.c > @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu, > unsigned long max_perf, min_perf, des_perf, > cap_perf, lowest_nonlinear_perf, max_freq; > struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > + if (!policy) > + return; This patch mixes code and declarations. While I personally don't prefer that, since we have moved to using C99, the compiler does not complain, nor does checkpatch complain. So is this ok for cpufreq, Rafael? Or would you prefer something like: unsigned long cap_perf, lowest_nonlinear_perf; unsigned long max_perf, min_perf, des_perf; struct cpufreq_policy *policy; struct amd_cpudata *cpudata; unsigned int target_freq; unsigned long max_freq; policy = cpufreq_cpu_get(cpu); if (!policy) return; cpudata = policy->driver_data; > struct amd_cpudata *cpudata = policy->driver_data; > unsigned int target_freq; > > @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata) > static void amd_pstate_update_limits(unsigned int cpu) > { > struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > + if (!policy) > + return; Ditto. > struct amd_cpudata *cpudata = policy->driver_data; > u32 prev_high = 0, cur_high = 0; > int ret; > -- > 2.30.2 > -- Thanks and Regards gautham.
Hello, 06/06/24 12:55, Gautham R. Shenoy пишет: > Hello, > > On Mon, Jun 03, 2024 at 02:07:41PM +0300, Anastasia Belova wrote: >> cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it >> and return in case of error. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. >> >> Signed-off-by: Anastasia Belova <abelova@astralinux.ru> > Thank you for the patch. Indeed we should be checking if the policy is > valid before dereferencing it. > >> --- >> drivers/cpufreq/amd-pstate.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >> index 1b7e82a0ad2e..672cb6c280a4 100644 >> --- a/drivers/cpufreq/amd-pstate.c >> +++ b/drivers/cpufreq/amd-pstate.c >> @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu, >> unsigned long max_perf, min_perf, des_perf, >> cap_perf, lowest_nonlinear_perf, max_freq; >> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); >> + if (!policy) >> + return; > This patch mixes code and declarations. While I personally don't > prefer that, since we have moved to using C99, the compiler does > not complain, nor does checkpatch complain. > > So is this ok for cpufreq, Rafael? Should I form the second version without mixing code and declarations? Or it is better to wait for Rafael's answer? > > Or would you prefer something like: > > unsigned long cap_perf, lowest_nonlinear_perf; > unsigned long max_perf, min_perf, des_perf; > struct cpufreq_policy *policy; > struct amd_cpudata *cpudata; > unsigned int target_freq; > unsigned long max_freq; > > policy = cpufreq_cpu_get(cpu); > if (!policy) > return; > > cpudata = policy->driver_data; > > > >> struct amd_cpudata *cpudata = policy->driver_data; >> unsigned int target_freq; >> >> @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata) >> static void amd_pstate_update_limits(unsigned int cpu) >> { >> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); >> + if (!policy) >> + return; > Ditto. > >> struct amd_cpudata *cpudata = policy->driver_data; >> u32 prev_high = 0, cur_high = 0; >> int ret; >> -- >> 2.30.2 >> > -- > Thanks and Regards > gautham. Thanks, Anastasia Belova
On 8/8/2024 08:07, Anastasia Belova wrote: > Hello, > > 06/06/24 12:55, Gautham R. Shenoy пишет: >> Hello, >> >> On Mon, Jun 03, 2024 at 02:07:41PM +0300, Anastasia Belova wrote: >>> cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it >>> and return in case of error. >>> >>> Found by Linux Verification Center (linuxtesting.org) with SVACE. >>> >>> Signed-off-by: Anastasia Belova <abelova@astralinux.ru> >> Thank you for the patch. Indeed we should be checking if the policy is >> valid before dereferencing it. >> >>> --- >>> drivers/cpufreq/amd-pstate.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c >>> index 1b7e82a0ad2e..672cb6c280a4 100644 >>> --- a/drivers/cpufreq/amd-pstate.c >>> +++ b/drivers/cpufreq/amd-pstate.c >>> @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu, >>> unsigned long max_perf, min_perf, des_perf, >>> cap_perf, lowest_nonlinear_perf, max_freq; >>> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); >>> + if (!policy) >>> + return; >> This patch mixes code and declarations. While I personally don't >> prefer that, since we have moved to using C99, the compiler does >> not complain, nor does checkpatch complain. >> >> So is this ok for cpufreq, Rafael? > > Should I form the second version without mixing code and declarations? > Or it is better to wait for Rafael's answer? FWIW, I don't really like it either. As it's amd-pstate code I'd say Gautham and I should make the call. Can you please change it to avoid mixing code and declarations? > >> >> Or would you prefer something like: >> >> unsigned long cap_perf, lowest_nonlinear_perf; >> unsigned long max_perf, min_perf, des_perf; >> struct cpufreq_policy *policy; >> struct amd_cpudata *cpudata; >> unsigned int target_freq; >> unsigned long max_freq; >> >> policy = cpufreq_cpu_get(cpu); >> if (!policy) >> return; >> >> cpudata = policy->driver_data; >> >> >> >>> struct amd_cpudata *cpudata = policy->driver_data; >>> unsigned int target_freq; >>> @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct >>> amd_cpudata *cpudata) >>> static void amd_pstate_update_limits(unsigned int cpu) >>> { >>> struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); >>> + if (!policy) >>> + return; >> Ditto. >> >>> struct amd_cpudata *cpudata = policy->driver_data; >>> u32 prev_high = 0, cur_high = 0; >>> int ret; >>> -- >>> 2.30.2 >>> >> -- >> Thanks and Regards >> gautham. > > Thanks, > Anastasia Belova
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c index 1b7e82a0ad2e..672cb6c280a4 100644 --- a/drivers/cpufreq/amd-pstate.c +++ b/drivers/cpufreq/amd-pstate.c @@ -621,6 +621,8 @@ static void amd_pstate_adjust_perf(unsigned int cpu, unsigned long max_perf, min_perf, des_perf, cap_perf, lowest_nonlinear_perf, max_freq; struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); + if (!policy) + return; struct amd_cpudata *cpudata = policy->driver_data; unsigned int target_freq; @@ -777,6 +779,8 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata) static void amd_pstate_update_limits(unsigned int cpu) { struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); + if (!policy) + return; struct amd_cpudata *cpudata = policy->driver_data; u32 prev_high = 0, cur_high = 0; int ret;
cpufreq_cpu_get may return NULL. To avoid NULL-dereference check it and return in case of error. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Anastasia Belova <abelova@astralinux.ru> --- drivers/cpufreq/amd-pstate.c | 4 ++++ 1 file changed, 4 insertions(+)