* [PATCH] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
@ 2022-05-09 6:06 ` Vaibhav Jain
0 siblings, 0 replies; 4+ messages in thread
From: Vaibhav Jain @ 2022-05-09 6:06 UTC (permalink / raw)
To: nvdimm, linuxppc-dev, Kajol Jain, Michael Ellerman
Cc: Vaibhav Jain, Dan Williams, Aneesh Kumar K . V, Shivaprasad G Bhat
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;
@@ -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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
@ 2022-05-09 6:06 ` Vaibhav Jain
0 siblings, 0 replies; 4+ messages in thread
From: Vaibhav Jain @ 2022-05-09 6:06 UTC (permalink / raw)
To: nvdimm, linuxppc-dev, Kajol Jain, Michael Ellerman
Cc: Vaibhav Jain, Dan Williams, Shivaprasad G Bhat, Aneesh Kumar K . V
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;
@@ -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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
2022-05-09 6:06 ` Vaibhav Jain
(?)
@ 2022-05-10 6:46 ` Aneesh Kumar K.V
2022-05-11 7:17 ` Vaibhav Jain
-1 siblings, 1 reply; 4+ messages in thread
From: Aneesh Kumar K.V @ 2022-05-10 6:46 UTC (permalink / raw)
To: Vaibhav Jain, nvdimm, linuxppc-dev, Kajol Jain, Michael Ellerman
Cc: Vaibhav Jain, Dan Williams, Shivaprasad G Bhat
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/papr_scm: Fix leaking nvdimm_events_map elements
2022-05-10 6:46 ` Aneesh Kumar K.V
@ 2022-05-11 7:17 ` Vaibhav Jain
0 siblings, 0 replies; 4+ messages in thread
From: Vaibhav Jain @ 2022-05-11 7:17 UTC (permalink / raw)
To: Aneesh Kumar K.V, nvdimm, linuxppc-dev, Kajol Jain, Michael Ellerman
Cc: Dan Williams, Shivaprasad G Bhat
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
>
--
Cheers
~ Vaibhav
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-11 7:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-09 6:06 [PATCH] powerpc/papr_scm: Fix leaking nvdimm_events_map elements Vaibhav Jain
2022-05-09 6:06 ` Vaibhav Jain
2022-05-10 6:46 ` Aneesh Kumar K.V
2022-05-11 7:17 ` Vaibhav Jain
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.