[PATCH] tools/mm/page_owner_sort: free per-record allocations

[PATCH] tools/mm/page_owner_sort: free per-record allocations

[chenyichong]

add_list() allocates a command name and text buffer for every page owner record.  The cleanup path only frees the outer list array, leaving both allocations for every retained record behind until process exit.

Records discarded while culling also lose their allocation references when the list is compacted.  Free those records as they are merged, track the compacted list size, and release the remaining per-record allocations on exit.  Also handle allocation failures in get_comm() and unwind the command name if allocating the text buffer fails.

Signed-off-by: chenyichong <chenyichong@uniontech.com>
---
 tools/mm/page_owner_sort.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/mm/page_owner_sort.c b/tools/mm/page_owner_sort.c
index e6954909401c..67a7fc6d9de2 100644
--- a/tools/mm/page_owner_sort.c
+++ b/tools/mm/page_owner_sort.c
@@ -372,6 +372,9 @@ static char *get_comm(char *buf)
 {
 	char *comm_str = malloc(TASK_COMM_LEN);
 
+	if (!comm_str)
+		return NULL;
+
 	memset(comm_str, 0, TASK_COMM_LEN);
 
 	search_pattern(&comm_pattern, comm_str, buf);
@@ -386,6 +389,12 @@ static char *get_comm(char *buf)
 	return comm_str;
 }
 
+static void free_block_list(struct block_list *block)
+{
+	free(block->comm);
+	free(block->txt);
+}
+
 static int get_arg_type(const char *arg)
 {
 	if (!strcmp(arg, "pid") || !strcmp(arg, "p"))
@@ -480,9 +489,15 @@ static bool add_list(char *buf, int len, char *ext_buf)
 	list[list_size].pid = get_pid(buf);
 	list[list_size].tgid = get_tgid(buf);
 	list[list_size].comm = get_comm(buf);
+	if (!list[list_size].comm) {
+		fprintf(stderr, "Out of memory\n");
+		return false;
+	}
 	list[list_size].txt = malloc(len+1);
 	if (!list[list_size].txt) {
 		fprintf(stderr, "Out of memory\n");
+		free(list[list_size].comm);
+		list[list_size].comm = NULL;
 		return false;
 	}
 	memcpy(list[list_size].txt, buf, len);
@@ -841,8 +856,10 @@ int main(int argc, char **argv)
 		} else {
 			list[count-1].num += list[i].num;
 			list[count-1].page_num += list[i].page_num;
+			free_block_list(&list[i]);
 		}
 	}
+	list_size = count;
 
 	qsort(list, count, sizeof(list[0]), compare_sort_condition);
 
@@ -876,8 +893,11 @@ int main(int argc, char **argv)
 		free(ext_buf);
 	if (buf)
 		free(buf);
-	if (list)
+	if (list) {
+		for (i = 0; i < list_size; i++)
+			free_block_list(&list[i]);
 		free(list);
+	}
 out_ts:
 	regfree(&ts_nsec_pattern);
 out_comm:
-- 
2.51.0

Re: [PATCH] tools/mm/page_owner_sort: free per-record allocations

[Vishal Moola]

On Wed, Jun 10, 2026 at 10:11:08AM +0800, chenyichong wrote:
> add_list() allocates a command name and text buffer for every page owner record.  The cleanup path only frees the outer list array, leaving both allocations for every retained record behind until process exit.
> 
> Records discarded while culling also lose their allocation references when the list is compacted.  Free those records as they are merged, track the compacted list size, and release the remaining per-record allocations on exit.  Also handle allocation failures in get_comm() and unwind the command name if allocating the text buffer fails.

Please take a look at the process[1]. Aka wrap commit messages at ~75
characters.

Running scripts/checkpatch.pl should catch this for you.

> Signed-off-by: chenyichong <chenyichong@uniontech.com>

Also I hope chenyichong is your name :). I'm not that familiar with
chinese names, and the lowercase one-word makes it easy to mistake as an
alias (we don't allow aliases). I'm used to seeing
"[First Name] [Surname]."

> ---
>  tools/mm/page_owner_sort.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/mm/page_owner_sort.c b/tools/mm/page_owner_sort.c
> index e6954909401c..67a7fc6d9de2 100644
> --- a/tools/mm/page_owner_sort.c
> +++ b/tools/mm/page_owner_sort.c
> @@ -372,6 +372,9 @@ static char *get_comm(char *buf)
>  {
>  	char *comm_str = malloc(TASK_COMM_LEN);
>  
> +	if (!comm_str)
> +		return NULL;
> +
>  	memset(comm_str, 0, TASK_COMM_LEN);
>  
>  	search_pattern(&comm_pattern, comm_str, buf);
> @@ -386,6 +389,12 @@ static char *get_comm(char *buf)
>  	return comm_str;
>  }
>  
> +static void free_block_list(struct block_list *block)
> +{
> +	free(block->comm);
> +	free(block->txt);
> +}
> +
>  static int get_arg_type(const char *arg)
>  {
>  	if (!strcmp(arg, "pid") || !strcmp(arg, "p"))
> @@ -480,9 +489,15 @@ static bool add_list(char *buf, int len, char *ext_buf)
>  	list[list_size].pid = get_pid(buf);
>  	list[list_size].tgid = get_tgid(buf);
>  	list[list_size].comm = get_comm(buf);
> +	if (!list[list_size].comm) {
> +		fprintf(stderr, "Out of memory\n");
> +		return false;
> +	}
>  	list[list_size].txt = malloc(len+1);
>  	if (!list[list_size].txt) {
>  		fprintf(stderr, "Out of memory\n");
> +		free(list[list_size].comm);
> +		list[list_size].comm = NULL;
>  		return false;

Returning false here sends us back to the error handling path in main()
where you end up calling your free_block_list() anyway. So we don't need
this here, right?

>  	}
>  	memcpy(list[list_size].txt, buf, len);
> @@ -841,8 +856,10 @@ int main(int argc, char **argv)
>  		} else {
>  			list[count-1].num += list[i].num;
>  			list[count-1].page_num += list[i].page_num;
> +			free_block_list(&list[i]);
>  		}
>  	}
> +	list_size = count;
>  
>  	qsort(list, count, sizeof(list[0]), compare_sort_condition);
>  
> @@ -876,8 +893,11 @@ int main(int argc, char **argv)
>  		free(ext_buf);
>  	if (buf)
>  		free(buf);
> -	if (list)
> +	if (list) {
> +		for (i = 0; i < list_size; i++)
> +			free_block_list(&list[i]);
>  		free(list);
> +	}
>  out_ts:
>  	regfree(&ts_nsec_pattern);
>  out_comm:

