linux-kernel - Re: [PATCH 2/2] fgraph: Free ret_stack when task is done with it

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: <20241025002121.ef5dc8be87e1b6baa2dd544c@kernel.org>
Date: Fri, 25 Oct 2024 00:21:21 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org, Masami
 Hiramatsu <mhiramat@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Thomas Gleixner <tglx@...utronix.de>, Peter
 Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH 2/2] fgraph: Free ret_stack when task is done with it

On Thu, 24 Oct 2024 05:27:25 -0400
Steven Rostedt <rostedt@...dmis.org> wrote:

> From: Steven Rostedt <rostedt@...dmis.org>
> 
> The shadow stack used for function graph is only freed when function graph
> is done for those tasks that are no longer using them. That's because a
> function that does a long sleep (like poll) could be traced, and its
> original return address on the stack has been replaced with a pointer to a
> trampoline, but that return address is saved on the shadow stack. It can
> not be freed until the function returns and there's no more return
> addresses being stored on the shadow stack.
> 
> Add a static_branch test in the return part of the function graph code
> that is called after the return address on the shadow stack is popped. If
> the shadow stack is empty, call an irq_work that will call a work queue
> that will run the shadow stack freeing code again. This will clean up all
> the shadow stacks that were not removed when function graph ended but are
> no longer being used.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>
> ---
>  kernel/trace/fgraph.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 3c7f115217b4..7520ceba7748 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -174,6 +174,11 @@ int ftrace_graph_active;
>  
>  static struct kmem_cache *fgraph_stack_cachep;
>  
> +DEFINE_STATIC_KEY_FALSE(fgraph_ret_stack_cleanup);
> +static struct workqueue_struct *fgraph_ret_stack_wq;
> +static struct work_struct fgraph_ret_stack_work;
> +static struct irq_work fgraph_ret_stack_irq_work;
> +
>  static struct fgraph_ops *fgraph_array[FGRAPH_ARRAY_SIZE];
>  static unsigned long fgraph_array_bitmask;
>  
> @@ -849,8 +854,15 @@ static unsigned long __ftrace_return_to_handler(struct fgraph_ret_regs *ret_regs
>  	 */
>  	barrier();
>  	current->curr_ret_stack = offset - FGRAPH_FRAME_OFFSET;
> -
>  	current->curr_ret_depth--;
> +
> +	/*
> +	 * If function graph is done and this task is no longer using ret_stack
> +	 * then start the work to free it.
> +	 */
> +	if (static_branch_unlikely(&fgraph_ret_stack_cleanup) && current->curr_ret_depth < 0)
> +		irq_work_queue(&fgraph_ret_stack_irq_work);
> +
>  	return ret;
>  }
>  
> @@ -1375,6 +1387,21 @@ static void free_ret_stacks(void)
>  	}
>  }
>  
> +static void fgraph_ret_stack_work_func(struct work_struct *work)
> +{
> +	mutex_lock(&ftrace_lock);
> +	if (!ftrace_graph_active)
> +		free_ret_stacks();
> +	mutex_unlock(&ftrace_lock);
> +}

Hmm, will you scan all tasks everytime? Shouldn't we have another global
list of skipped tasks in remove_ret_stack(), like below?

static void remove_ret_stack(struct task_struct *t, struct list_head *freelist, struct list_head *skiplist, int list_index)
{
	struct ret_stack_free_data *free_data;
	struct list_head *head;

	/* If the ret_stack is still in use, skip this */
	if (t->curr_ret_depth >= 0)
		head = skiplist;
	else
		head = freelist;

	free_data = (struct ret_stack_free_data*)(t->ret_stack + list_index);
	list_add(&free_data->list, head);
	free_data->task = t;
}

Then we can scan only skiplist in free_ret_stacks() in fgraph_ret_stack_work_func().

Of course this will need to decouple preparing freelist/skiplist and
actual free function.

Thank you,

> +
> +static void fgraph_ret_stack_irq_func(struct irq_work *iwork)
> +{
> +	if (unlikely(!fgraph_ret_stack_wq))
> +		return;
> +	queue_work(fgraph_ret_stack_wq, &fgraph_ret_stack_work);
> +}
> +
>  static __init int fgraph_init(void)
>  {
>  	int ret;
> @@ -1385,6 +1412,12 @@ static __init int fgraph_init(void)
>  		pr_warn("fgraph: Error to init cpu hotplug support\n");
>  		return ret;
>  	}
> +	fgraph_ret_stack_wq = alloc_workqueue("fgraph_ret_stack_wq",
> +				      WQ_UNBOUND | WQ_MEM_RECLAIM, 0);
> +	WARN_ON(!fgraph_ret_stack_wq);
> +
> +	INIT_WORK(&fgraph_ret_stack_work, fgraph_ret_stack_work_func);
> +	init_irq_work(&fgraph_ret_stack_irq_work, fgraph_ret_stack_irq_func);
>  	return 0;
>  }
>  core_initcall(fgraph_init)
> @@ -1434,6 +1467,7 @@ int register_ftrace_graph(struct fgraph_ops *gops)
>  		ftrace_graph_disable_direct(true);
>  
>  	if (ftrace_graph_active == 1) {
> +		static_branch_disable(&fgraph_ret_stack_cleanup);
>  		ftrace_graph_enable_direct(false, gops);
>  		register_pm_notifier(&ftrace_suspend_notifier);
>  		ret = start_graph_tracing();
> @@ -1502,6 +1536,7 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
>  		ftrace_graph_entry = ftrace_graph_entry_stub;
>  		unregister_pm_notifier(&ftrace_suspend_notifier);
>  		unregister_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL);
> +		static_branch_enable(&fgraph_ret_stack_cleanup);
>  		free_ret_stacks();
>  	}
>   out:
> -- 
> 2.45.2
> 
> 


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