* [PATCH v2] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
@ 2022-05-11 8:26 ` Vaibhav Jain
0 siblings, 0 replies; 4+ messages in thread
From: Vaibhav Jain @ 2022-05-11 8:26 UTC (permalink / raw)
To: nvdimm, linuxppc-dev, Kajol Jain
Cc: Vaibhav Jain, Dan Williams, Aneesh Kumar K . V, Michael Ellerman,
Shivaprasad G Bhat, Ira Weiny
Right now 'char *' elements allocated for individual 'stat_id' in
'papr_scm_priv.nvdimm_events_map[]' during papr_scm_pmu_check_events(), get
leaked 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.
Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Change-log:
v2:
* Removed the typedef 'stat_id_t' which will be introduced in a later patch
[Aneesh]
* Updated the patch description a bit
---
arch/powerpc/platforms/pseries/papr_scm.c | 54 ++++++++++-------------
1 file changed, 24 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 39962c905542..181b855b3050 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -125,8 +125,8 @@ struct papr_scm_priv {
/* The bits which needs to be overridden */
u64 health_bitmap_inject_mask;
- /* array to have event_code and stat_id mappings */
- char **nvdimm_events_map;
+ /* array to have event_code and stat_id mappings */
+ u8 *nvdimm_events_map;
};
static int papr_scm_pmem_flush(struct nd_region *nd_region,
@@ -370,7 +370,7 @@ static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev,
stat = &stats->scm_statistic[0];
memcpy(&stat->stat_id,
- p->nvdimm_events_map[event->attr.config],
+ &p->nvdimm_events_map[event->attr.config * sizeof(stat->stat_id)],
sizeof(stat->stat_id));
stat->stat_val = 0;
@@ -462,14 +462,13 @@ 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;
u32 available_events;
-
- if (!p->stat_buffer_len)
- return -ENOENT;
+ int index, rc = 0;
available_events = (p->stat_buffer_len - sizeof(struct papr_scm_perf_stats))
/ sizeof(struct papr_scm_perf_stat);
+ if (available_events == 0)
+ return -EOPNOTSUPP;
/* Allocate the buffer for phyp where stats are written */
stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
@@ -478,35 +477,30 @@ 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->stat_id),
+ 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 * sizeof(stat->stat_id)],
+ &stat->stat_id, sizeof(stat->stat_id));
+ }
+out:
kfree(stats);
return rc;
}
base-commit: 348c71344111d7a48892e3e52264ff11956fc196
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
@ 2022-05-11 8:26 ` Vaibhav Jain
0 siblings, 0 replies; 4+ messages in thread
From: Vaibhav Jain @ 2022-05-11 8:26 UTC (permalink / raw)
To: nvdimm, linuxppc-dev, Kajol Jain
Cc: Shivaprasad G Bhat, Aneesh Kumar K . V, Vaibhav Jain,
Dan Williams, Ira Weiny
Right now 'char *' elements allocated for individual 'stat_id' in
'papr_scm_priv.nvdimm_events_map[]' during papr_scm_pmu_check_events(), get
leaked 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.
Fixes: 4c08d4bbc089 ("powerpc/papr_scm: Add perf interface support")
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Change-log:
v2:
* Removed the typedef 'stat_id_t' which will be introduced in a later patch
[Aneesh]
* Updated the patch description a bit
---
arch/powerpc/platforms/pseries/papr_scm.c | 54 ++++++++++-------------
1 file changed, 24 insertions(+), 30 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 39962c905542..181b855b3050 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -125,8 +125,8 @@ struct papr_scm_priv {
/* The bits which needs to be overridden */
u64 health_bitmap_inject_mask;
- /* array to have event_code and stat_id mappings */
- char **nvdimm_events_map;
+ /* array to have event_code and stat_id mappings */
+ u8 *nvdimm_events_map;
};
static int papr_scm_pmem_flush(struct nd_region *nd_region,
@@ -370,7 +370,7 @@ static int papr_scm_pmu_get_value(struct perf_event *event, struct device *dev,
stat = &stats->scm_statistic[0];
memcpy(&stat->stat_id,
- p->nvdimm_events_map[event->attr.config],
+ &p->nvdimm_events_map[event->attr.config * sizeof(stat->stat_id)],
sizeof(stat->stat_id));
stat->stat_val = 0;
@@ -462,14 +462,13 @@ 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;
u32 available_events;
-
- if (!p->stat_buffer_len)
- return -ENOENT;
+ int index, rc = 0;
available_events = (p->stat_buffer_len - sizeof(struct papr_scm_perf_stats))
/ sizeof(struct papr_scm_perf_stat);
+ if (available_events == 0)
+ return -EOPNOTSUPP;
/* Allocate the buffer for phyp where stats are written */
stats = kzalloc(p->stat_buffer_len, GFP_KERNEL);
@@ -478,35 +477,30 @@ 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->stat_id),
+ 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 * sizeof(stat->stat_id)],
+ &stat->stat_id, sizeof(stat->stat_id));
+ }
+out:
kfree(stats);
return rc;
}
base-commit: 348c71344111d7a48892e3e52264ff11956fc196
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
2022-05-11 8:26 ` Vaibhav Jain
@ 2022-05-24 11:09 ` Michael Ellerman
-1 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2022-05-24 11:09 UTC (permalink / raw)
To: nvdimm, Kajol Jain, Vaibhav Jain, linuxppc-dev
Cc: Dan Williams, Ira Weiny, Shivaprasad G Bhat, Aneesh Kumar K . V
On Wed, 11 May 2022 13:56:36 +0530, Vaibhav Jain wrote:
> Right now 'char *' elements allocated for individual 'stat_id' in
> 'papr_scm_priv.nvdimm_events_map[]' during papr_scm_pmu_check_events(), get
> leaked 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.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
https://git.kernel.org/powerpc/c/0e0946e22f3665d27325d389ff45ade6e93f3678
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
@ 2022-05-24 11:09 ` Michael Ellerman
0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2022-05-24 11:09 UTC (permalink / raw)
To: nvdimm, Kajol Jain, Vaibhav Jain, linuxppc-dev
Cc: Shivaprasad G Bhat, Ira Weiny, Michael Ellerman,
Aneesh Kumar K . V, Dan Williams
On Wed, 11 May 2022 13:56:36 +0530, Vaibhav Jain wrote:
> Right now 'char *' elements allocated for individual 'stat_id' in
> 'papr_scm_priv.nvdimm_events_map[]' during papr_scm_pmu_check_events(), get
> leaked 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.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
https://git.kernel.org/powerpc/c/0e0946e22f3665d27325d389ff45ade6e93f3678
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-24 11:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-11 8:26 [PATCH v2] powerpc/papr_scm: Fix leaking nvdimm_events_map elements Vaibhav Jain
2022-05-11 8:26 ` Vaibhav Jain
2022-05-24 11:09 ` Michael Ellerman
2022-05-24 11:09 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.