linux-kernel - Re: [for-next][PATCH 2/4] ftrace/x86: Show trampoline call function in enabled

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: <20141106090157.5b2b562b@gandalf.local.home>
Date:	Thu, 6 Nov 2014 09:01:57 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Borislav Petkov <bp@...en8.de>
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Seth Jennings <sjenning@...hat.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	"H. Peter Anvin" <hpa@...ux.intel.com>
Subject: Re: [for-next][PATCH 2/4] ftrace/x86: Show trampoline call function
 in enabled_functions

On Wed, 5 Nov 2014 11:42:07 +0100
Borislav Petkov <bp@...en8.de> wrote:


> > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> > index e4d48f6cad86..ca17c20a1010 100644
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -48,7 +48,7 @@ int ftrace_arch_code_modify_post_process(void)
> >  union ftrace_code_union {
> >  	char code[MCOUNT_INSN_SIZE];
> >  	struct {
> > -		char e8;
> > +		unsigned char e8;
> 
> u8?
> 
> "u8 e8;" looks supa dupa though :-)

And compete with PowerPC's eieio command.

> 
> >  		int offset;
> >  	} __attribute__((packed));
> >  };
> > @@ -797,12 +797,26 @@ static unsigned long create_trampoline(struct ftrace_ops *ops)
> >  	return (unsigned long)trampoline;
> >  }
> >  
> > +static unsigned long calc_trampoline_call_offset(bool save_regs)
> > +{
> > +	unsigned long start_offset;
> > +	unsigned long call_offset;
> > +
> > +	if (save_regs) {
> > +		start_offset = (unsigned long)ftrace_regs_caller;
> > +		call_offset = (unsigned long)ftrace_regs_call;
> > +	} else {
> > +		start_offset = (unsigned long)ftrace_caller;
> > +		call_offset = (unsigned long)ftrace_call;
> > +	}
> > +
> > +	return call_offset - start_offset;
> > +}
> > +
> >  void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
> >  {
> >  	ftrace_func_t func;
> >  	unsigned char *new;
> > -	unsigned long start_offset;
> > -	unsigned long call_offset;
> >  	unsigned long offset;
> >  	unsigned long ip;
> >  	int ret;
> > @@ -820,15 +834,7 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
> >  			return;
> >  	}
> >  
> > -	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
> > -		start_offset = (unsigned long)ftrace_regs_caller;
> > -		call_offset = (unsigned long)ftrace_regs_call;
> > -	} else {
> > -		start_offset = (unsigned long)ftrace_caller;
> > -		call_offset = (unsigned long)ftrace_call;
> > -	}
> > -
> > -	offset = call_offset - start_offset;
> > +	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
> >  	ip = ops->trampoline + offset;
> >  
> >  	func = ftrace_ops_get_func(ops);
> > @@ -840,6 +846,74 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
> >  	/* The update should never fail */
> >  	WARN_ON(ret);
> >  }
> > +
> > +/* Return the address of the function the trampoline calls */
> > +static void *addr_from_call(void *ptr)
> > +{
> > +	union ftrace_code_union calc;
> > +	int ret;
> > +
> > +	ret = probe_kernel_read(&calc, ptr, MCOUNT_INSN_SIZE);
> > +	if (WARN_ON_ONCE(ret < 0))
> > +		return NULL;
> > +
> > +	/* Make sure this is a call */
> > +	if (WARN_ON_ONCE(calc.e8 != 0xe8)) {
> > +		pr_warn("Expected e8, got %x\n", calc.e8);
> 
> Simply WARN_ONCE gives you both.

Ah, could do that.

> 
> > +		return NULL;
> > +	}
> > +
> > +	return ptr + MCOUNT_INSN_SIZE + calc.offset;
> > +}
> > +
> > +void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
> > +			   unsigned long frame_pointer);
> > +
> > +/*
> > + * If the ops->trampoline was not allocated, then it probably
> > + * has a static trampoline func, or is the ftrace caller itself.
> > + */
> > +static void *static_tramp_func(struct ftrace_ops *ops, struct dyn_ftrace *rec)
> 
> I guess you can merge this one into its only caller below - it is not
> that huge... yet! :-)
> 

Hmm, it's big enough not to be in that function as a if statement that
will return anyway.

gcc will just make it inlined anyway. I think it's better separate for
readability.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/