linux-kernel - Re: [External] Re: [PATCH] arm64/ftrace: Make function graph use ftrace directly

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] [day] [month] [year] [list]

Date:   Thu, 24 Feb 2022 10:03:13 +0800
From:   Chengming Zhou <zhouchengming@...edance.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     mingo@...hat.com, catalin.marinas@....com, will@...nel.org,
        mark.rutland@....com, broonie@...nel.org, songmuchun@...edance.com,
        qirui.001@...edance.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] arm64/ftrace: Make function graph use
 ftrace directly

On 2022/2/24 9:30 上午, Steven Rostedt wrote:
> On Wed, 23 Feb 2022 16:00:27 +0800
> Chengming Zhou <zhouchengming@...edance.com> wrote:
> 
>> Yes, it would be better to implement DYNAMIC_FTRACE_WITH_ARGS on arm64 too,
>> and this patch just use DYNAMIC_FTRACE_WITH_REGS to install return_hooker
>> for graph tracer, so it's a code cleanup, no performance optimization.
> 
> I'm worried that a clean up is either breaking the design or hurting
> performance.
> 
> You have:
> 
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>> +{
>> +	struct pt_regs *regs = arch_ftrace_get_regs(fregs);
> 
> Now, technically, arch_ftrace_get_regs() is to return NULL if the
> ftrace_ops was not registered with ops->flags |= FTRACE_OPS_FL_SAVE_REGS.
> 
> Which function graph does not do.
> 
> But this is in arch specific code so you have more control of this
> "undefined behavior". But you really should have a comment saying that
> this needs to be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
> 

Ok, I will add a comment noting that.

Thanks.

> -- Steve
> 
> 
>> +	unsigned long *parent = (unsigned long *)&procedure_link_pointer(regs);
>> +
>> +	prepare_ftrace_return(ip, parent, frame_pointer(regs));
>> +}
> 
> -- Steve