[PATCH 3/3] early kprobes: x86: don't try to recover ftraced instruction before ftrace get ready.
Masami Hiramatsu
masami.hiramatsu.pt at hitachi.com
Wed Mar 4 07:26:48 PST 2015
Hi Wang,
(2015/03/04 20:22), Wang Nan wrote:
> Hi Masami,
>
> Following your advise, I adjusted early kprobe patches, added a
> kprobes_init_stage var to indicate the initiaization progress of
> kprobes. It has following avaliable values:
>
> typedef enum {
> /* kprobe initialization is error. */
> KP_STG_ERR = -1,
Eventually, all the negative values should be handled as an error,
then we can store an encoded error reason or location on it.
Anyway, at this point we don't need it.
> /* kprobe is not ready. */
> KP_STG_NONE,
Please put the comment on the same line, as below.
KP_STG_ERR = -1, /* kprobe initialization is error. */
KP_STG_NONE, /* kprobe is not ready. */
...
> /* kprobe is available. */
> KP_STG_AVAIL,
KP_STG_EARLY is better, isn't it? :)
> /* Ftrace initialize is ready */
> KP_STG_FTRACE_READY,
> /* All resources are ready */
> KP_STG_FULL,
> /*
> * Since memory system is ready before ftrace, after ftrace inited we
> * are able to alloc normal kprobes.
> */
> #define KP_STG_NORMAL KP_STG_FTRACE_READY
No need to use macro, you can directly define enum symbol.
KP_STG_NORMAL = KP_STG_FTRACE_READY
BTW, what's the difference of FULL and NORMAL?
> } kprobes_init_stage_t;
>
> And kprobes_is_early becomes:
>
> static inline bool kprobes_is_early(void)
> {
> return (kprobes_init_stage < KP_STG_NORMAL);
> }
>
> A helper function is add to make progress:
>
> static void kprobes_update_init_stage(kprobes_init_stage_t s)
> {
> BUG_ON((kprobes_init_stage == KP_STG_ERR) ||
> ((s <= kprobes_init_stage) && (s != KP_STG_ERR)));
> kprobes_init_stage = s;
> }
Good! this serializes the initialization stage :)
>
> Original kprobes_initialized, kprobes_blacklist_initialized
> kprobes_on_ftrace_initialized are all removed, replaced by:
>
> kprobes_initialized --> (kprobes_init_stage >= KP_STG_AVAIL)
> kprobes_blacklist_initialized --> (kprobes_init_stage >= KP_STG_FULL)
> kprobes_on_ftrace_initialized --> (kprobes_init_stage >= KP_STG_FTRACE_READY)
>
> respectively.
Yes, that is what I want.
>
> Following patch is extracted from my WIP v5 series and will be distributed
> into separated patches.
>
> (Please note that it is edited manually and unable to be applied directly.)
>
> Do you have any futher suggestion?
Sorry, I have still not reviewed your series yet, but it seems that your
patches are chopped to smaller pieces. I recommend you to fold them up to
better granularity, like
- Each patch can be compiled without warnings.
- If you introduce new functions, it should be called from somewhere in
the same patch. (no orphaned functions, except for module APIs)
- Bugfix, cleanup, and enhance it.
And now I'm considering that the early kprobe should be started as an
independent series from ftrace. For example, we can configure early_kprobe
only when KPROBE_ON_FTRACE=n. That will make it simpler and we can focus
on what we basically need to do.
Thank you,
>
> Thank you!
>
> ---
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 87beb64..f8b7dcb 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -234,6 +234,20 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> */
> if (WARN_ON(faddr && faddr != addr))
> return 0UL;
> +
> + /*
> + * If ftrace is not ready yet, pretend this is not an ftrace
> + * location, because currently the target instruction has not
> + * been replaced by a NOP yet. When ftrace trying to convert
> + * it to NOP, kprobe should be notified and the kprobe data
> + * should be fixed at that time.
> + *
> + * Since it is possible that an early kprobe already on that
> + * place, don't return addr directly.
> + */
> + if (unlikely(kprobes_init_stage < KP_STG_FTRACE_READY))
> + faddr = 0UL;
> +
> /*
> * Use the current code if it is not modified by Kprobe
> * and it cannot be modified by ftrace.
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 2d78bbb..04b97ec 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -50,7 +50,31 @@
> #define KPROBE_REENTER 0x00000004
> #define KPROBE_HIT_SSDONE 0x00000008
>
> -extern bool kprobes_is_early(void);
> +/* Initialization state of kprobe */
> +typedef enum {
> + /* kprobe initialization is error. */
> + KP_STG_ERR = -1,
> + /* kprobe is not ready. */
> + KP_STG_NONE,
> + /* kprobe is available. */
> + KP_STG_AVAIL,
> + /* Ftrace initialize is ready */
> + KP_STG_FTRACE_READY,
> + /* All resources are ready */
> + KP_STG_FULL,
> +/*
> + * Since memory system is ready before ftrace, after ftrace inited we
> + * are able to alloc normal kprobes.
> + */
> +#define KP_STG_NORMAL KP_STG_FTRACE_READY
> +} kprobes_init_stage_t;
> +
> +extern kprobes_init_stage_t kprobes_init_stage;
> +
> +static inline bool kprobes_is_early(void)
> +{
> + return (kprobes_init_stage < KP_STG_NORMAL);
> +}
>
> #else /* CONFIG_KPROBES */
> typedef int kprobe_opcode_t;
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 1ec8e6e..f4d9fca 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -67,17 +67,14 @@
> addr = ((kprobe_opcode_t *)(kallsyms_lookup_name(name)))
> #endif
>
> -static int kprobes_initialized;
> -static int kprobes_blacklist_initialized;
> -#if defined(CONFIG_KPROBES_ON_FTRACE) && defined(CONFIG_EARLY_KPROBES)
> -static bool kprobes_on_ftrace_initialized __read_mostly = false;
> -#else
> -# define kprobes_on_ftrace_initialized false
> -#endif
> +kprobes_init_stage_t kprobes_init_stage __read_mostly = KP_STG_NONE;
> +#define kprobes_initialized (kprobes_init_stage >= KP_STG_AVAIL)
>
> -bool kprobes_is_early(void)
> +static void kprobes_update_init_stage(kprobes_init_stage_t s)
> {
> - return !(kprobes_initialized && kprobes_blacklist_initialized);
> + BUG_ON((kprobes_init_stage == KP_STG_ERR) ||
> + ((s <= kprobes_init_stage) && (s != KP_STG_ERR)));
> + kprobes_init_stage = s;
> }
>
> static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> @@ -1416,7 +1415,7 @@ static bool within_kprobe_blacklist(unsigned long addr)
> return true;
> #endif
>
> - if (!kprobes_blacklist_initialized)
> + if (kprobes_init_stage < KP_STG_FULL)
> return within_kprobe_blacklist_early(addr);
>
> /*
> @@ -1502,7 +1501,7 @@ int __weak arch_check_ftrace_location(struct kprobe *p)
> /* Given address is not on the instruction boundary */
> if ((unsigned long)p->addr != ftrace_addr)
> return -EILSEQ;
> - if (unlikely(!kprobes_on_ftrace_initialized))
> + if (unlikely(kprobes_init_stage < KP_STG_FTRACE_READY))
> p->flags |= KPROBE_FLAG_FTRACE_EARLY;
> else
> p->flags |= KPROBE_FLAG_FTRACE;
> @@ -1569,10 +1568,8 @@ int register_kprobe(struct kprobe *p)
> struct module *probed_mod;
> kprobe_opcode_t *addr;
>
> -#ifndef CONFIG_EARLY_KPROBES
> - if (kprobes_is_early())
> - return -EAGAIN;
> -#endif
> + if (kprobes_init_stage < KP_STG_AVAIL)
> + return -ENOSYS;
>
> /* Adjust probe address from symbol */
> addr = kprobe_addr(p);
> @@ -2271,7 +2286,8 @@ void init_kprobes_early(void)
> if (!err)
> err = register_module_notifier(&kprobe_module_nb);
>
> - kprobes_initialized = (err == 0);
> + kprobes_update_init_stage(err == 0 ? KP_STG_AVAIL : KP_STG_ERR);
> +
> #ifdef CONFIG_EARLY_KPROBES
> if (!err)
> init_test_probes();
> @@ -2301,9 +2317,7 @@ static int __init init_kprobes(void)
> pr_err("kprobes: failed to populate blacklist: %d\n", err);
> pr_err("Please take care of using kprobes.\n");
> }
> - kprobes_blacklist_initialized = (err == 0);
> -
> - err = kprobes_is_early() ? -ENOSYS : 0;
> + kprobes_update_init_stage(err == 0 ? KP_STG_FULL : KP_STG_ERR);
>
> /* TODO: deal with early kprobes. */
>
> @@ -2607,7 +2621,7 @@ bool kprobe_fix_ftrace_make_nop(struct dyn_ftrace *rec)
> int optimized;
> void *addr;
>
> - if (kprobes_on_ftrace_initialized)
> + if (kprobes_init_stage >= KP_STG_FTRACE_READY)
> return false;
>
> addr = (void *)rec->ip;
> @@ -2731,15 +2745,20 @@ static void convert_early_kprobes_on_ftrace(void)
> }
> }
> }
> - mutex_unlock(&kprobe_mutex);
> }
> +#else
> +static inline void convert_early_kprobes_on_ftrace(void)
> +{
> +}
> +#endif // CONFIG_KPROBES_ON_FTRACE && CONFIG_EARLY_KPROBES
>
> void init_kprobes_on_ftrace(void)
> {
> - kprobes_on_ftrace_initialized = true;
> + mutex_lock(&kprobe_mutex);
> + kprobes_update_init_stage(KP_STG_FTRACE_READY);
> convert_early_kprobes_on_ftrace();
> + mutex_unlock(&kprobe_mutex);
> }
> -#endif
>
> #ifdef CONFIG_EARLY_KPROBES
> static int early_kprobe_pre_handler(struct kprobe *p, struct pt_regs *regs)
>
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt at hitachi.com
More information about the linux-arm-kernel
mailing list