linux-kernel - Re: [PATCH] fgraph: Use CPU hotplug mechanism to initialize idle shadow stacks

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: <20241021145810.9a9daec35cb83a04fe10208a@kernel.org>
Date: Mon, 21 Oct 2024 14:58:10 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux trace kernel
 <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Mark Rutland <mark.rutland@....com>, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] fgraph: Use CPU hotplug mechanism to initialize idle
 shadow stacks

On Fri, 18 Oct 2024 21:43:00 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> From: Steven Rostedt <rostedt@...dmis.org>
> 
> The function graph infrastructure allocates a shadow stack for every task
> when enabled. This includes the idle tasks. The first time the function
> graph is invoked, the shadow stacks are created and never freed until the
> task exits. This includes the idle tasks.
> 
> Only the idle tasks that were for online CPUs had their shadow stacks
> created when function graph tracing started. If function graph tracing is
> enabled and a CPU comes online, the idle task representing that CPU will
> not have its shadow stack created, and all function graph tracing for that
> idle task will be silently dropped.
> 
> Instead, use the CPU hotplug mechanism to allocate the idle shadow stacks.
> This will include idle tasks for CPUs that come online during tracing.
> 
> This issue can be reproduced by:
> 
>  # cd /sys/kernel/tracing
>  # echo 0 > /sys/devices/system/cpu/cpu1/online
>  # echo 0 > set_ftrace_pid
>  # echo function_graph > current_tracer
>  # echo 1 > options/funcgraph-proc
>  # echo 1 > /sys/devices/system/cpu/cpu1
>  # grep '<idle>' per_cpu/cpu1/trace | head
> 
> Before, nothing would show up.
> 
> After:
>  1)    <idle>-0    |   0.811 us    |                        __enqueue_entity();
>  1)    <idle>-0    |   5.626 us    |                      } /* enqueue_entity */
>  1)    <idle>-0    |               |                      dl_server_update_idle_time() {
>  1)    <idle>-0    |               |                        dl_scaled_delta_exec() {
>  1)    <idle>-0    |   0.450 us    |                          arch_scale_cpu_capacity();
>  1)    <idle>-0    |   1.242 us    |                        }
>  1)    <idle>-0    |   1.908 us    |                      }
>  1)    <idle>-0    |               |                      dl_server_start() {
>  1)    <idle>-0    |               |                        enqueue_dl_entity() {
>  1)    <idle>-0    |               |                          task_contending() {
> 
> Note, if tracing stops and restarts, the old way would then initialize
> the onlined CPUs.
> 

Looks good to me, except one comment below;

Acked-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>

[...]
>  int register_ftrace_graph(struct fgraph_ops *gops)
>  {
> +	static bool fgraph_initialized;
>  	int command = 0;
>  	int ret = 0;
>  	int i = -1;
>  
>  	mutex_lock(&ftrace_lock);
>  
> +	if (!fgraph_initialized) {
> +		ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "fgraph_idle_init",

Nit: Maybe it is better to call it as "tracing/fgraph:online" ?

Thank you,

> +					fgraph_cpu_init, NULL);
> +		if (ret < 0) {
> +			pr_warn("fgraph: Error to init cpu hotplug support\n");
> +			return ret;
> +		}
> +		fgraph_initialized = true;
> +		ret = 0;
> +	}
> +
>  	if (!fgraph_array[0]) {
>  		/* The array must always have real data on it */
>  		for (i = 0; i < FGRAPH_ARRAY_SIZE; i++)
> -- 
> 2.45.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>