livepatching tree for linux-next

livepatching tree for linux-next

[Jiri Kosina]

Hi Stephen,

a substantial amount of work has been invested into abstracing "Live 
Patching" core functionality out of the already existing implementations, 
so that further improvements can be built on top of it in incremental 
steps.

The core functionality (which is self-contained) now works and has been 
Reviewed/Acked by both interested parties (i.e. people working on kPatch 
and kGraft) and agreed to be a common ground on which further development 
will happen.

We plan to send a pull request for 3.20, therefore I'd like to ask you to 
include 'for-next' branch of

	git://git.kernel.org/pub/scm/linux/kernel/git/jikos/livepatching.git

Into linux-next pile.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: livepatching tree for linux-next

[Christoph Hellwig]

On Mon, Dec 22, 2014 at 08:52:02PM +0100, Jiri Kosina wrote:
> Hi Stephen,
> 
> a substantial amount of work has been invested into abstracing "Live 
> Patching" core functionality out of the already existing implementations, 
> so that further improvements can be built on top of it in incremental 
> steps.
> 
> The core functionality (which is self-contained) now works and has been 
> Reviewed/Acked by both interested parties (i.e. people working on kPatch 
> and kGraft) and agreed to be a common ground on which further development 
> will happen.
> 
> We plan to send a pull request for 3.20, therefore I'd like to ask you to 
> include 'for-next' branch of

This is still missing the actual patch generators, which should be
merged together with the code, otherwise we'll get a mess with forever
out of tree tools like systemtap again.

Re: livepatching tree for linux-next

[Josh Poimboeuf]

On Tue, Dec 23, 2014 at 01:46:07AM -0800, Christoph Hellwig wrote:
> On Mon, Dec 22, 2014 at 08:52:02PM +0100, Jiri Kosina wrote:
> > Hi Stephen,
> > 
> > a substantial amount of work has been invested into abstracing "Live 
> > Patching" core functionality out of the already existing implementations, 
> > so that further improvements can be built on top of it in incremental 
> > steps.
> > 
> > The core functionality (which is self-contained) now works and has been 
> > Reviewed/Acked by both interested parties (i.e. people working on kPatch 
> > and kGraft) and agreed to be a common ground on which further development 
> > will happen.
> > 
> > We plan to send a pull request for 3.20, therefore I'd like to ask you to 
> > include 'for-next' branch of
> 
> This is still missing the actual patch generators, which should be
> merged together with the code, otherwise we'll get a mess with forever
> out of tree tools like systemtap again.

Well, a patch generator isn't required.  You can build a patch module
from source with kbuild, just like you would for any other kernel
module.  This is what kGraft already does today.  For example:

   https://git.kernel.org/cgit/linux/kernel/git/jikos/livepatching.git/commit/?h=for-next&id=13d1cf7e702596e0cd8ec62afa6bd49c431f2d0c

We do want to add a generator, but there are two of them out there that
need to be converged.  It doesn't make sense to do that work until we
have first converged the rest of the stack (namely, consistency models).
That's our next step.

The current code is by no means a final product, but it's still quite
useful already.

-- 
Josh

Re: livepatching tree for linux-next

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2762 bytes --]

Hi Jiri,

On Tue, 23 Dec 2014 09:10:56 -0600 Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Tue, Dec 23, 2014 at 01:46:07AM -0800, Christoph Hellwig wrote:
> > On Mon, Dec 22, 2014 at 08:52:02PM +0100, Jiri Kosina wrote:
> > > 
> > > a substantial amount of work has been invested into abstracing "Live 
> > > Patching" core functionality out of the already existing implementations, 
> > > so that further improvements can be built on top of it in incremental 
> > > steps.
> > > 
> > > The core functionality (which is self-contained) now works and has been 
> > > Reviewed/Acked by both interested parties (i.e. people working on kPatch 
> > > and kGraft) and agreed to be a common ground on which further development 
> > > will happen.
> > > 
> > > We plan to send a pull request for 3.20, therefore I'd like to ask you to 
> > > include 'for-next' branch of
> > 
> > This is still missing the actual patch generators, which should be
> > merged together with the code, otherwise we'll get a mess with forever
> > out of tree tools like systemtap again.
> 
> Well, a patch generator isn't required.  You can build a patch module
> from source with kbuild, just like you would for any other kernel
> module.  This is what kGraft already does today.  For example:
> 
>    https://git.kernel.org/cgit/linux/kernel/git/jikos/livepatching.git/commit/?h=for-next&id=13d1cf7e702596e0cd8ec62afa6bd49c431f2d0c
> 
> We do want to add a generator, but there are two of them out there that
> need to be converged.  It doesn't make sense to do that work until we
> have first converged the rest of the stack (namely, consistency models).
> That's our next step.
> 
> The current code is by no means a final product, but it's still quite
> useful already.

OK, I have added this from today but if it becomes an issue, I will
drop it again.  Lets see how it goes.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgment of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
        Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr@canb.auug.org.au

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: livepatching tree for linux-next

[Andrew Morton]

On Fri, 26 Dec 2014 15:56:13 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> OK, I have added this from today

My x86_64 allmodconfig broke.

In file included from include/linux/livepatch.h:29,
                 from kernel/livepatch/core.c:30:
./arch/x86/include/asm/livepatch.h:29:2: error: #error Your compiler must support -mfentry for live patching to work

Re: livepatching tree for linux-next

[Jiri Kosina]

On Wed, 7 Jan 2015, Andrew Morton wrote:

> > OK, I have added this from today
> 
> My x86_64 allmodconfig broke.
> 
> In file included from include/linux/livepatch.h:29,
>                  from kernel/livepatch/core.c:30:
> ./arch/x86/include/asm/livepatch.h:29:2: error: #error Your compiler must support -mfentry for live patching to work

[ adding Steven and Masami to CC, as this in some sense is related in 
  both to ftrace regs caller, and to IPMODIFY users in general ]

