Re: [PATCH 1/4] powerpc/perf: Rearrange memory freeing in imc init

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Monday 09 April 2018 02:30 PM, Anju T Sudhakar wrote:
> When any of the IMC (In-Memory Collection counter) devices fail
> to initialize, imc_common_mem_free() frees set of memory. In doing so,
> pmu_ptr pointer is also freed. But pmu_ptr pointer is used in subsequent
> function (imc_common_cpuhp_mem_free()) which is wrong. Patch here reorders
> the code to avoid such access.
>
> Also free the memory which is dynamically allocated during imc initialization,
> wherever required.

Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>

> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> ---
> test matrix and static checker run details are updated in the cover letter
> patch is based on
> https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git (branch: merge)
>
>   arch/powerpc/perf/imc-pmu.c               | 32 ++++++++++++++++---------------
>   arch/powerpc/platforms/powernv/opal-imc.c | 13 ++++++++++---
>   2 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index d7532e7..258b0f4 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1153,7 +1153,7 @@ static void cleanup_all_core_imc_memory(void)
>   	/* mem_info will never be NULL */
>   	for (i = 0; i < nr_cores; i++) {
>   		if (ptr[i].vbase)
> -			free_pages((u64)ptr->vbase, get_order(size));
> +			free_pages((u64)ptr[i].vbase, get_order(size));
>   	}
>
>   	kfree(ptr);
> @@ -1191,7 +1191,6 @@ static void imc_common_mem_free(struct imc_pmu *pmu_ptr)
>   	if (pmu_ptr->attr_groups[IMC_EVENT_ATTR])
>   		kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]->attrs);
>   	kfree(pmu_ptr->attr_groups[IMC_EVENT_ATTR]);
> -	kfree(pmu_ptr);
>   }
>
>   /*
> @@ -1208,6 +1207,7 @@ static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
>   			cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
>   			kfree(nest_imc_refc);
>   			kfree(per_nest_pmu_arr);
> +			per_nest_pmu_arr = NULL;
>   		}
>
>   		if (nest_pmus > 0)
> @@ -1319,10 +1319,8 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   	int ret;
>
>   	ret = imc_mem_init(pmu_ptr, parent, pmu_idx);
> -	if (ret) {
> -		imc_common_mem_free(pmu_ptr);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_free_mem;
>
>   	switch (pmu_ptr->domain) {
>   	case IMC_DOMAIN_NEST:
> @@ -1337,7 +1335,9 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   			ret = init_nest_pmu_ref();
>   			if (ret) {
>   				mutex_unlock(&nest_init_lock);
> -				goto err_free;
> +				kfree(per_nest_pmu_arr);
> +				per_nest_pmu_arr = NULL;
> +				goto err_free_mem;
>   			}
>   			/* Register for cpu hotplug notification. */
>   			ret = nest_pmu_cpumask_init();
> @@ -1345,7 +1345,8 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   				mutex_unlock(&nest_init_lock);
>   				kfree(nest_imc_refc);
>   				kfree(per_nest_pmu_arr);
> -				goto err_free;
> +				per_nest_pmu_arr = NULL;
> +				goto err_free_mem;
>   			}
>   		}
>   		nest_pmus++;
> @@ -1355,7 +1356,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   		ret = core_imc_pmu_cpumask_init();
>   		if (ret) {
>   			cleanup_all_core_imc_memory();
> -			return ret;
> +			goto err_free_mem;
>   		}
>
>   		break;
> @@ -1363,7 +1364,7 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>   		ret = thread_imc_cpu_init();
>   		if (ret) {
>   			cleanup_all_thread_imc_memory();
> -			return ret;
> +			goto err_free_mem;
>   		}
>
>   		break;
> @@ -1373,23 +1374,24 @@ int init_imc_pmu(struct device_node *parent, struct imc_pmu *pmu_ptr, int pmu_id
>
>   	ret = update_events_in_group(parent, pmu_ptr);
>   	if (ret)
> -		goto err_free;
> +		goto err_free_cpuhp_mem;
>
>   	ret = update_pmu_ops(pmu_ptr);
>   	if (ret)
> -		goto err_free;
> +		goto err_free_cpuhp_mem;
>
>   	ret = perf_pmu_register(&pmu_ptr->pmu, pmu_ptr->pmu.name, -1);
>   	if (ret)
> -		goto err_free;
> +		goto err_free_cpuhp_mem;
>
>   	pr_info("%s performance monitor hardware support registered\n",
>   							pmu_ptr->pmu.name);
>
>   	return 0;
>
> -err_free:
> -	imc_common_mem_free(pmu_ptr);
> +err_free_cpuhp_mem:
>   	imc_common_cpuhp_mem_free(pmu_ptr);
> +err_free_mem:
> +	imc_common_mem_free(pmu_ptr);
>   	return ret;
>   }
> diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
> index 2a14fda..490bb72 100644
> --- a/arch/powerpc/platforms/powernv/opal-imc.c
> +++ b/arch/powerpc/platforms/powernv/opal-imc.c
> @@ -115,8 +115,10 @@ static int imc_get_mem_addr_nest(struct device_node *node,
>   		return -ENOMEM;
>
>   	chipid_arr = kcalloc(nr_chips, sizeof(*chipid_arr), GFP_KERNEL);
> -	if (!chipid_arr)
> +	if (!chipid_arr) {
> +		kfree(base_addr_arr);
>   		return -ENOMEM;
> +	}
>
>   	if (of_property_read_u32_array(node, "chip-id", chipid_arr, nr_chips))
>   		goto error;
> @@ -143,7 +145,6 @@ static int imc_get_mem_addr_nest(struct device_node *node,
>   	return 0;
>
>   error:
> -	kfree(pmu_ptr->mem_info);
>   	kfree(base_addr_arr);
>   	kfree(chipid_arr);
>   	return -1;
> @@ -183,8 +184,14 @@ static int imc_pmu_create(struct device_node *parent, int pmu_index, int domain)
>
>   	/* Function to register IMC pmu */
>   	ret = init_imc_pmu(parent, pmu_ptr, pmu_index);
> -	if (ret)
> +	if (ret) {
>   		pr_err("IMC PMU %s Register failed\n", pmu_ptr->pmu.name);
> +		kfree(pmu_ptr->pmu.name);
> +		if (pmu_ptr->domain == IMC_DOMAIN_NEST)
> +			kfree(pmu_ptr->mem_info);
> +		kfree(pmu_ptr);
> +		return ret;
> +	}
>
>   	return 0;
>