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, 7 Jul 2015 14:23:42 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Pan Xinhui <xinhuix.pan@...el.com>
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

On 07-07-15, 15:52, Pan Xinhui wrote:
> 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);

Sigh. Too bad. So what has changed is that the sysfs directory is
allocated to policy->cpu during init and never changed. But
policy->cpu can surely change.

Sorry for that.

> 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.

Why?

> 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.

That is more internal to the core. Better don't use it.

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

Anyway, your patch is far from complete. You have just fixed a single
place where per-cpu data is accessed with policy->cpu. What about rest
of the code? Like target() :)

--
viresh
--
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/