Well, if your gcc is too old (which is a fact detemined during build time, 
so there is no way to express this in Kconfig language in form of 
dependencies), we have to introduce build-time failure, as there is no way 
for this to work on compilers that don't support fentry on x86_64.

The only remaining option is to let the code build, pretend that 
everything is working, but do something like

#ifndef CC_USING_FENTRY
	printk("The compiler you used to compile your kernel was ancient "
		"there is no way for you to make use of this feature\n");
	return -EINVAL;
#endif

or so ... which I personally detest even more.

-- 
Jiri Kosina
SUSE Labs

Re: livepatching tree for linux-next

[Andrew Morton]

On Thu, 8 Jan 2015 00:01:02 +0100 (CET) Jiri Kosina <jkosina@suse.cz> wrote:

> On Wed, 7 Jan 2015, Andrew Morton wrote:
> 
> > > OK, I have added this from today
> > 
> > My x86_64 allmodconfig broke.
> > 
> > In file included from include/linux/livepatch.h:29,
> >                  from kernel/livepatch/core.c:30:
> > ./arch/x86/include/asm/livepatch.h:29:2: error: #error Your compiler must support -mfentry for live patching to work
> 
> [ adding Steven and Masami to CC, as this in some sense is related in 
>   both to ftrace regs caller, and to IPMODIFY users in general ]
> 
> Well, if your gcc is too old (which is a fact detemined during build time, 
> so there is no way to express this in Kconfig language in form of 
> dependencies), we have to introduce build-time failure, as there is no way 
> for this to work on compilers that don't support fentry on x86_64.
> 
> The only remaining option is to let the code build, pretend that 
> everything is working, but do something like
> 
> #ifndef CC_USING_FENTRY
> 	printk("The compiler you used to compile your kernel was ancient "
> 		"there is no way for you to make use of this feature\n");
> 	return -EINVAL;
> #endif
> 
> or so ... which I personally detest even more.

um, what's so special about this patchset that it gets to be the first
code in the history of the kernel which breaks allmodconfig?

Please find a way to fix it.  Copying CONFIG_CC_STACKPROTECTOR is one way.

Re: livepatching tree for linux-next

[Jiri Kosina]

On Wed, 7 Jan 2015, Andrew Morton wrote:

> Please find a way to fix it.  Copying CONFIG_CC_STACKPROTECTOR is one way.

Hmm ... is that actually really a good example? 

I think it will warn (explicitly from the top-level Makefile so that you 
are aware why the things that will follow are happening), and then let gcc 
fail the build due to unsupported option anyway ... because it will 
(deliberately, so that the build fails) keep the 'stackp-flag' set even 
for unsupported compiler options. No?

If we really don't want to be causing this though, how about the below 
instead?



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] livepatching: handle ancient compilers with more grace

