Message-ID: <alpine.DEB.2.10.1603081135110.27741@vshiva-Udesk>
Date:	Tue, 8 Mar 2016 11:36:21 -0800 (PST)
From:	Vikas Shivappa <vikas.shivappa@...el.com>
To:	Thomas Gleixner <tglx@...utronix.de>
cc:	Vikas Shivappa <vikas.shivappa@...ux.intel.com>,
	vikas.shivappa@...el.com, linux-kernel@...r.kernel.org,
	x86@...nel.org, hpa@...or.com, mingo@...nel.org,
	peterz@...radead.org, ravi.v.shankar@...el.com,
	tony.luck@...el.com, fenghua.yu@...el.com, h.peter.anvin@...el.com
Subject: Re: [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@...el.com>
>> Signed-off-by: Vikas Shivappa <vikas.shivappa@...ux.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.
>

Powered by blists - more mailing lists