| Message ID | 20220509060629.179282-1-vaibhav@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/papr_scm: Fix leaking nvdimm_events_map elements | expand |
Vaibhav Jain <vaibhav@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@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? > > @@ -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
Thanks for reviewing this patch Aneesh, "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > Vaibhav Jain <vaibhav@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@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 >
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; @@ -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; }
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@linux.ibm.com> --- arch/powerpc/platforms/pseries/papr_scm.c | 48 +++++++++++------------ 1 file changed, 22 insertions(+), 26 deletions(-) base-commit: 348c71344111d7a48892e3e52264ff11956fc196