We are aborting a build in case when gcc doesn't support fentry on x86_64 
(regs->ip modification can't really reliably work with mcount).

This however breaks allmodconfig for people with older gccs that don't 
support -mfentry.

Turn the build-time failure into runtime failure, resulting in the whole 
infrastructure not being initialized if CC_USING_FENTRY is unset.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/include/asm/livepatch.h | 6 +++++-
 kernel/livepatch/core.c          | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/livepatch.h b/arch/x86/include/asm/livepatch.h
index b5608d7..fa7f448 100644
--- a/arch/x86/include/asm/livepatch.h
+++ b/arch/x86/include/asm/livepatch.h
@@ -25,9 +25,13 @@
 #include <linux/ftrace.h>
 
 #ifdef CONFIG_LIVE_PATCHING
+static inline int klp_check_compiler_support(void)
+{
 #ifndef CC_USING_FENTRY
-#error Your compiler must support -mfentry for live patching to work
+	        return 1;
 #endif
+	        return 0;
+}
 extern int klp_write_module_reloc(struct module *mod, unsigned long type,
 				  unsigned long loc, unsigned long value);
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 6f63879..ce42d3b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -911,6 +911,12 @@ static int klp_init(void)
 {
 	int ret;
 
+	ret = klp_check_compiler_support();
+	if (ret) {
+		pr_info("Your compiler is too old; turning off.\n");
+		return -EINVAL;
+	}
+
 	ret = register_module_notifier(&klp_module_nb);
 	if (ret)
 		return ret;

-- 
Jiri Kosina
SUSE Labs

Re: livepatching tree for linux-next

[Andrew Morton]

On Thu, 8 Jan 2015 00:49:49 +0100 (CET) Jiri Kosina <jkosina@suse.cz> wrote:

> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -911,6 +911,12 @@ static int klp_init(void)
>  {
>  	int ret;
>  
> +	ret = klp_check_compiler_support();
> +	if (ret) {
> +		pr_info("Your compiler is too old; turning off.\n");
> +		return -EINVAL;
> +	}
> +

Looks reasonable.  It's a shame we've never figured out a way to do
this at Kconfig-time.


That second #error doing in livepatch.h is a bit odd.  It errors out if
anyone includes livepatch.h when CONFIG_LIVE_PATCHING=n.  Methinks it
would be saner to change it to

#error include linux/livepatch.h, not asm/livepatch.h

Re: livepatching tree for linux-next

[Jiri Kosina]

On Wed, 7 Jan 2015, Andrew Morton wrote:

> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -911,6 +911,12 @@ static int klp_init(void)
> >  {
> >  	int ret;
> >  
> > +	ret = klp_check_compiler_support();
> > +	if (ret) {
> > +		pr_info("Your compiler is too old; turning off.\n");
> > +		return -EINVAL;
> > +	}
> > +
> 
> Looks reasonable.  

Thanks. Can I treat this as your Reported-and-tested-by .. ?

> It's a shame we've never figured out a way to do this at Kconfig-time.

That's something that has been on my TODO list for very long time (this is 
not the first time I've been biten by that), but unfortunately rather low. 
I will talk to Michal Marek to see whether he doesn't have any idea how to 
implement this in an elegant way ... but let's keep that separate from 
this thread.

In any case, Masami, I really think you would like to do something like 
that for IPMODIFY as well ... or are you deliberately defering the 
responsibility to handle the possible mcount fallout to the ftrace_ops 
owner?

> That second #error doing in livepatch.h is a bit odd.  It errors out if 
> anyone includes livepatch.h when CONFIG_LIVE_PATCHING=n.  Methinks it 
> would be saner to change it to
> 
> #error include linux/livepatch.h, not asm/livepatch.h

I guess that's a nice cleanup. Noted, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: livepatching tree for linux-next

[Andrew Morton]

On Thu, 8 Jan 2015 01:11:03 +0100 (CET) Jiri Kosina <jkosina@suse.cz> wrote:

> On Wed, 7 Jan 2015, Andrew Morton wrote:
> 
> > > --- a/kernel/livepatch/core.c
> > > +++ b/kernel/livepatch/core.c
> > > @@ -911,6 +911,12 @@ static int klp_init(void)
> > >  {
> > >  	int ret;
> > >  
> > > +	ret = klp_check_compiler_support();
> > > +	if (ret) {
> > > +		pr_info("Your compiler is too old; turning off.\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > Looks reasonable.  
> 
> Thanks. Can I treat this as your Reported-and-tested-by .. ?

I compile tested it.  Then I got bored trying to hunt down all the
Kconfig things I needed to turn on to runtime test it ;)

btw, I suggest using HAVE_LIVE_PATCHING instead of
ARCH_HAVE_LIVE_PATCHING.  because this:

akpm3:/usr/src/25> grep " HAVE_" arch/x86/Kconfig | wc -l
67
akpm3:/usr/src/25> grep " ARCH_HAVE_" arch/x86/Kconfig | wc -l  
3
akpm3:/usr/src/25> grep " ARCH_HAS_" arch/x86/Kconfig | wc -l   
7
akpm3:/usr/src/25> grep " HAVE_ARCH" arch/x86/Kconfig | wc -l         
8
akpm3:/usr/src/25> grep " ARCH_USE" arch/x86/Kconfig | wc -l     
4
akpm3:/usr/src/25> grep " ARCH_WANT" arch/x86/Kconfig | wc -l   
6

makes me cry.

Re: livepatching tree for linux-next

[Jingoo Han]

On Thursday, January 08, 2015 9:34 AM, Andrew Morton wrote:
> On Thu, 8 Jan 2015 01:11:03 +0100 (CET) Jiri Kosina <jkosina@suse.cz> wrote:
> 
> > On Wed, 7 Jan 2015, Andrew Morton wrote:
> >
> > > > --- a/kernel/livepatch/core.c
> > > > +++ b/kernel/livepatch/core.c
> > > > @@ -911,6 +911,12 @@ static int klp_init(void)
> > > >  {
> > > >  	int ret;
> > > >
> > > > +	ret = klp_check_compiler_support();
> > > > +	if (ret) {
> > > > +		pr_info("Your compiler is too old; turning off.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > >
> > > Looks reasonable.
> >
> > Thanks. Can I treat this as your Reported-and-tested-by .. ?
> 
> I compile tested it.  Then I got bored trying to hunt down all the
> Kconfig things I needed to turn on to runtime test it ;)
> 
> btw, I suggest using HAVE_LIVE_PATCHING instead of
> ARCH_HAVE_LIVE_PATCHING.  because this:

I agree with your opinion. :-)
Using 'HAVE_LIVE_PATCHING' looks better.

Best regards,
Jingoo Han

> 
> akpm3:/usr/src/25> grep " HAVE_" arch/x86/Kconfig | wc -l
> 67
> akpm3:/usr/src/25> grep " ARCH_HAVE_" arch/x86/Kconfig | wc -l
> 3
> akpm3:/usr/src/25> grep " ARCH_HAS_" arch/x86/Kconfig | wc -l
> 7
> akpm3:/usr/src/25> grep " HAVE_ARCH" arch/x86/Kconfig | wc -l
> 8
> akpm3:/usr/src/25> grep " ARCH_USE" arch/x86/Kconfig | wc -l
> 4
> akpm3:/usr/src/25> grep " ARCH_WANT" arch/x86/Kconfig | wc -l
> 6
> 
> makes me cry.

Re: livepatching tree for linux-next

[Jiri Kosina]

On Wed, 7 Jan 2015, Andrew Morton wrote:

> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -911,6 +911,12 @@ static int klp_init(void)
> >  {
> >  	int ret;
> >  
> > +	ret = klp_check_compiler_support();
> > +	if (ret) {
> > +		pr_info("Your compiler is too old; turning off.\n");
> > +		return -EINVAL;
> > +	}
> > +
> 
> Looks reasonable.  

I've applied it and pushed out.

> It's a shame we've never figured out a way to do this at Kconfig-time.

The biggest problems probably is: what if you supply different CC during 
build? Then you'll have to invalidate all dependencies and restart the 
whole dependency resolution. The same goes for cross-compilation I guess.

-- 
Jiri Kosina
SUSE Labs

Re: Re: livepatching tree for linux-next

[Masami Hiramatsu]

(2015/01/08 9:11), Jiri Kosina wrote:
> On Wed, 7 Jan 2015, Andrew Morton wrote:
> 
>>> --- a/kernel/livepatch/core.c
>>> +++ b/kernel/livepatch/core.c
>>> @@ -911,6 +911,12 @@ static int klp_init(void)
>>>  {
>>>  	int ret;
>>>  
>>> +	ret = klp_check_compiler_support();
>>> +	if (ret) {
>>> +		pr_info("Your compiler is too old; turning off.\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>
>> Looks reasonable.  
> 
> Thanks. Can I treat this as your Reported-and-tested-by .. ?
> 
>> It's a shame we've never figured out a way to do this at Kconfig-time.
> 
> That's something that has been on my TODO list for very long time (this is 
> not the first time I've been biten by that), but unfortunately rather low. 
> I will talk to Michal Marek to see whether he doesn't have any idea how to 
> implement this in an elegant way ... but let's keep that separate from 
> this thread.
> 
> In any case, Masami, I really think you would like to do something like 
> that for IPMODIFY as well ... or are you deliberately defering the 
> responsibility to handle the possible mcount fallout to the ftrace_ops 
> owner?

Ah, good point. I just tried to use ftrace and WARN if not possible
to use it. I'll see it tomorrow. Anyway, I'd prefer to have this
kind of checking functionality in ftrace.

Thank you,

>> That second #error doing in livepatch.h is a bit odd.  It errors out if 
>> anyone includes livepatch.h when CONFIG_LIVE_PATCHING=n.  Methinks it 
>> would be saner to change it to
>>
>> #error include linux/livepatch.h, not asm/livepatch.h
> 
> I guess that's a nice cleanup. Noted, thanks.
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

[PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)

[Jiri Kosina]

On Mon, 12 Jan 2015, Masami Hiramatsu wrote:

> > In any case, Masami, I really think you would like to do something 
> > like that for IPMODIFY as well ... or are you deliberately defering 
> > the responsibility to handle the possible mcount fallout to the 
> > ftrace_ops owner?
> 
> Ah, good point. I just tried to use ftrace and WARN if not possible
> to use it. I'll see it tomorrow. Anyway, I'd prefer to have this
> kind of checking functionality in ftrace.

Okay, so how about something like this, for example ... ?



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support

Using IPMODIFY needs to be allowed only with compilers which are 
guaranteed to generate function prologues compatible with function 
redirection through changing instruction pointer in saved regs.

For example changing regs->ip on x86_64 in cases when gcc is using mcount 
(and not fentry) is not allowed, because at the time mcount call is 
issued, the original function's prologue has already been executed, which 
leads to all kinds of inconsistent havoc.

There is currently no way to express dependency on gcc features in 
Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
possible for Kconfig symbol to depend on it) so this needs to be checked 
in runtime.

Mark x86_64 with fentry supported for now. Other archs can be added 
gradually.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/include/asm/ftrace.h | 2 ++
 include/linux/ftrace.h        | 4 ++++
 kernel/trace/ftrace.c         | 5 +++++
 3 files changed, 11 insertions(+)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index f45acad..29fa417 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -4,8 +4,10 @@
 #ifdef CONFIG_FUNCTION_TRACER
 #ifdef CC_USING_FENTRY
 # define MCOUNT_ADDR		((long)(__fentry__))
+# define arch_ftrace_ipmodify_compiler_support(void) ({ 1; })
 #else
 # define MCOUNT_ADDR		((long)(mcount))
+#define arch_ftrace_ipmodify_compiler_support(void) ({ 0; })
 #endif
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
 
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..655ba99 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -244,6 +244,10 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
 			struct ftrace_ops *op, struct pt_regs *regs);
 
+#ifndef arch_ftrace_ipmodify_compiler_support
+/* let's not make any implicit assumptions about profiling call placement */
+# define arch_ftrace_ipmodify_compiler_support() { 0; }
+#endif
 #else /* !CONFIG_FUNCTION_TRACER */
 /*
  * (un)register_ftrace_function must be a macro since the ops parameter
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 929a733..11370fd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1809,6 +1809,11 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
 		return 0;
 
+	if (!arch_ftrace_ipmodify_compiler_support()) {
+		WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
+		return 0;
+	}
+
 	/*
 	 * Since the IPMODIFY is a very address sensitive action, we do not
 	 * allow ftrace_ops to set all functions to new hash.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)

[Steven Rostedt]

On Tue, 13 Jan 2015 23:47:57 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] ftrace: don't allow IPMODIFY without proper compiler
> support
> 
> Using IPMODIFY needs to be allowed only with compilers which are 
> guaranteed to generate function prologues compatible with function 
> redirection through changing instruction pointer in saved regs.
> 
> For example changing regs->ip on x86_64 in cases when gcc is using
> mcount (and not fentry) is not allowed, because at the time mcount
> call is issued, the original function's prologue has already been
> executed, which leads to all kinds of inconsistent havoc.
> 
> There is currently no way to express dependency on gcc features in 
> Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
> possible for Kconfig symbol to depend on it) so this needs to be
> checked in runtime.
> 
> Mark x86_64 with fentry supported for now. Other archs can be added 
> gradually.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  arch/x86/include/asm/ftrace.h | 2 ++
>  include/linux/ftrace.h        | 4 ++++
>  kernel/trace/ftrace.c         | 5 +++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/arch/x86/include/asm/ftrace.h
> b/arch/x86/include/asm/ftrace.h index f45acad..29fa417 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -4,8 +4,10 @@
>  #ifdef CONFIG_FUNCTION_TRACER
>  #ifdef CC_USING_FENTRY
>  # define MCOUNT_ADDR		((long)(__fentry__))
> +# define arch_ftrace_ipmodify_compiler_support(void) ({ 1; })
>  #else
>  # define MCOUNT_ADDR		((long)(mcount))
> +#define arch_ftrace_ipmodify_compiler_support(void) ({ 0; })
>  #endif
>  #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
>  
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1da6029..655ba99 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -244,6 +244,10 @@ static inline int
> ftrace_function_local_disabled(struct ftrace_ops *ops) extern void
> ftrace_stub(unsigned long a0, unsigned long a1, struct ftrace_ops
> *op, struct pt_regs *regs); 
> +#ifndef arch_ftrace_ipmodify_compiler_support
> +/* let's not make any implicit assumptions about profiling call
> placement */ +# define arch_ftrace_ipmodify_compiler_support() { 0; }

Is there any reason that this is a macro function? Why not just define:

#define ftrace_ipmodify_supported 0

?

-- Steve

> +#endif
>  #else /* !CONFIG_FUNCTION_TRACER */
>  /*
>   * (un)register_ftrace_function must be a macro since the ops
> parameter diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 929a733..11370fd 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1809,6 +1809,11 @@ static int
> __ftrace_hash_update_ipmodify(struct ftrace_ops *ops, if
> (!(ops->flags & FTRACE_OPS_FL_IPMODIFY)) return 0;
>  
> +	if (!arch_ftrace_ipmodify_compiler_support()) {
> +		WARN(1, "Your compiler doesn't support features
> necessary for IPMODIFY");
> +		return 0;
> +	}
> +
>  	/*
>  	 * Since the IPMODIFY is a very address sensitive action, we
> do not
>  	 * allow ftrace_ops to set all functions to new hash.
> 

Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)

[Masami Hiramatsu]

(2015/01/14 7:47), Jiri Kosina wrote:
> On Mon, 12 Jan 2015, Masami Hiramatsu wrote:
> 
>>> In any case, Masami, I really think you would like to do something 
>>> like that for IPMODIFY as well ... or are you deliberately defering 
>>> the responsibility to handle the possible mcount fallout to the 
>>> ftrace_ops owner?
>>
>> Ah, good point. I just tried to use ftrace and WARN if not possible
>> to use it. I'll see it tomorrow. Anyway, I'd prefer to have this
>> kind of checking functionality in ftrace.
> 
> Okay, so how about something like this, for example ... ?

Thanks! Could you read my comments?

> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support
> 
> Using IPMODIFY needs to be allowed only with compilers which are 
> guaranteed to generate function prologues compatible with function 
> redirection through changing instruction pointer in saved regs.
> 
> For example changing regs->ip on x86_64 in cases when gcc is using mcount 
> (and not fentry) is not allowed, because at the time mcount call is 
> issued, the original function's prologue has already been executed, which 
> leads to all kinds of inconsistent havoc.
> 
> There is currently no way to express dependency on gcc features in 
> Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
> possible for Kconfig symbol to depend on it) so this needs to be checked 
> in runtime.
> 
> Mark x86_64 with fentry supported for now. Other archs can be added 
> gradually.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  arch/x86/include/asm/ftrace.h | 2 ++
>  include/linux/ftrace.h        | 4 ++++
>  kernel/trace/ftrace.c         | 5 +++++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index f45acad..29fa417 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -4,8 +4,10 @@
>  #ifdef CONFIG_FUNCTION_TRACER
>  #ifdef CC_USING_FENTRY
>  # define MCOUNT_ADDR		((long)(__fentry__))
> +# define arch_ftrace_ipmodify_compiler_support(void) ({ 1; })
>  #else
>  # define MCOUNT_ADDR		((long)(mcount))
> +#define arch_ftrace_ipmodify_compiler_support(void) ({ 0; })

Hmm, can we just define ARCH_FTRACE_SUPPORT_IPMODIFY here?

>  #endif
>  #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
>  
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1da6029..655ba99 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -244,6 +244,10 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
>  extern void ftrace_stub(unsigned long a0, unsigned long a1,
>  			struct ftrace_ops *op, struct pt_regs *regs);
>  
> +#ifndef arch_ftrace_ipmodify_compiler_support
> +/* let's not make any implicit assumptions about profiling call placement */
> +# define arch_ftrace_ipmodify_compiler_support() { 0; }
> +#endif
>  #else /* !CONFIG_FUNCTION_TRACER */
>  /*
>   * (un)register_ftrace_function must be a macro since the ops parameter
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 929a733..11370fd 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1809,6 +1809,11 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
>  	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
>  		return 0;
>  
> +	if (!arch_ftrace_ipmodify_compiler_support()) {
> +		WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
> +		return 0;
> +	}

Actually, if ftrace doesn't support IPMODIFY, I would like to just drop
the entire code for CONFIG_KPROBES_ON_FTRACE(this is a hidden config),
instead of checking at runtime.

So, there are 2 ifdefs of code in kernel/kprobes.c for
CONFIG_KPROBES_ON_FTRACE, those should also check
ARCH_FTRACE_SUPPORT_IPMODIFY too.

Thank you,

> +
>  	/*
>  	 * Since the IPMODIFY is a very address sensitive action, we do not
>  	 * allow ftrace_ops to set all functions to new hash.
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)

[Jiri Kosina]

On Wed, 14 Jan 2015, Masami Hiramatsu wrote:

> > diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> > index f45acad..29fa417 100644
> > --- a/arch/x86/include/asm/ftrace.h
> > +++ b/arch/x86/include/asm/ftrace.h
> > @@ -4,8 +4,10 @@
> >  #ifdef CONFIG_FUNCTION_TRACER
> >  #ifdef CC_USING_FENTRY
> >  # define MCOUNT_ADDR		((long)(__fentry__))
> > +# define arch_ftrace_ipmodify_compiler_support(void) ({ 1; })
> >  #else
> >  # define MCOUNT_ADDR		((long)(mcount))
> > +#define arch_ftrace_ipmodify_compiler_support(void) ({ 0; })
> 
> Hmm, can we just define ARCH_FTRACE_SUPPORT_IPMODIFY here?

Yup, that's more or less what Steven suggested as well. I'll send v2.

> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 929a733..11370fd 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -1809,6 +1809,11 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
> >  	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
> >  		return 0;
> >  
> > +	if (!arch_ftrace_ipmodify_compiler_support()) {
> > +		WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
> > +		return 0;
> > +	}
> 
> Actually, if ftrace doesn't support IPMODIFY, I would like to just drop
> the entire code for CONFIG_KPROBES_ON_FTRACE(this is a hidden config),
> instead of checking at runtime.
> 
> So, there are 2 ifdefs of code in kernel/kprobes.c for 
> CONFIG_KPROBES_ON_FTRACE, those should also check 
> ARCH_FTRACE_SUPPORT_IPMODIFY too.

Makes sense. Will send that as a followup patch once the way how to do 
this in ftrace is settled.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH 1/2 v2] ftrace: don't allow IPMODIFY without proper compiler support

[Jiri Kosina]

Using IPMODIFY needs to be allowed only with compilers which are 
guaranteed to generate function prologues compatible with function 
redirection through changing instruction pointer in saved regs.

For example changing regs->ip on x86_64 in cases when gcc is using mcount 
(and not fentry) is not allowed, because at the time mcount call is 
issued, the original function's prologue has already been executed, which 
leads to all kinds of inconsistent havoc.

There is currently no way to express dependency on gcc features in 
Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
possible for Kconfig symbol to depend on it) so this needs to be checked 
in runtime.

Mark x86_64 with fentry supported for now. Other archs can be added
gradually.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

v1 -> v2: turn macro function into a plain macro

 arch/x86/include/asm/ftrace.h | 2 ++
 include/linux/ftrace.h        | 5 +++++
 kernel/trace/ftrace.c         | 5 +++++
 3 files changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index f45acad..f84eaaf 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -4,8 +4,10 @@
 #ifdef CONFIG_FUNCTION_TRACER
 #ifdef CC_USING_FENTRY
 # define MCOUNT_ADDR		((long)(__fentry__))
+# define ftrace_ipmodify_supported 1
 #else
 # define MCOUNT_ADDR		((long)(mcount))
+# define ftrace_ipmodify_supported 0
 #endif
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
 
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..837153a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -244,6 +244,11 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
 			struct ftrace_ops *op, struct pt_regs *regs);
 
+#ifndef ftrace_ipmodify_supported
+/* let's not make any implicit assumptions about profiling call placement */
+# define ftrace_ipmodify_supported 0
+
+#endif
 #else /* !CONFIG_FUNCTION_TRACER */
 /*
  * (un)register_ftrace_function must be a macro since the ops parameter
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 929a733..05af4cd 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1809,6 +1809,11 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
 	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
 		return 0;
 
+	if (!ftrace_ipmodify_supported) {
+		WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
+		return 0;
+	}
+
 	/*
 	 * Since the IPMODIFY is a very address sensitive action, we do not
 	 * allow ftrace_ops to set all functions to new hash.

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/2] kprobes: compile out IPMODIFY support if ftrace doesn't support it

[Jiri Kosina]

If ftrace doesn't claim to support IPMODIFY support, there is no need to 
compile IPMODIFY support in kprobes either.

Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 kernel/kprobes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 06f5830..49a69d7 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -912,7 +912,7 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 }
 #endif /* CONFIG_OPTPROBES */
 
-#ifdef CONFIG_KPROBES_ON_FTRACE
+#if defined(CONFIG_KPROBES_ON_FTRACE) && (ftrace_ipmodify_supported == 1)
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
 	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
@@ -957,7 +957,7 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 			   (unsigned long)p->addr, 1, 0);
 	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
 }
-#else	/* !CONFIG_KPROBES_ON_FTRACE */
+#else	/* !CONFIG_KPROBES_ON_FTRACE || ftrace_ipmodify_supported == 0 */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)	do {} while (0)
 #define disarm_kprobe_ftrace(p)	do {} while (0)
@@ -1416,12 +1416,12 @@ int __weak arch_check_ftrace_location(struct kprobe *p)
 
 	ftrace_addr = ftrace_location((unsigned long)p->addr);
 	if (ftrace_addr) {
-#ifdef CONFIG_KPROBES_ON_FTRACE
+#if defined(CONFIG_KPROBES_ON_FTRACE) && (ftrace_ipmodify_supported == 1)
 		/* Given address is not on the instruction boundary */
 		if ((unsigned long)p->addr != ftrace_addr)
 			return -EILSEQ;
 		p->flags |= KPROBE_FLAG_FTRACE;
-#else	/* !CONFIG_KPROBES_ON_FTRACE */
+#else	/* !CONFIG_KPROBES_ON_FTRACE || ftrace_ipmodify_supported == 0 */
 		return -EINVAL;
 #endif
 	}

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/2] kprobes: compile out IPMODIFY support if ftrace doesn't support it

[Masami Hiramatsu]

(2015/01/14 17:48), Jiri Kosina wrote:
> If ftrace doesn't claim to support IPMODIFY support, there is no need to 
> compile IPMODIFY support in kprobes either.
> 

This looks good to me :)

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thanks!

> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  kernel/kprobes.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 06f5830..49a69d7 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -912,7 +912,7 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
>  }
>  #endif /* CONFIG_OPTPROBES */
>  
> -#ifdef CONFIG_KPROBES_ON_FTRACE
> +#if defined(CONFIG_KPROBES_ON_FTRACE) && (ftrace_ipmodify_supported == 1)
>  static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
>  	.func = kprobe_ftrace_handler,
>  	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
> @@ -957,7 +957,7 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
>  			   (unsigned long)p->addr, 1, 0);
>  	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
>  }
> -#else	/* !CONFIG_KPROBES_ON_FTRACE */
> +#else	/* !CONFIG_KPROBES_ON_FTRACE || ftrace_ipmodify_supported == 0 */
>  #define prepare_kprobe(p)	arch_prepare_kprobe(p)
>  #define arm_kprobe_ftrace(p)	do {} while (0)
>  #define disarm_kprobe_ftrace(p)	do {} while (0)
> @@ -1416,12 +1416,12 @@ int __weak arch_check_ftrace_location(struct kprobe *p)
>  
>  	ftrace_addr = ftrace_location((unsigned long)p->addr);
>  	if (ftrace_addr) {
> -#ifdef CONFIG_KPROBES_ON_FTRACE
> +#if defined(CONFIG_KPROBES_ON_FTRACE) && (ftrace_ipmodify_supported == 1)
>  		/* Given address is not on the instruction boundary */
>  		if ((unsigned long)p->addr != ftrace_addr)
>  			return -EILSEQ;
>  		p->flags |= KPROBE_FLAG_FTRACE;
> -#else	/* !CONFIG_KPROBES_ON_FTRACE */
> +#else	/* !CONFIG_KPROBES_ON_FTRACE || ftrace_ipmodify_supported == 0 */
>  		return -EINVAL;
>  #endif
>  	}
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 1/2 v2] ftrace: don't allow IPMODIFY without proper compiler support

[Masami Hiramatsu]

(2015/01/14 17:47), Jiri Kosina wrote:
> Using IPMODIFY needs to be allowed only with compilers which are 
> guaranteed to generate function prologues compatible with function 
> redirection through changing instruction pointer in saved regs.
> 
> For example changing regs->ip on x86_64 in cases when gcc is using mcount 
> (and not fentry) is not allowed, because at the time mcount call is 
> issued, the original function's prologue has already been executed, which 
> leads to all kinds of inconsistent havoc.
> 
> There is currently no way to express dependency on gcc features in 
> Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
> possible for Kconfig symbol to depend on it) so this needs to be checked 
> in runtime.
> 
> Mark x86_64 with fentry supported for now. Other archs can be added
> gradually.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
> 
> v1 -> v2: turn macro function into a plain macro
> 
>  arch/x86/include/asm/ftrace.h | 2 ++
>  include/linux/ftrace.h        | 5 +++++
>  kernel/trace/ftrace.c         | 5 +++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
> index f45acad..f84eaaf 100644
> --- a/arch/x86/include/asm/ftrace.h
> +++ b/arch/x86/include/asm/ftrace.h
> @@ -4,8 +4,10 @@
>  #ifdef CONFIG_FUNCTION_TRACER
>  #ifdef CC_USING_FENTRY
>  # define MCOUNT_ADDR		((long)(__fentry__))
> +# define ftrace_ipmodify_supported 1
>  #else
>  # define MCOUNT_ADDR		((long)(mcount))
> +# define ftrace_ipmodify_supported 0
>  #endif
>  #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
>  
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 1da6029..837153a 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -244,6 +244,11 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
>  extern void ftrace_stub(unsigned long a0, unsigned long a1,
>  			struct ftrace_ops *op, struct pt_regs *regs);
>  
> +#ifndef ftrace_ipmodify_supported
> +/* let's not make any implicit assumptions about profiling call placement */
> +# define ftrace_ipmodify_supported 0
> +
> +#endif
>  #else /* !CONFIG_FUNCTION_TRACER */
>  /*
>   * (un)register_ftrace_function must be a macro since the ops parameter
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 929a733..05af4cd 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1809,6 +1809,11 @@ static int __ftrace_hash_update_ipmodify(struct ftrace_ops *ops,
>  	if (!(ops->flags & FTRACE_OPS_FL_IPMODIFY))
>  		return 0;
>  
> +	if (!ftrace_ipmodify_supported) {
> +		WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
> +		return 0;
> +	}
> +

Hmm, if this binary doesn't support IPMODIFY, it should return -ENOTSUPP.
And also, IMHO, we'd better reject registering ftrace_ops with IPMODIFY
in this situation before updating hash table.

Thank you,

>  	/*
>  	 * Since the IPMODIFY is a very address sensitive action, we do not
>  	 * allow ftrace_ops to set all functions to new hash.
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 1/2 v2] ftrace: don't allow IPMODIFY without proper compiler support

[Jiri Kosina]

On Thu, 15 Jan 2015, Masami Hiramatsu wrote:

> Hmm, if this binary doesn't support IPMODIFY, it should return -ENOTSUPP.

Good poing, will send v3.

> And also, IMHO, we'd better reject registering ftrace_ops with IPMODIFY 
> in this situation before updating hash table.

That would happen automatically if we return -ENOTSUPP, right? Because the 
check in ftrace_hash_move() would cause freeing of the new_hash and the 
error value would get propagated out to ftrace_hash_move() callers.

-- 
Jiri Kosina
SUSE Labs

[PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support

[Jiri Kosina]

Using IPMODIFY needs to be allowed only with compilers which are 
guaranteed to generate function prologues compatible with function 
redirection through changing instruction pointer in saved regs.

For example changing regs->ip on x86_64 in cases when gcc is using mcount 
(and not fentry) is not allowed, because at the time mcount call is 
issued, the original function's prologue has already been executed, which 
leads to all kinds of inconsistent havoc.

There is currently no way to express dependency on gcc features in 
Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
possible for Kconfig symbol to depend on it) so this needs to be checked 
in runtime.

Mark x86_64 with fentry supported for now. Other archs can be added
gradually.

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

v1 -> v2: turn macro function into a plain macro
v2 -> v3: return ENOTSUPP and reject registering ftrace_ops in 
	  ftrace_hash_move() already

 arch/x86/include/asm/ftrace.h | 2 ++
 include/linux/ftrace.h        | 5 +++++
 kernel/trace/ftrace.c         | 5 +++++
 3 files changed, 12 insertions(+)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index f45acad..f84eaaf 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -4,8 +4,10 @@
 #ifdef CONFIG_FUNCTION_TRACER
 #ifdef CC_USING_FENTRY
 # define MCOUNT_ADDR		((long)(__fentry__))
+# define ftrace_ipmodify_supported 1
 #else
 # define MCOUNT_ADDR		((long)(mcount))
+# define ftrace_ipmodify_supported 0
 #endif
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
 
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 1da6029..837153a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -244,6 +244,11 @@ static inline int ftrace_function_local_disabled(struct ftrace_ops *ops)
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
 			struct ftrace_ops *op, struct pt_regs *regs);
 
+#ifndef ftrace_ipmodify_supported
+/* let's not make any implicit assumptions about profiling call placement */
+# define ftrace_ipmodify_supported 0
+
+#endif
 #else /* !CONFIG_FUNCTION_TRACER */
 /*
  * (un)register_ftrace_function must be a macro since the ops parameter
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 929a733..3c1261c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1378,6 +1378,11 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
 	if (ops->flags & FTRACE_OPS_FL_IPMODIFY && !enable)
 		return -EINVAL;
 
+	if ((ops->flags & FTRACE_OPS_FL_IPMODIFY) && !ftrace_ipmodify_supported) {
+		WARN(1, "Your compiler doesn't support features necessary for IPMODIFY");
+		return -ENOTSUPP;
+	}
+
 	/*
 	 * If the new source is empty, just free dst and assign it
 	 * the empty_hash.

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/2 v3] kprobes: compile out IPMODIFY support if ftrace doesn't support it

[Jiri Kosina]

If ftrace doesn't claim to support IPMODIFY support, there is no need to 
compile IPMODIFY support in kprobes either.

Suggested-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

v1 -> v2: nothing
v2 -> v3: add Masami's ack and remove superfluous '== 1' comparsions

 kernel/kprobes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 06f5830..dfb296c 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -912,7 +912,7 @@ static struct kprobe *alloc_aggr_kprobe(struct kprobe *p)
 }
 #endif /* CONFIG_OPTPROBES */
 
