linux-kernel - Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 07 Jul 2015 15:52:23 +0800
From:	Pan Xinhui <xinhuix.pan@...el.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-pm@...r.kernel.org, rjw@...ysocki.net,
	"yanmin_zhang@...ux.intel.com" <yanmin_zhang@...ux.intel.com>,
	"mnipxh@....com" <mnipxh@....com>
Subject: Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

hi, Viresh
	thanks for your reply.

On 2015年07月07日 14:54, Viresh Kumar wrote:
> On 06-07-15, 14:30, Pan Xinhui wrote:
>>
>> policy->cpu in acpi_cpufreq_cpu_init/exit is the same cpu in most cases.
>> However during cpu hotplug,
>> cpufreq core might nominate a new cpu for policy->cpu.
> 
> Why aren't above lines well aligned? A simple trick to share for vim
> users:
> 
> - Select lines you want to auto-align with shift+v and up-down keys
> - press gq
> - That's it and vim will do it for you. You need to set vim's
>   'textwidth' to 72 or 80, based on what you are editing, so that vim
>   knows where you need to break the line. I have this in vimrc
> 
>         set textwidth=80
>         au FileType gitcommit set textwidth=72
> 

thanks, that will save me time.

> 
> 
> Back to the real stuff. Few core changes have gone into v4.2-rc1 and
> policy->cpu doesn't change any longer on hotplug (unless its a
> physical hotplug). So you shouldn't see any issues.
> 
I have latest codes.
codes in cpufreq.c are below.
1436     down_write(&policy->rwsem);
1437     cpumask_clear_cpu(cpu, policy->cpus);
1438 
1439     if (policy_is_inactive(policy)) {
1440         if (has_target())
1441             strncpy(policy->last_governor, policy->governor->name,
1442                 CPUFREQ_NAME_LEN);
1443     } else if (cpu == policy->cpu) {
1444         /* Nominate new CPU */
1445         policy->cpu = cpumask_any(policy->cpus);
1446     }
1447     up_write(&policy->rwsem);

line 1445 will change the policy->cpu.
for example, cpu2,3 has same policy, and policy->cpu is 2 at beginning.
If we disable cpu2, policy->cpu is 3.
yes, at most time, cpu0,1,2,3,,etc share the same policy, and policy->cpu is 0 which can't be offline.
So no memory leak. it is just lucky. :)

back to my previous patch, you suggest me to use policy->driver_data to *store* data and don't need use per_cpu anymore.
codes in acpi-cpufreq.c are below.
 365 static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
 366 {
 367     struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
 368     unsigned int freq;
 369     unsigned int cached_freq;

we get *data* through per_cpu for now, as the parameter is cpu only.
If we store *data* in policy->driver_data, we need call
struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) to get policy.
We do a full codes review, and find there should be deadlock if we doing so.
But as cpufreq code offers
238 /* Only for cpufreq core internal use */
239 struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)

I have a small question,if we can use *cpufreq_cpu_get_raw* in ->get callback, which is already lock hold,
But the comment(line 238) is... hmm.

thanks for your kind reply. any advices or comments are welcome.

thanks
xinhui
--
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/