linux-kernel - Re: [PATCH v5 2/4] tracing: Have reserve_mem use phys_to

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: <Z-0CPFGDqcUt-fMp@kernel.org>
Date: Wed, 2 Apr 2025 12:24:12 +0300
From: Mike Rapoport <rppt@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Vincent Donnefort <vdonnefort@...gle.com>,
	Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>
Subject: Re: [PATCH v5 2/4] tracing: Have reserve_mem use phys_to_virt() and
 separate from memmap buffer

On Tue, Apr 01, 2025 at 06:58:13PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@...dmis.org>
> 
> The reserve_mem kernel command line option may pass back a physical
> address, but the memory is still part of the normal memory just like
> using memblock_reserve() would be. This means that the physical memory

... using memblock_alloc() would be

> returned by the reserve_mem command line option can be converted directly
> to virtual memory by simply using phys_to_virt().
> 
> When freeing the buffer there's no need to call vunmap() anymore as the
> memory allocated by reserve_mem is freed by the call to
> reserve_mem_release_by_name().
> 
> Because the persistent ring buffer can also be allocated via the memmap
> option, which *is* different than normal memory as it cannot be added back
> to the buddy system, it must be treated differently. It still needs to be
> virtually mapped to have access to it. It also can not be freed nor can it
> ever be memory mapped to user space.
> 
> Create a new trace_array flag called TRACE_ARRAY_FL_MEMMAP which gets set
> if the buffer is created by the memmap option, and this will prevent the
> buffer from being memory mapped by user space.
> 
> Also increment the ref count for memmap'ed buffers so that they can never
> be freed.
> 
> Link: https://lore.kernel.org/all/Z-wFszhJ_9o4dc8O@kernel.org/
> 
> Suggested-by: Mike Rapoport <rppt@...nel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
>  kernel/trace/trace.c | 23 ++++++++++++++++-------
>  kernel/trace/trace.h |  1 +
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index de9c237e5826..2f9c91f26d5b 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -8505,6 +8505,10 @@ static int tracing_buffers_mmap(struct file *filp, struct vm_area_struct *vma)
>  	struct trace_iterator *iter = &info->iter;
>  	int ret = 0;
>  
> +	/* A memmap'ed buffer is not supported for user space mmap */
> +	if (iter->tr->flags & TRACE_ARRAY_FL_MEMMAP)
> +		return -ENODEV;
> +
>  	/* Currently the boot mapped buffer is not supported for mmap */
>  	if (iter->tr->flags & TRACE_ARRAY_FL_BOOT)
>  		return -ENODEV;
> @@ -9614,9 +9618,6 @@ static void free_trace_buffers(struct trace_array *tr)
>  #ifdef CONFIG_TRACER_MAX_TRACE
>  	free_trace_buffer(&tr->max_buffer);
>  #endif
> -
> -	if (tr->range_addr_start)
> -		vunmap((void *)tr->range_addr_start);
>  }
>  
>  static void init_trace_flags_index(struct trace_array *tr)
> @@ -10710,6 +10711,7 @@ static inline void do_allocate_snapshot(const char *name) { }
>  __init static void enable_instances(void)
>  {
>  	struct trace_array *tr;
> +	bool memmap_area = false;
>  	char *curr_str;
>  	char *name;
>  	char *str;
> @@ -10778,6 +10780,7 @@ __init static void enable_instances(void)
>  					name);
>  				continue;
>  			}
> +			memmap_area = true;
>  		} else if (tok) {
>  			if (!reserve_mem_find_by_name(tok, &start, &size)) {
>  				start = 0;
> @@ -10800,7 +10803,10 @@ __init static void enable_instances(void)
>  				continue;
>  			}
>  
> -			addr = map_pages(start, size);
> +			if (memmap_area)
> +				addr = map_pages(start, size);
> +			else
> +				addr = (unsigned long)phys_to_virt(start);
>  			if (addr) {
>  				pr_info("Tracing: mapped boot instance %s at physical memory %pa of size 0x%lx\n",
>  					name, &start, (unsigned long)size);
> @@ -10827,10 +10833,13 @@ __init static void enable_instances(void)
>  			update_printk_trace(tr);
>  
>  		/*
> -		 * If start is set, then this is a mapped buffer, and
> -		 * cannot be deleted by user space, so keep the reference
> -		 * to it.
> +		 * memmap'd buffers can not be freed.
>  		 */
> +		if (memmap_area) {
> +			tr->flags |= TRACE_ARRAY_FL_MEMMAP;
> +			tr->ref++;
> +		}
> +
>  		if (start) {
>  			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
>  			tr->range_name = no_free_ptr(rname);
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index c20f6bcc200a..f9513dc14c37 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -447,6 +447,7 @@ enum {
>  	TRACE_ARRAY_FL_BOOT		= BIT(1),
>  	TRACE_ARRAY_FL_LAST_BOOT	= BIT(2),
>  	TRACE_ARRAY_FL_MOD_INIT		= BIT(3),
> +	TRACE_ARRAY_FL_MEMMAP		= BIT(4),
>  };
>  
>  #ifdef CONFIG_MODULES
> -- 
> 2.47.2
> 
> 

-- 
Sincerely yours,
Mike.