-#ifdef CONFIG_KPROBES_ON_FTRACE
+#if defined(CONFIG_KPROBES_ON_FTRACE) && (ftrace_ipmodify_supported)
 static struct ftrace_ops kprobe_ftrace_ops __read_mostly = {
 	.func = kprobe_ftrace_handler,
 	.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_IPMODIFY,
@@ -957,7 +957,7 @@ static void disarm_kprobe_ftrace(struct kprobe *p)
 			   (unsigned long)p->addr, 1, 0);
 	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
 }
-#else	/* !CONFIG_KPROBES_ON_FTRACE */
+#else	/* !CONFIG_KPROBES_ON_FTRACE || !ftrace_ipmodify_supported */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
 #define arm_kprobe_ftrace(p)	do {} while (0)
 #define disarm_kprobe_ftrace(p)	do {} while (0)
@@ -1416,12 +1416,12 @@ int __weak arch_check_ftrace_location(struct kprobe *p)
 
 	ftrace_addr = ftrace_location((unsigned long)p->addr);
 	if (ftrace_addr) {
-#ifdef CONFIG_KPROBES_ON_FTRACE
+#if defined(CONFIG_KPROBES_ON_FTRACE) && (ftrace_ipmodify_supported)
 		/* Given address is not on the instruction boundary */
 		if ((unsigned long)p->addr != ftrace_addr)
 			return -EILSEQ;
 		p->flags |= KPROBE_FLAG_FTRACE;
-#else	/* !CONFIG_KPROBES_ON_FTRACE */
+#else	/* !CONFIG_KPROBES_ON_FTRACE || !ftrace_ipmodify_supported */
 		return -EINVAL;
 #endif
 	}
-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support

[Steven Rostedt]

On Thu, 15 Jan 2015 10:50:07 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:

> Using IPMODIFY needs to be allowed only with compilers which are 
> guaranteed to generate function prologues compatible with function 
> redirection through changing instruction pointer in saved regs.

That's actually not true.

Sorry, I started thinking about this more, and I disagree with this
change.

> 
> For example changing regs->ip on x86_64 in cases when gcc is using mcount 
> (and not fentry) is not allowed, because at the time mcount call is 
> issued, the original function's prologue has already been executed, which 
> leads to all kinds of inconsistent havoc.

No, it only causes havoc if whoever modifies the ip doesn't know it's
not at the start of the function.

Hence, it's only the user of ftrace that wants to replace functions
that needs to worry about this.

In fact, there's nothing wrong if kprobes to use ftrace to change ip
even if fentry isn't supported. That's because if fentry isn't
supported, kprobes will notice that there's no ftrace call at the start
of a function and will use a breakpoint instead. If a kprobe user still
wants to change the ip after the stack frame was set up, they still can
do that, they just need to find where the mcount call is. kprobe does
its work based on the kernel address to probe, not where ftrace places
its hooks.

Now you could argue that there's no reason to change ip if ftrace isn't
using fentry, but there's nothing to prevent a user from doing so.

It also bothers me that you just prevented all users of kprobes from
taking advantage of hooking to a ftrace caller if fentry isn't
supported. All kprobes really needs is the REGS version supported. 99%
of all kprobes do not modify ip.

I have to say NAK on this change.

Instead, make live kernel patching fail to load if fentry isn't
supported. IOW, instead of ftrace_ipmodify_supported, have a
live_kernel_patching_supported that could be based on fentry being used
or not.

-- Steve


> 
> There is currently no way to express dependency on gcc features in 
> Kconfig, (CC_USING_FENTRY is defined only during build, so it's not 
> possible for Kconfig symbol to depend on it) so this needs to be checked 
> in runtime.
> 
> Mark x86_64 with fentry supported for now. Other archs can be added
> gradually.
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support

[Jiri Kosina]

On Fri, 16 Jan 2015, Steven Rostedt wrote:

> Instead, make live kernel patching fail to load if fentry isn't 
> supported. IOW, instead of ftrace_ipmodify_supported, have a 
> live_kernel_patching_supported that could be based on fentry being used 
> or not.

I can live with that, we are handling non-fentry case ourselves already.

You expressed that you would accept such patch for ftrace in a past, so I 
thought I'll move it out from livepatching to ftrace. But if you don't 
want it now, that's not a big issue, and handling the possible fallout 
stays the responsibility of ftrace_ops "owner".

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2 v3] ftrace: don't allow IPMODIFY without proper compiler support

[Steven Rostedt]

On Fri, 16 Jan 2015 17:41:58 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:

> On Fri, 16 Jan 2015, Steven Rostedt wrote:
> 
> > Instead, make live kernel patching fail to load if fentry isn't 
> > supported. IOW, instead of ftrace_ipmodify_supported, have a 
> > live_kernel_patching_supported that could be based on fentry being used 
> > or not.
> 
> I can live with that, we are handling non-fentry case ourselves already.
> 
> You expressed that you would accept such patch for ftrace in a past, so I 

Right, but then I thought about it more :-)

I had a lot of time thinking about things like this while debugging
the dance between function graph and jprobes.

> thought I'll move it out from livepatching to ftrace. But if you don't 
> want it now, that's not a big issue, and handling the possible fallout 
> stays the responsibility of ftrace_ops "owner".
> 

Exactly!

-- Steve