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: <20241024210515.53943bdf@rorschach.local.home>
Date: Thu, 24 Oct 2024 21:05:15 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.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 Fri, 25 Oct 2024 00:21:21 +0900
Masami Hiramatsu (Google) <mhiramat@...nel.org> wrote:

> > +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.

I thought about doing it this way, but I felt that it made the code
more complex with little benefit. Yeah, we scan all tasks, but it only
happens in a work queue that is grabbing the ftrace_lock mutex. If
anything, I rather keep it this way and if it ends up being an issue we
can change it later.

One thing Thomas always says is "correctness first, optimize later".
This is much easier to get correct. Adding a skip list will add
complexity. Like I said, nothing prevents us from adding that feature
later, and if it ends up buggy, we can know which change caused the bug.

-- Steve