Message-ID: <54F72438.3010405@hitachi.com>
Date:	Thu, 05 Mar 2015 00:26:48 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Wang Nan <wangnan0@...wei.com>
CC:	pmladek@...e.cz, lizefan@...wei.com, linux-kernel@...r.kernel.org,
	x86@...nel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: Re: [PATCH 3/3] early kprobes: x86: don't try to recover ftraced
 instruction before ftrace get ready.

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@...achi.com


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

Powered by blists - more mailing lists