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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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