Re: [PATCH 3/3] x86/ftrace: Use text_poke()

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	Nadav Amit <nadav.amit@gmail.com>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Song Liu <songliubraving@fb.com>,
	Masami Hiramatsu <mhiramat@kernel.org>
Subject: Re: [PATCH 3/3] x86/ftrace: Use text_poke()
Date: Wed, 23 Oct 2019 15:36:54 -0700	[thread overview]
Message-ID: <20191023223652.aemv225jvx5yhocc@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20191023160852.0606bc68@gandalf.local.home>

On Wed, Oct 23, 2019 at 04:08:52PM -0400, Steven Rostedt wrote:
> > 
> > It seems to me that it's a bit of overkill to add new config knob
> > for every ftrace feature.
> > HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS (that arch defined) would
> > be enough to check and return error in register_ftrace_direct()
> > right?
> 
> IIRC, we started doing this because it allows the dependencies to be
> defined in the kernel/trace directory. That is, if
> CONFIG_DYNAMIC_FATRCE_WITH_DIRECT_CALLS is set, then we know that
> direct calls *and* DYNAMIC_FTRACE is enabled. It cuts down on some of
> the more complex #if or the arch needing to do
> 
>  select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS if DYNAMIC_FTRACE
> 
> It may be overkill, but it does keep the pain in one place.

Ok.
could you please add
static inline int register_ftrace_direct(unsigned long ip, unsigned long addr)
{
        return -ENOTSUPP;
}
when appropriate configs are not enabled?
The current approach of:
#define register_ftrace_function(ops) ({ 0; })
doesn't look right, since users are being mislead that it's a success.

> > > +struct ftrace_ops direct_ops = {
> > > +	.func		= call_direct_funcs,
> > > +	.flags		= FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_RECURSION_SAFE
> > > +#if 1
> > > +					| FTRACE_OPS_FL_DIRECT
> > > +#endif
> > > +#ifndef CONFIG_DYNAMIC_FTRACE_WITH_IPMODIFY_ONLY
> > > +					| FTRACE_OPS_FL_SAVE_REGS
> > > +#endif  
> > 
> > With FL_DIRECT the CONFIG_DYNAMIC_FTRACE_WITH_IPMODIFY_ONLY won't be needed, right ?
> > At least not for bpf use case.
> > Do you see livepatching using it or switching to FL_DIRECT too?
> 
> Correct. I talked with Josh on IRC and we are looking into removing the
> pushf/popf from the ftrace_regs_caller to see if that helps in the
> performance for live patching. I'm also currently working on making
> this patch not on top of the IP modify one, so the IP modify doesn't
> need to be applied.
> 
> This also cleans up the asm code a bit more (getting rid of the macro).

awesome.

> > 
> > I have one more question/request.
> > Looks like ftrace can be turned off with sysctl.
> > Which means that a person or a script can accidently turn it off
> > and all existing kprobe+bpf stuff that is ftrace based will
> > be silently switched off.
> 
> See http://lkml.kernel.org/r/20191016113316.13415-1-mbenes@suse.cz
> 
> I can (and should) add the PERMANENT flag to the direct_ops.
> 
> Note, the PERMANENT patches will be added before this one.

Ahh. Perfect. That works.
I was wondering whether livepatching should care...
clearly they are :)

> > Fast forward a year and imagine few host critical bpf progs
> > are running in production and relying on FL_DIRECT.
> > Now somebody decided to test new ftrace feature and
> > it hit one of FTRACE_WARN_ON().
> > That will shutdown the whole ftrace and bpf progs
> > will stop executing. I'd like to avoid that.
> > May be I misread the code?
> 
> It basically freezes it. Current registered ftrace_ops will not be
> touched. But nothing can be removed or added.

got it. that makes sense.

> 
> OK, I'll work to get this patch in for the next merge window.

As soon as you do first round of cleanups please ping me with the link
to your latest branch. I'll start building on top in the mean time.
Thanks!