| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <15297094.Px11LlFYVk@vostro.rjw.lan>
Date: Sat, 18 Jul 2015 02:34:52 +0200
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Pan Xinhui <xinhuix.pan@...el.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
Viresh Kumar <viresh.kumar@...aro.org>,
"yanmin_zhang@...ux.intel.com" <yanmin_zhang@...ux.intel.com>,
"mnipxh@....com" <mnipxh@....com>
Subject: Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
On Tuesday, July 14, 2015 10:52:35 AM Pan Xinhui wrote:
> hi, Rafael,
> let me do more explanation :)
>
> On 2015年07月14日 10:09, Pan Xinhui wrote:
> > hi, Rafael,
> > thanks for you reply :)
> > On 2015年07月14日 07:26, Rafael J. Wysocki wrote:
> >> On Monday, July 13, 2015 02:33:08 PM Pan Xinhui wrote:
> >>> hi, Rafeal
> >>> thanks for your reply. :)
> >>>
> >>> On 2015年07月11日 04:44, Rafael J. Wysocki wrote:
> >>>> Hi,
> >>>>
> >>>> On Fri, Jul 10, 2015 at 7:50 AM, Pan Xinhui <xinhuix.pan@...el.com> wrote:
> >>>>>
> >>>>> If CONFIG_X86_ACPI_CPUFREQ_CPB has not been defined, the placeholder for
> >>>>> cpb is not needed. Add ifdef around it.
> >>>>>
> >>>>> Signed-off-by: Pan Xinhui <xinhuix.pan@...el.com>
> >>>>> ---
> >>>>> drivers/cpufreq/acpi-cpufreq.c | 2 ++
> >>>>> 1 file changed, 2 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> >>>>> index e7fcaa6..314a19e 100644
> >>>>> --- a/drivers/cpufreq/acpi-cpufreq.c
> >>>>> +++ b/drivers/cpufreq/acpi-cpufreq.c
> >>>>> @@ -884,7 +884,9 @@ static int acpi_cpufreq_resume(struct cpufreq_policy *policy)
> >>>>> static struct freq_attr *acpi_cpufreq_attr[] = {
> >>>>> &cpufreq_freq_attr_scaling_available_freqs,
> >>>>> &freqdomain_cpus,
> >>>>> +#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> >>>>> NULL, /* this is a placeholder for cpb, do not remove */
> >>>>> +#endif
> >>>>
> >>>> Adding the ifdef here doesn't change anything, because the next NULL
> >>>> will play the role of the one you've just #ifdefed and the structure
> >>>> will be filled with zeros from that point on anyway.
> >>>>
> >>> Yes, adding ifdef here does not change any binary codes. But I want to make the codes more readable. :)
> >>> Patch author has noticed two *NULL* here would confuse people, especially who first read this acpi-cpufreq.c file
> >>> From code style point, it would be better to have #ifdef around it.
> >>
> >> Not really.
> >>
> >> Why don't you simply drop *both* NULLs?
> >>
> > Just like string end with *NULL* :)
> >
> > 1021 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
> > 1022 struct device *dev)
> > 1023 {
> > 1024 struct freq_attr **drv_attr;
> > 1025 int ret = 0;
> > 1026
> > 1027 /* set up files for this cpu device */
> > 1028 drv_attr = cpufreq_driver->attr;
> > 1029 while (drv_attr && *drv_attr) {
> > 1030 ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
> > 1031 if (ret)
> > 1032 return ret;
> > 1033 drv_attr++;
> > 1034 }
> > If struct freq_attr *acpi_cpufreq_attr[] did not end with NULL, line 1033 will access invalid data area.
> > If *drv_attr(the data after struct freq_attr * array[]) happened to be not NULL. panic may hit in sysfs_create_file :(
> > So at least one *NULL* must be in the end of freq_attr *array[].
OK, so the array is NULL-terminated and one NULL is needed to mark the end of it.
> >
> > Actually in acpi-cpufreq.c, in acpi_cpufreq_init function.
> > 957 struct freq_attr **iter;
> > 958
> > 959 pr_debug("adding sysfs entry for cpb\n");
> > 960
> > 961 for (iter = acpi_cpufreq_attr; *iter != NULL; iter++)
> > 962 ;
> > 963
> > 964 /* make sure there is a terminator behind it */
> > 965 if (iter[1] == NULL)
> > 966 *iter = &cpb;
> > 967 }
> > line965, check of iter[1] is not needed. Maybe the patch author was afraid of an unexpected remove of first *NULL*.
> > It might be a better solution to add ifdef CONFIG_X86_ACPI_CPUFREQ_CPB around that *NULL*, and remove this !iter[1] check.
Ah, so that is an exceptionally ugly piece of code.
What about the patch below?
---
drivers/cpufreq/acpi-cpufreq.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
+++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
@@ -884,7 +884,9 @@ static int acpi_cpufreq_resume(struct cp
static struct freq_attr *acpi_cpufreq_attr[] = {
&cpufreq_freq_attr_scaling_available_freqs,
&freqdomain_cpus,
- NULL, /* this is a placeholder for cpb, do not remove */
+#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
+ &cpb,
+#endif
NULL,
};
@@ -957,17 +959,16 @@ static int __init acpi_cpufreq_init(void
* only if configured. This is considered legacy code, which
* will probably be removed at some point in the future.
*/
- if (check_amd_hwpstate_cpu(0)) {
- struct freq_attr **iter;
-
- pr_debug("adding sysfs entry for cpb\n");
+ if (!check_amd_hwpstate_cpu(0)) {
+ struct freq_attr **attr;
- for (iter = acpi_cpufreq_attr; *iter != NULL; iter++)
- ;
+ pr_debug("CPB unsupported, do not expose it\n");
- /* make sure there is a terminator behind it */
- if (iter[1] == NULL)
- *iter = &cpb;
+ for (attr = acpi_cpufreq_attr; *attr; attr++)
+ if (*attr == &cpb) {
+ *attr = NULL;
+ break;
+ }
}
#endif
acpi_cpufreq_boost_init();
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists