linux-kernel - Re: [PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3f2137d5-5bfc-44e7-9269-3069e44eda31@arm.com>
Date: Fri, 12 Sep 2025 09:55:57 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: Markus Elfring <Markus.Elfring@....de>, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 James Clark <james.clark@...aro.org>, Mike Leach <mike.leach@...aro.org>,
 Suzuki K Poulose <suzuki.poulose@....com>
Cc: LKML <linux-kernel@...r.kernel.org>, Leo Yan <leo.yan@....com>,
 Tamas Zsoldos <tamas.zsoldos@....com>
Subject: Re: [PATCH] coresight: trbe: Use scope-based resource management in
 arm_trbe_alloc_buffer()

On 09/09/25 5:58 PM, Markus Elfring wrote:
> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Tue, 9 Sep 2025 14:20:06 +0200
> 
> Scope-based resource management became supported for some
> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:

			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

SHA ID here just needs first 12 digits not the entire string.
> Introduce __cleanup() based infrastructure").
> 
> * Thus use the attribute “__free(kfree)”.
> 
> * Reduce the scopes for the local variables “buf” and “pglist”.

What is that required ?
> 
> * Omit four kfree() calls accordingly.

Right.

The commit message should be re-written with little more
context from arm_trbe_alloc_buffer(), after describing
the new scope-based resource management.
> 
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 21 ++++++++------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 8f9bbef71f23..1b0d58bf8613 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -733,8 +733,6 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>  				   struct perf_event *event, void **pages,
>  				   int nr_pages, bool snapshot)
>  {
> -	struct trbe_buf *buf;
> -	struct page **pglist;
>  	int i;
>  
>  	/*
> @@ -746,32 +744,29 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>  	if (nr_pages < 2)
>  		return NULL;
>  
> -	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, trbe_alloc_node(event));
> +	struct trbe_buf *buf __free(kfree) = kzalloc_node(sizeof(*buf),
> +							  GFP_KERNEL,
> +							  trbe_alloc_node(event));
>  	if (!buf)
>  		return NULL;
>  
> -	pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
> -	if (!pglist) {
> -		kfree(buf);
> +	struct page **pglist __free(kfree) = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
> +	if (!pglist)
>  		return NULL;
> -	}
>  
>  	for (i = 0; i < nr_pages; i++)
>  		pglist[i] = virt_to_page(pages[i]);
>  
>  	buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
> -	if (!buf->trbe_base) {
> -		kfree(pglist);
> -		kfree(buf);
> +	if (!buf->trbe_base)
>  		return NULL;
> -	}
> +
>  	buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
>  	buf->trbe_write = buf->trbe_base;
>  	buf->snapshot = snapshot;
>  	buf->nr_pages = nr_pages;
>  	buf->pages = pages;
> -	kfree(pglist);
> -	return buf;
> +	return_ptr(buf);
>  }
>  
>  static void arm_trbe_free_buffer(void *config)

Seems like a good idea though. But please keep the local variable
declaration in the current place which is bit cleaner and reduces
code churn as well.

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 7a226316c48f..7babba1a9afb 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -733,8 +733,8 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
                                   struct perf_event *event, void **pages,
                                   int nr_pages, bool snapshot)
 {
-       struct trbe_buf *buf;
-       struct page **pglist;
+       struct trbe_buf *buf __free(kfree);
+       struct page **pglist __free(kfree);
        int i;

        /*
@@ -751,27 +751,22 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
                return NULL;

        pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
-       if (!pglist) {
-               kfree(buf);
+       if (!pglist)
                return NULL;
-       }

        for (i = 0; i < nr_pages; i++)
                pglist[i] = virt_to_page(pages[i]);

        buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
-       if (!buf->trbe_base) {
-               kfree(pglist);
-               kfree(buf);
+       if (!buf->trbe_base)
                return NULL;
-       }
+
        buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
        buf->trbe_write = buf->trbe_base;
        buf->snapshot = snapshot;
        buf->nr_pages = nr_pages;
        buf->pages = pages;
-       kfree(pglist);
-       return buf;
+       return_ptr(buf);
 }

 static void arm_trbe_free_buffer(void *config)