From: Thomas Garnier via Virtualization Subject: Re: [PATCH v3 21/27] x86/ftrace: Adapt function tracing for PIE support Date: Tue, 29 May 2018 11:37:45 -0700 Message-ID: References: <20180523195421.180248-1-thgarnie@google.com> <20180523195421.180248-22-thgarnie@google.com> <20180524114024.pa67zjipy5qcg4tm@pathway.suse.cz> <20180524161622.4202319f@gandalf.local.home> Reply-To: Thomas Garnier Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: Kate Stewart , Nicolas Pitre , the arch/x86 maintainers , Sergey Senozhatsky , Len Brown , Tom Lendacky , Peter Zijlstra , Yonghong Song , Christopher Li , Dave Hansen , Dominik Brodowski , LKML , Masahiro Yamada , Jan Beulich , Pavel Machek , "H . Peter Anvin" , Kernel Hardening , Christoph Lameter , Alok Kataria , Linux Doc Mailing List , linux-arch , Jonathan Corbet Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: linux-crypto.vger.kernel.org On Thu, May 24, 2018 at 1:41 PM Thomas Garnier wrote: > On Thu, May 24, 2018 at 1:16 PM Steven Rostedt wrote: > > On Thu, 24 May 2018 13:40:24 +0200 > > Petr Mladek wrote: > > > On Wed 2018-05-23 12:54:15, Thomas Garnier wrote: > > > > When using -fPIE/PIC with function tracing, the compiler generates a > > > > call through the GOT (call *__fentry__@GOTPCREL). This instruction > > > > takes 6 bytes instead of 5 on the usual relative call. > > > > > > > > If PIE is enabled, replace the 6th byte of the GOT call by a 1-byte > nop > > > > so ftrace can handle the previous 5-bytes as before. > > > > > > > > Position Independent Executable (PIE) support will allow to extended > the > > > > KASLR randomization range below the -2G memory limit. > > > > > > > > Signed-off-by: Thomas Garnier > > > > --- > > > > arch/x86/include/asm/ftrace.h | 6 +++-- > > > > arch/x86/include/asm/sections.h | 4 ++++ > > > > arch/x86/kernel/ftrace.c | 42 > +++++++++++++++++++++++++++++++-- > > > > 3 files changed, 48 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/x86/include/asm/ftrace.h > b/arch/x86/include/asm/ftrace.h > > > > index c18ed65287d5..8f2decce38d8 100644 > > > > --- a/arch/x86/include/asm/ftrace.h > > > > +++ b/arch/x86/include/asm/ftrace.h > > > > @@ -25,9 +25,11 @@ extern void __fentry__(void); > > > > static inline unsigned long ftrace_call_adjust(unsigned long addr) > > > > { > > > > /* > > > > - * addr is the address of the mcount call instruction. > > > > - * recordmcount does the necessary offset calculation. > > > > + * addr is the address of the mcount call instruction. PIE has > always a > > > > + * byte added to the start of the function. > > > > */ > > > > + if (IS_ENABLED(CONFIG_X86_PIE)) > > > > + addr -= 1; > > > > > > This seems to modify the address even for modules that are _not_ > compiled with > > > PIE, see below. > > Can one load a module not compiled for PIE in a kernel with PIE? > > > > > > > return addr; > > > > } > > > > > > > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > > > > index 01ebcb6f263e..73b3c30cb7a3 100644 > > > > --- a/arch/x86/kernel/ftrace.c > > > > +++ b/arch/x86/kernel/ftrace.c > > > > @@ -135,6 +135,44 @@ ftrace_modify_code_direct(unsigned long ip, > unsigned const char *old_code, > > > > return 0; > > > > } > > > > > > > > +/* Bytes before call GOT offset */ > > > > +const unsigned char got_call_preinsn[] = { 0xff, 0x15 }; > > > > + > > > > +static int > > > > +ftrace_modify_initial_code(unsigned long ip, unsigned const char > *old_code, > > > > + unsigned const char *new_code) > > > > +{ > > > > + unsigned char replaced[MCOUNT_INSN_SIZE + 1]; > > > > + > > > > + ftrace_expected = old_code; > > > > + > > > > + /* > > > > + * If PIE is not enabled or no GOT call was found, default to the > > > > + * original approach to code modification. > > > > + */ > > > > + if (!IS_ENABLED(CONFIG_X86_PIE) || > > > > + probe_kernel_read(replaced, (void *)ip, sizeof(replaced)) || > > > > + memcmp(replaced, got_call_preinsn, sizeof(got_call_preinsn))) > > > > + return ftrace_modify_code_direct(ip, old_code, new_code); > > > > > > And this looks like an attempt to handle modules compiled without > > > PIE. Does it works with the right ip in that case? > > I'm guessing the || is for the "or no GOT call was found", but it > > doesn't explain why no GOT would be found. > Yes, maybe I could have made it work by using text_ip_addr earlier. > > > > > > I wonder if a better solution would be to update > > > scripts/recordmcount.c to store the incremented location into the > module. > I will look into it. Found a way to properly change the __mcount_loc using the preprocessing (removing the need for -1 on the addr). It will be part of the next version. Thanks for the feedback. > > If recordmcount.c can handle this, then I think that's the preferred > > approach. Thanks! > > -- Steve > > > > > > IMPORTANT: I have only vague picture about how this all works. It is > > > possible that I am completely wrong. The code might be correct, > > > especially if you tested this situation. > > > > > > Best Regards, > > > Petr > -- > Thomas -- Thomas