[1] https://docs.kernel.org/process/submitting-patches.html

[PATCH v2] tools/mm/page_owner_sort: free per-record allocations

[Yichong Chen]

add_list() allocates comm and txt for each page owner record, but the
cleanup path only frees the outer list array. This leaks both buffers for
every retained record.

Free discarded records during culling and free the retained records on
exit. Also unwind comm when allocating txt fails.

Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
Changes in v2:
- Wrap commit message lines to approximately 75 columns.
- Use "Yichong Chen" as the author name.

 tools/mm/page_owner_sort.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/mm/page_owner_sort.c b/tools/mm/page_owner_sort.c
index e6954909401c..67a7fc6d9de2 100644
--- a/tools/mm/page_owner_sort.c
+++ b/tools/mm/page_owner_sort.c
@@ -372,6 +372,9 @@ static char *get_comm(char *buf)
 {
 	char *comm_str = malloc(TASK_COMM_LEN);
 
+	if (!comm_str)
+		return NULL;
+
 	memset(comm_str, 0, TASK_COMM_LEN);
 
 	search_pattern(&comm_pattern, comm_str, buf);
@@ -386,6 +389,12 @@ static char *get_comm(char *buf)
 	return comm_str;
 }
 
+static void free_block_list(struct block_list *block)
+{
+	free(block->comm);
+	free(block->txt);
+}
+
 static int get_arg_type(const char *arg)
 {
 	if (!strcmp(arg, "pid") || !strcmp(arg, "p"))
@@ -480,9 +489,15 @@ static bool add_list(char *buf, int len, char *ext_buf)
 	list[list_size].pid = get_pid(buf);
 	list[list_size].tgid = get_tgid(buf);
 	list[list_size].comm = get_comm(buf);
+	if (!list[list_size].comm) {
+		fprintf(stderr, "Out of memory\n");
+		return false;
+	}
 	list[list_size].txt = malloc(len+1);
 	if (!list[list_size].txt) {
 		fprintf(stderr, "Out of memory\n");
+		free(list[list_size].comm);
+		list[list_size].comm = NULL;
 		return false;
 	}
 	memcpy(list[list_size].txt, buf, len);
@@ -841,8 +856,10 @@ int main(int argc, char **argv)
 		} else {
 			list[count-1].num += list[i].num;
 			list[count-1].page_num += list[i].page_num;
+			free_block_list(&list[i]);
 		}
 	}
+	list_size = count;
 
 	qsort(list, count, sizeof(list[0]), compare_sort_condition);
 
@@ -876,8 +893,11 @@ int main(int argc, char **argv)
 		free(ext_buf);
 	if (buf)
 		free(buf);
-	if (list)
+	if (list) {
+		for (i = 0; i < list_size; i++)
+			free_block_list(&list[i]);
 		free(list);
+	}
 out_ts:
 	regfree(&ts_nsec_pattern);
 out_comm:
-- 
2.51.0

Re: [PATCH v2] tools/mm/page_owner_sort: free per-record allocations

[Vishal Moola]

On Thu, Jun 11, 2026 at 10:34:11AM +0800, Yichong Chen wrote:
> add_list() allocates comm and txt for each page owner record, but the
> cleanup path only frees the outer list array. This leaks both buffers for
> every retained record.
> 
> Free discarded records during culling and free the retained records on
> exit. Also unwind comm when allocating txt fails.
> 
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
> Changes in v2:
> - Wrap commit message lines to approximately 75 columns.
> - Use "Yichong Chen" as the author name.

Thanks. Also, in the future send new versions as new threads :)

The patch looks fine, just see below for my comment you might have
missed last time.

> @@ -480,9 +489,15 @@ static bool add_list(char *buf, int len, char *ext_buf)
>  	list[list_size].pid = get_pid(buf);
>  	list[list_size].tgid = get_tgid(buf);
>  	list[list_size].comm = get_comm(buf);
> +	if (!list[list_size].comm) {
> +		fprintf(stderr, "Out of memory\n");
> +		return false;
> +	}
>  	list[list_size].txt = malloc(len+1);
>  	if (!list[list_size].txt) {
>  		fprintf(stderr, "Out of memory\n");
> +		free(list[list_size].comm);
> +		list[list_size].comm = NULL;
>  		return false;

Returning false here sends us back to the error handling path in main()
where you end up calling your free_block_list() anyway. So we don't need
this here, right?

>  	}
>  	memcpy(list[list_size].txt, buf, len);