* [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value @ 2024-06-03 11:07 Anastasia Belova 2024-06-04 15:56 ` Mario Limonciello 2024-06-06 9:55 ` Gautham R. Shenoy 0 siblings, 2 replies; 5+ messages in thread From: Anastasia Belova @ 2024-06-03 11:07 UTC (permalink / raw) To: Huang Rui Cc: Anastasia Belova, Gautham R. Shenoy, Mario Limonciello, Perry Yuan, Rafael J. Wysocki, Viresh Kumar, linux-pm, linux-kernel, lvc-project 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(+) 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; -- 2.30.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value 2024-06-03 11:07 [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value Anastasia Belova @ 2024-06-04 15:56 ` Mario Limonciello 2024-06-06 9:55 ` Gautham R. Shenoy 1 sibling, 0 replies; 5+ messages in thread From: Mario Limonciello @ 2024-06-04 15:56 UTC (permalink / raw) To: Anastasia Belova, Huang Rui Cc: Gautham R. Shenoy, Perry Yuan, Rafael J. Wysocki, Viresh Kumar, linux-pm, linux-kernel, lvc-project 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; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value 2024-06-03 11:07 [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value Anastasia Belova 2024-06-04 15:56 ` Mario Limonciello @ 2024-06-06 9:55 ` Gautham R. Shenoy 2024-08-08 13:07 ` Anastasia Belova 1 sibling, 1 reply; 5+ messages in thread From: Gautham R. Shenoy @ 2024-06-06 9:55 UTC (permalink / raw) To: Anastasia Belova Cc: Huang Rui, Mario Limonciello, Perry Yuan, Rafael J. Wysocki, Viresh Kumar, linux-pm, linux-kernel, lvc-project 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value 2024-06-06 9:55 ` Gautham R. Shenoy @ 2024-08-08 13:07 ` Anastasia Belova 2024-08-23 16:19 ` Mario Limonciello 0 siblings, 1 reply; 5+ messages in thread From: Anastasia Belova @ 2024-08-08 13:07 UTC (permalink / raw) To: Gautham R. Shenoy Cc: Huang Rui, Mario Limonciello, Perry Yuan, Rafael J. Wysocki, Viresh Kumar, linux-pm, linux-kernel, lvc-project 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value 2024-08-08 13:07 ` Anastasia Belova @ 2024-08-23 16:19 ` Mario Limonciello 0 siblings, 0 replies; 5+ messages in thread From: Mario Limonciello @ 2024-08-23 16:19 UTC (permalink / raw) To: Anastasia Belova, Gautham R. Shenoy Cc: Huang Rui, Perry Yuan, Rafael J. Wysocki, Viresh Kumar, linux-pm, linux-kernel, lvc-project 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-08-23 16:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-06-03 11:07 [PATCH] cpufreq: amd-pstate: add check for cpufreq_cpu_get's return value Anastasia Belova 2024-06-04 15:56 ` Mario Limonciello 2024-06-06 9:55 ` Gautham R. Shenoy 2024-08-08 13:07 ` Anastasia Belova 2024-08-23 16:19 ` Mario Limonciello
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox