Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)
(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
> 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
> ---
> 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...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)
On Tue, 13 Jan 2015 23:47:57 +0100 (CET)
Jiri Kosina wrote:
> From: Jiri Kosina
> 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
> ---
> 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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ftrace: don't allow IPMODIFY without proper compiler support (was Re: Re: livepatching tree for linux-next)
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
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Re: livepatching tree for linux-next
(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...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
On Thursday, January 08, 2015 9:34 AM, Andrew Morton wrote:
> On Thu, 8 Jan 2015 01:11:03 +0100 (CET) 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 .. ?
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
On Thu, 8 Jan 2015 01:11:03 +0100 (CET) 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 .. ?
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
On Thu, 8 Jan 2015 00:49:49 +0100 (CET) Jiri Kosina 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
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
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
---
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
#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
On Thu, 8 Jan 2015 00:01:02 +0100 (CET) Jiri Kosina 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
On Fri, 26 Dec 2014 15:56:13 +1100 Stephen Rothwell 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
Hi Jiri, On Tue, 23 Dec 2014 09:10:56 -0600 Josh Poimboeuf 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 s...@canb.auug.org.au pgpOmXpDjUb0Q.pgp Description: OpenPGP digital signature
Re: livepatching tree for linux-next
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: livepatching tree for linux-next
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
