lkml.org 
[lkml]   [2016]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/6] x86,perf/cqm: Fix cqm memory leak and notifier leak


On Tue, 8 Mar 2016, Thomas Gleixner wrote:

> On Wed, 2 Mar 2016, Vikas Shivappa wrote:
>
> Please fix the subject line prefix: "x86/perf/intel/cqm:"

Will fix..

>
>> Fixes the hotcpu notifier leak and other global variable memory leaks
>> during cqm(cache quality of service monitoring) initialization.
>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
>> ---
>>
>> Fixed the memory leak for cqm_rmid_ptrs as per Thomas feedback.
>>
>> arch/x86/kernel/cpu/perf_event_intel_cqm.c | 34 ++++++++++++++++++++++++------
>> 1 file changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/perf_event_intel_cqm.c b/arch/x86/kernel/cpu/perf_event_intel_cqm.c
>> index e6be335..37a93fa 100644
>> --- a/arch/x86/kernel/cpu/perf_event_intel_cqm.c
>> +++ b/arch/x86/kernel/cpu/perf_event_intel_cqm.c
>> @@ -250,10 +250,13 @@ static int intel_cqm_setup_rmid_cache(void)
>>
>> return 0;
>> fail:
>> - while (r--)
>> + while (r--) {
>> kfree(cqm_rmid_ptrs[r]);
>> + cqm_rmid_ptrs[r] = NULL;
>> + }
>>
>> kfree(cqm_rmid_ptrs);
>> + cqm_rmid_ptrs = NULL;
>
> This partial rollback is crap. Why can't you utilize cqm_cleanup() ?
> Performance certainly is not an issue here.
>
>> return -ENOMEM;
>> }
>>
>> @@ -1320,9 +1323,19 @@ static const struct x86_cpu_id intel_cqm_match[] = {
>> {}
>> };
>>
>> +static void cqm_cleanup(void)
>> +{
>> + int r = cqm_max_rmid;
>> +
>> + while (r--)
>> + kfree(cqm_rmid_ptrs[r]);
>> +
>> + kfree(cqm_rmid_ptrs);
>> +}
>> +
>> static int __init intel_cqm_init(void)
>> {
>> - char *str, scale[20];
>> + char *str = NULL, scale[20];
>> int i, cpu, ret;
>>
>> if (!x86_match_cpu(intel_cqm_match))
>> @@ -1382,16 +1395,25 @@ static int __init intel_cqm_init(void)
>> cqm_pick_event_reader(i);
>> }
>>
>> - __perf_cpu_notifier(intel_cqm_cpu_notifier);
>> -
>> ret = perf_pmu_register(&intel_cqm_pmu, "intel_cqm", -1);
>> - if (ret)
>> + if (ret) {
>> pr_err("Intel CQM perf registration failed: %d\n", ret);
>> - else
>> + goto out;
>> + } else {
>> pr_info("Intel CQM monitoring enabled\n");
>> + }
>>
>> + /*
>> + * Register the hot cpu notifier once we are sure cqm
>> + * is enabled to avoid notifier leak.
>> + */
>> + __perf_cpu_notifier(intel_cqm_cpu_notifier);
>> out:
>> cpu_notifier_register_done();
>> + if (ret) {
>> + kfree(str);
>> + cqm_cleanup();
>
> This is still broken. If intel_cqm_setup_rmid_cache() fails, you clear
> cqm_rmid_ptrs and then jump to out. ret is -ENOMEM, so you will call
> cqm_cleanup which will dereference cqm_rmid_ptrs .....
>
> Find below a delta patch, which fixes that proper and safe.

Thanks for sending it. will resend the patch with your fix.

-Vikas

>
> Thanks,
>
> tglx
>
> 8<----------------
>
> arch/x86/events/intel/cqm.c | 40 +++++++++++++++++++---------------------
> 1 file changed, 19 insertions(+), 21 deletions(-)
>
> --- a/arch/x86/events/intel/cqm.c
> +++ b/arch/x86/events/intel/cqm.c
> @@ -211,6 +211,20 @@ static void __put_rmid(u32 rmid)
> list_add_tail(&entry->list, &cqm_rmid_limbo_lru);
> }
>
> +static void cqm_cleanup(void)
> +{
> + int i;
> +
> + if (!cqm_rmid_ptrs)
> + return;
> +
> + for (i = 0; i < cqm_max_rmid; i++)
> + kfree(cqm_rmid_ptrs[i]);
> +
> + kfree(cqm_rmid_ptrs);
> + cqm_rmid_ptrs = NULL;
> +}
> +
> static int intel_cqm_setup_rmid_cache(void)
> {
> struct cqm_rmid_entry *entry;
> @@ -218,7 +232,7 @@ static int intel_cqm_setup_rmid_cache(vo
> int r = 0;
>
> nr_rmids = cqm_max_rmid + 1;
> - cqm_rmid_ptrs = kmalloc(sizeof(struct cqm_rmid_entry *) *
> + cqm_rmid_ptrs = kzalloc(sizeof(struct cqm_rmid_entry *) *
> nr_rmids, GFP_KERNEL);
> if (!cqm_rmid_ptrs)
> return -ENOMEM;
> @@ -247,16 +261,10 @@ static int intel_cqm_setup_rmid_cache(vo
> mutex_lock(&cache_mutex);
> intel_cqm_rotation_rmid = __get_rmid();
> mutex_unlock(&cache_mutex);
> -
> return 0;
> -fail:
> - while (r--) {
> - kfree(cqm_rmid_ptrs[r]);
> - cqm_rmid_ptrs[r] = NULL;
> - }
>
> - kfree(cqm_rmid_ptrs);
> - cqm_rmid_ptrs = NULL;
> +fail:
> + cqm_cleanup();
> return -ENOMEM;
> }
>
> @@ -1313,16 +1321,6 @@ static const struct x86_cpu_id intel_cqm
> {}
> };
>
> -static void cqm_cleanup(void)
> -{
> - int r = cqm_max_rmid;
> -
> - while (r--)
> - kfree(cqm_rmid_ptrs[r]);
> -
> - kfree(cqm_rmid_ptrs);
> -}
> -
> static int __init intel_cqm_init(void)
> {
> char *str = NULL, scale[20];
> @@ -1389,10 +1387,10 @@ static int __init intel_cqm_init(void)
> if (ret) {
> pr_err("Intel CQM perf registration failed: %d\n", ret);
> goto out;
> - } else {
> - pr_info("Intel CQM monitoring enabled\n");
> }
>
> + pr_info("Intel CQM monitoring enabled\n");
> +
> /*
> * Register the hot cpu notifier once we are sure cqm
> * is enabled to avoid notifier leak.
>

\
 
 \ /
  Last update: 2016-03-08 21:21    [W:1.000 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site