[PATCH] powerpc/papr_scm: Fix leaking nvdimm_events_map elements

[Vaibhav Jain]

Thanks for reviewing this patch Aneesh,

"Aneesh Kumar K.V" <aneesh.kumar at linux.ibm.com> writes:

> Vaibhav Jain <vaibhav at linux.ibm.com> writes:
>
>> Right now 'char *' elements allocated individual 'stat_id' in
>> 'papr_scm_priv.nvdimm_events_map' during papr_scm_pmu_check_events() leak in
>> papr_scm_remove() and papr_scm_pmu_register(), papr_scm_pmu_check_events() error
>> paths.
>>
>> Also individual 'stat_id' arent NULL terminated 'char *' instead they are fixed
>> 8-byte sized identifiers. However papr_scm_pmu_register() assumes it to be a
>> NULL terminated 'char *' and at other places it assumes it to be a
>> 'papr_scm_perf_stat.stat_id' sized string which is 8-byes in size.
>>
>> Fix this by allocating the memory for papr_scm_priv.nvdimm_events_map to also
>> include space for 'stat_id' entries. This is possible since number of available
>> events/stat_ids are known upfront. This saves some memory and one extra level of
>> indirection from 'nvdimm_events_map' to 'stat_id'. Also rest of the code
>> can continue to call 'kfree(papr_scm_priv.nvdimm_events_map)' without needing to
>> iterate over the array and free up individual elements.
>>
>> Also introduce a new typedef called 'state_id_t' thats a 'u8[8]' and can be used
>> across papr_scm to deal with stat_ids.
>>
>> Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
>> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
>> ---
>>  arch/powerpc/platforms/pseries/papr_scm.c | 48 +++++++++++------------
>>  1 file changed, 22 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 39962c905542..f33a865ad5fb 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -70,8 +70,10 @@
>>  #define PAPR_SCM_PERF_STATS_VERSION 0x1
>>  
>>  /* Struct holding a single performance metric */
>> +typedef u8 stat_id_t[8];
>> +
>>  struct papr_scm_perf_stat {
>> -	u8 stat_id[8];
>> +	stat_id_t stat_id;
>>  	__be64 stat_val;
>>  } __packed;
>
> Can we do this as two patch? One that fix the leak and other that adds
> new type?
>
Yeah, sure that would be a simple change. I will send a v2 across that
only removes the leak and then a subsequent patch that changes the usage
of stat_id within papr_scm to the new typedef

>>  
>> @@ -126,7 +128,7 @@ struct papr_scm_priv {
>>  	u64 health_bitmap_inject_mask;
>>  
>>  	 /* array to have event_code and stat_id mappings */
>> -	char **nvdimm_events_map;
>> +	stat_id_t *nvdimm_events_map;
>>  };
>>  
>>  static int papr_scm_pmem_flush(struct nd_region *nd_region,
>> @@ -462,7 +464,7 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
>>  {
>>  	struct papr_scm_perf_stat *stat;
>>  	struct papr_scm_perf_stats *stats;
>> -	int index, rc, count;
>> +	int index, rc = 0;
>>  	u32 available_events;
>>  
>>  	if (!p->stat_buffer_len)
>> @@ -478,35 +480,29 @@ static int papr_scm_pmu_check_events(struct papr_scm_priv *p, struct nvdimm_pmu
>>  		return rc;
>>  	}
>>  
>> -	/* Allocate memory to nvdimm_event_map */
>> -	p->nvdimm_events_map = kcalloc(available_events, sizeof(char *), GFP_KERNEL);
>> -	if (!p->nvdimm_events_map) {
>> -		rc = -ENOMEM;
>> -		goto out_stats;
>> -	}
>> -
>>  	/* Called to get list of events supported */
>>  	rc = drc_pmem_query_stats(p, stats, 0);
>>  	if (rc)
>> -		goto out_nvdimm_events_map;
>> -
>> -	for (index = 0, stat = stats->scm_statistic, count = 0;
>> -		     index < available_events; index++, ++stat) {
>> -		p->nvdimm_events_map[count] = kmemdup_nul(stat->stat_id, 8, GFP_KERNEL);
>> -		if (!p->nvdimm_events_map[count]) {
>> -			rc = -ENOMEM;
>> -			goto out_nvdimm_events_map;
>> -		}
>> +		goto out;
>>  
>> -		count++;
>> +	/*
>> +	 * Allocate memory and populate nvdimm_event_map.
>> +	 * Allocate an extra element for NULL entry
>> +	 */
>> +	p->nvdimm_events_map = kcalloc(available_events + 1,
>> +				       sizeof(stat_id_t), GFP_KERNEL);
>> +	if (!p->nvdimm_events_map) {
>> +		rc = -ENOMEM;
>> +		goto out;
>>  	}
>> -	p->nvdimm_events_map[count] = NULL;
>> -	kfree(stats);
>> -	return 0;
>>  
>> -out_nvdimm_events_map:
>> -	kfree(p->nvdimm_events_map);
>> -out_stats:
>> +	/* Copy all stat_ids to event map */
>> +	for (index = 0, stat = stats->scm_statistic;
>> +	     index < available_events; index++, ++stat) {
>> +		memcpy(&p->nvdimm_events_map[index], &stat->stat_id,
>> +		       sizeof(stat_id_t));
>> +	}
>> +out:
>>  	kfree(stats);
>>  	return rc;
>>  }
>>
>> base-commit: 348c71344111d7a48892e3e52264ff11956fc196
>> -- 
>> 2.35.1
>

-- 
Cheers
~ Vaibhav