[PATCH] kernel/kprobes.c: move cleanup_rp_inst() to where CONFIG_KRETPROBES enabled

[PATCH] kernel/kprobes.c: move cleanup_rp_inst() to where CONFIG_KRETPROBES enabled

[Chen Gang]

When CONFIG_KRETPROBES disabled, cleanup_rp_inst() is useless too. It
is only called by unregister_kretprobes() which is in CONFIG_KRETPROBES
enabled area.

The related warning (allmodconfig under avr32):

  kernel/kprobes.c:1181: warning: 'cleanup_rp_inst' defined but not used


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 kernel/kprobes.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ceeadfc..18a520d 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1178,26 +1178,6 @@ static inline void free_rp_inst(struct kretprobe *rp)
 	}
 }
 
-static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
-{
-	unsigned long flags, hash;
-	struct kretprobe_instance *ri;
-	struct hlist_node *next;
-	struct hlist_head *head;
-
-	/* No race here */
-	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
-		kretprobe_table_lock(hash, &flags);
-		head = &kretprobe_inst_table[hash];
-		hlist_for_each_entry_safe(ri, next, head, hlist) {
-			if (ri->rp == rp)
-				ri->rp = NULL;
-		}
-		kretprobe_table_unlock(hash, &flags);
-	}
-	free_rp_inst(rp);
-}
-
 /*
 * Add the new probe to ap->list. Fail if this is the
 * second jprobe at the address - two jprobes can't coexist
@@ -1885,6 +1865,26 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
 }
 EXPORT_SYMBOL_GPL(unregister_kretprobe);
 
+static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
+{
+	unsigned long flags, hash;
+	struct kretprobe_instance *ri;
+	struct hlist_node *next;
+	struct hlist_head *head;
+
+	/* No race here */
+	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
+		kretprobe_table_lock(hash, &flags);
+		head = &kretprobe_inst_table[hash];
+		hlist_for_each_entry_safe(ri, next, head, hlist) {
+			if (ri->rp == rp)
+				ri->rp = NULL;
+		}
+		kretprobe_table_unlock(hash, &flags);
+	}
+	free_rp_inst(rp);
+}
+
 void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
 {
 	int i;
-- 
1.7.11.7

Re: [PATCH] kernel/kprobes.c: move cleanup_rp_inst() to where CONFIG_KRETPROBES enabled

[Masami Hiramatsu]

(2014/02/01 21:17), Chen Gang wrote:
> When CONFIG_KRETPROBES disabled, cleanup_rp_inst() is useless too. It
> is only called by unregister_kretprobes() which is in CONFIG_KRETPROBES
> enabled area.
> 
> The related warning (allmodconfig under avr32):
> 
>   kernel/kprobes.c:1181: warning: 'cleanup_rp_inst' defined but not used

This patch itself looks good to me.
And it seems that not only the cleanup_rp_inst, but also other
kretprobe related functions should be moved (free_rp_inst,etc)

Thank you,

> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  kernel/kprobes.c | 40 ++++++++++++++++++++--------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ceeadfc..18a520d 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1178,26 +1178,6 @@ static inline void free_rp_inst(struct kretprobe *rp)
>  	}
>  }
>  
> -static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
> -{
> -	unsigned long flags, hash;
> -	struct kretprobe_instance *ri;
> -	struct hlist_node *next;
> -	struct hlist_head *head;
> -
> -	/* No race here */
> -	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
> -		kretprobe_table_lock(hash, &flags);
> -		head = &kretprobe_inst_table[hash];
> -		hlist_for_each_entry_safe(ri, next, head, hlist) {
> -			if (ri->rp == rp)
> -				ri->rp = NULL;
> -		}
> -		kretprobe_table_unlock(hash, &flags);
> -	}
> -	free_rp_inst(rp);
> -}
> -
>  /*
>  * Add the new probe to ap->list. Fail if this is the
>  * second jprobe at the address - two jprobes can't coexist
> @@ -1885,6 +1865,26 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
>  }
>  EXPORT_SYMBOL_GPL(unregister_kretprobe);
>  
> +static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
> +{
> +	unsigned long flags, hash;
> +	struct kretprobe_instance *ri;
> +	struct hlist_node *next;
> +	struct hlist_head *head;
> +
> +	/* No race here */
> +	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
> +		kretprobe_table_lock(hash, &flags);
> +		head = &kretprobe_inst_table[hash];
> +		hlist_for_each_entry_safe(ri, next, head, hlist) {
> +			if (ri->rp == rp)
> +				ri->rp = NULL;
> +		}
> +		kretprobe_table_unlock(hash, &flags);
> +	}
> +	free_rp_inst(rp);
> +}
> +
>  void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
>  {
>  	int i;
> 


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

Re: [PATCH] kernel/kprobes.c: move cleanup_rp_inst() to where CONFIG_KRETPROBES enabled

[Chen Gang]

On 02/02/2014 10:40 AM, Masami Hiramatsu wrote:
> (2014/02/01 21:17), Chen Gang wrote:
>> When CONFIG_KRETPROBES disabled, cleanup_rp_inst() is useless too. It
>> is only called by unregister_kretprobes() which is in CONFIG_KRETPROBES
>> enabled area.
>>
>> The related warning (allmodconfig under avr32):
>>
>>   kernel/kprobes.c:1181: warning: 'cleanup_rp_inst' defined but not used
> 
> This patch itself looks good to me.
> And it seems that not only the cleanup_rp_inst, but also other
> kretprobe related functions should be moved (free_rp_inst,etc)
> 

OK, thanks, need/should I check them again and send patch v2 for them?


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] kernel/kprobes.c: move cleanup_rp_inst() to where CONFIG_KRETPROBES enabled

[Masami Hiramatsu]

(2014/02/03 20:48), Chen Gang wrote:
> On 02/02/2014 10:40 AM, Masami Hiramatsu wrote:
>> (2014/02/01 21:17), Chen Gang wrote:
>>> When CONFIG_KRETPROBES disabled, cleanup_rp_inst() is useless too. It
>>> is only called by unregister_kretprobes() which is in CONFIG_KRETPROBES
>>> enabled area.
>>>
>>> The related warning (allmodconfig under avr32):
>>>
>>>   kernel/kprobes.c:1181: warning: 'cleanup_rp_inst' defined but not used
>>
>> This patch itself looks good to me.
>> And it seems that not only the cleanup_rp_inst, but also other
>> kretprobe related functions should be moved (free_rp_inst,etc)
>>
> 
> OK, thanks, need/should I check them again and send patch v2 for them?

Yes, I'm happy to review it :)

Thank you!

> 
> 
> Thanks.
> 


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

Re: [PATCH] kernel/kprobes.c: move cleanup_rp_inst() to where CONFIG_KRETPROBES enabled

[Chen Gang]

On 02/03/2014 11:42 PM, Masami Hiramatsu wrote:
> (2014/02/03 20:48), Chen Gang wrote:
>> On 02/02/2014 10:40 AM, Masami Hiramatsu wrote:
>>> (2014/02/01 21:17), Chen Gang wrote:
>>>> When CONFIG_KRETPROBES disabled, cleanup_rp_inst() is useless too. It
>>>> is only called by unregister_kretprobes() which is in CONFIG_KRETPROBES
>>>> enabled area.
>>>>
>>>> The related warning (allmodconfig under avr32):
>>>>
>>>>   kernel/kprobes.c:1181: warning: 'cleanup_rp_inst' defined but not used
>>>
>>> This patch itself looks good to me.
>>> And it seems that not only the cleanup_rp_inst, but also other
>>> kretprobe related functions should be moved (free_rp_inst,etc)
>>>
>>
>> OK, thanks, need/should I check them again and send patch v2 for them?
> 
> Yes, I'm happy to review it :)
> 
> Thank you!
> 

I will/should finish in these days (within 2014-02-07).

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

[PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
are useless, so need move them to CONFIG_KPROBES enabled area.

Now, *kretprobe* generic implementation are all implemented in 2 files:

 - in "include/linux/kprobes.h":

     move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
     move some *kprobe() declarations which kretprobe*() call, to front.
     not touch kretprobe_blacklist[] which is architecture's variable.

 - in "kernel/kprobes.c":

     move all kretprobe* to CONFIG_KPROBES area and dummy outside.
     define kretprobe_flush_task() to let kprobe_flush_task() call.
     define init_kretprobes() to let init_kprobes() call.

The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
in.o") under avr32 and x86_64 allmodconfig, and passes building (get
bzImage and Modpost modules) under x86_64 defconfig.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 include/linux/kprobes.h |  58 +++++----
 kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
 2 files changed, 222 insertions(+), 164 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 925eaf2..c0d1212 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
 	return 1;
 }
 
+int disable_kprobe(struct kprobe *kp);
+int enable_kprobe(struct kprobe *kp);
+
+void dump_kprobe(struct kprobe *kp);
+
+extern struct kretprobe_blackpoint kretprobe_blacklist[];
+
 #ifdef CONFIG_KRETPROBES
 extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				   struct pt_regs *regs);
 extern int arch_trampoline_kprobe(struct kprobe *p);
+static inline void kretprobe_assert(struct kretprobe_instance *ri,
+	unsigned long orig_ret_address, unsigned long trampoline_address)
+{
+	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
+		printk(KERN_ERR
+			"kretprobe BUG!: Processing kretprobe %p @ %p\n",
+			ri->rp, ri->rp->kp.addr);
+		BUG();
+	}
+}
+static inline int disable_kretprobe(struct kretprobe *rp)
+{
+	return disable_kprobe(&rp->kp);
+}
+static inline int enable_kretprobe(struct kretprobe *rp)
+{
+	return enable_kprobe(&rp->kp);
+}
+
 #else /* CONFIG_KRETPROBES */
 static inline void arch_prepare_kretprobe(struct kretprobe *rp,
 					struct pt_regs *regs)
@@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
 {
 	return 0;
 }
-#endif /* CONFIG_KRETPROBES */
-
-extern struct kretprobe_blackpoint kretprobe_blacklist[];
-
 static inline void kretprobe_assert(struct kretprobe_instance *ri,
 	unsigned long orig_ret_address, unsigned long trampoline_address)
 {
-	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
-		printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
-				ri->rp, ri->rp->kp.addr);
-		BUG();
-	}
 }
+static inline int disable_kretprobe(struct kretprobe *rp)
+{
+	return 0;
+}
+static inline int enable_kretprobe(struct kretprobe *rp)
+{
+	return 0;
+}
+
+#endif /* CONFIG_KRETPROBES */
 
 #ifdef CONFIG_KPROBES_SANITY_TEST
 extern int init_test_probes(void);
@@ -379,11 +406,6 @@ void unregister_kretprobes(struct kretprobe **rps, int num);
 void kprobe_flush_task(struct task_struct *tk);
 void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
 
-int disable_kprobe(struct kprobe *kp);
-int enable_kprobe(struct kprobe *kp);
-
-void dump_kprobe(struct kprobe *kp);
-
 #else /* !CONFIG_KPROBES: */
 
 static inline int kprobes_built_in(void)
@@ -459,14 +481,6 @@ static inline int enable_kprobe(struct kprobe *kp)
 	return -ENOSYS;
 }
 #endif /* CONFIG_KPROBES */
-static inline int disable_kretprobe(struct kretprobe *rp)
-{
-	return disable_kprobe(&rp->kp);
-}
-static inline int enable_kretprobe(struct kretprobe *rp)
-{
-	return enable_kprobe(&rp->kp);
-}
 static inline int disable_jprobe(struct jprobe *jp)
 {
 	return disable_kprobe(&jp->kp);
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ceeadfc..e305a81 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -69,7 +69,6 @@
 
 static int kprobes_initialized;
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
-static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
 /* NOTE: change this value only with kprobe_mutex held */
 static bool kprobes_all_disarmed;
@@ -77,14 +76,6 @@ static bool kprobes_all_disarmed;
 /* This protects kprobe_table and optimizing_list */
 static DEFINE_MUTEX(kprobe_mutex);
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
-static struct {
-	raw_spinlock_t lock ____cacheline_aligned_in_smp;
-} kretprobe_table_locks[KPROBE_TABLE_SIZE];
-
-static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
-{
-	return &(kretprobe_table_locks[hash].lock);
-}
 
 /*
  * Normally, functions that we'd want to prohibit kprobes in, are marked
@@ -1079,125 +1070,6 @@ void __kprobes kprobes_inc_nmissed_count(struct kprobe *p)
 	return;
 }
 
-void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
-				struct hlist_head *head)
-{
-	struct kretprobe *rp = ri->rp;
-
-	/* remove rp inst off the rprobe_inst_table */
-	hlist_del(&ri->hlist);
-	INIT_HLIST_NODE(&ri->hlist);
-	if (likely(rp)) {
-		raw_spin_lock(&rp->lock);
-		hlist_add_head(&ri->hlist, &rp->free_instances);
-		raw_spin_unlock(&rp->lock);
-	} else
-		/* Unregistering */
-		hlist_add_head(&ri->hlist, head);
-}
-
-void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
-			 struct hlist_head **head, unsigned long *flags)
-__acquires(hlist_lock)
-{
-	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
-	raw_spinlock_t *hlist_lock;
-
-	*head = &kretprobe_inst_table[hash];
-	hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_lock_irqsave(hlist_lock, *flags);
-}
-
-static void __kprobes kretprobe_table_lock(unsigned long hash,
-	unsigned long *flags)
-__acquires(hlist_lock)
-{
-	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_lock_irqsave(hlist_lock, *flags);
-}
-
-void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
-	unsigned long *flags)
-__releases(hlist_lock)
-{
-	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
-	raw_spinlock_t *hlist_lock;
-
-	hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_unlock_irqrestore(hlist_lock, *flags);
-}
-
-static void __kprobes kretprobe_table_unlock(unsigned long hash,
-       unsigned long *flags)
-__releases(hlist_lock)
-{
-	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_unlock_irqrestore(hlist_lock, *flags);
-}
-
-/*
- * This function is called from finish_task_switch when task tk becomes dead,
- * so that we can recycle any function-return probe instances associated
- * with this task. These left over instances represent probed functions
- * that have been called but will never return.
- */
-void __kprobes kprobe_flush_task(struct task_struct *tk)
-{
-	struct kretprobe_instance *ri;
-	struct hlist_head *head, empty_rp;
-	struct hlist_node *tmp;
-	unsigned long hash, flags = 0;
-
-	if (unlikely(!kprobes_initialized))
-		/* Early boot.  kretprobe_table_locks not yet initialized. */
-		return;
-
-	INIT_HLIST_HEAD(&empty_rp);
-	hash = hash_ptr(tk, KPROBE_HASH_BITS);
-	head = &kretprobe_inst_table[hash];
-	kretprobe_table_lock(hash, &flags);
-	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
-		if (ri->task == tk)
-			recycle_rp_inst(ri, &empty_rp);
-	}
-	kretprobe_table_unlock(hash, &flags);
-	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
-		hlist_del(&ri->hlist);
-		kfree(ri);
-	}
-}
-
-static inline void free_rp_inst(struct kretprobe *rp)
-{
-	struct kretprobe_instance *ri;
-	struct hlist_node *next;
-
-	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
-		hlist_del(&ri->hlist);
-		kfree(ri);
-	}
-}
-
-static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
-{
-	unsigned long flags, hash;
-	struct kretprobe_instance *ri;
-	struct hlist_node *next;
-	struct hlist_head *head;
-
-	/* No race here */
-	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
-		kretprobe_table_lock(hash, &flags);
-		head = &kretprobe_inst_table[hash];
-		hlist_for_each_entry_safe(ri, next, head, hlist) {
-			if (ri->rp == rp)
-				ri->rp = NULL;
-		}
-		kretprobe_table_unlock(hash, &flags);
-	}
-	free_rp_inst(rp);
-}
-
 /*
 * Add the new probe to ap->list. Fail if this is the
 * second jprobe at the address - two jprobes can't coexist
@@ -1764,6 +1636,55 @@ void __kprobes unregister_jprobes(struct jprobe **jps, int num)
 EXPORT_SYMBOL_GPL(unregister_jprobes);
 
 #ifdef CONFIG_KRETPROBES
+static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
+static struct {
+	raw_spinlock_t lock ____cacheline_aligned_in_smp;
+} kretprobe_table_locks[KPROBE_TABLE_SIZE];
+
+static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
+{
+	return &(kretprobe_table_locks[hash].lock);
+}
+
+void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
+			 struct hlist_head **head, unsigned long *flags)
+__acquires(hlist_lock)
+{
+	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
+	raw_spinlock_t *hlist_lock;
+
+	*head = &kretprobe_inst_table[hash];
+	hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_lock_irqsave(hlist_lock, *flags);
+}
+
+void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
+	unsigned long *flags)
+__releases(hlist_lock)
+{
+	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
+	raw_spinlock_t *hlist_lock;
+
+	hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_unlock_irqrestore(hlist_lock, *flags);
+}
+
+static void __kprobes kretprobe_table_lock(unsigned long hash,
+	unsigned long *flags)
+__acquires(hlist_lock)
+{
+	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_lock_irqsave(hlist_lock, *flags);
+}
+
+static void __kprobes kretprobe_table_unlock(unsigned long hash,
+	unsigned long *flags)
+__releases(hlist_lock)
+{
+	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_unlock_irqrestore(hlist_lock, *flags);
+}
+
 /*
  * This kprobe pre_handler is registered with every kretprobe. When probe
  * hits it will set up the return probe.
@@ -1808,6 +1729,17 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
 	return 0;
 }
 
+static inline void free_rp_inst(struct kretprobe *rp)
+{
+	struct kretprobe_instance *ri;
+	struct hlist_node *next;
+
+	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
+}
+
 int __kprobes register_kretprobe(struct kretprobe *rp)
 {
 	int ret = 0;
@@ -1885,6 +1817,26 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
 }
 EXPORT_SYMBOL_GPL(unregister_kretprobe);
 
+static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
+{
+	unsigned long flags, hash;
+	struct kretprobe_instance *ri;
+	struct hlist_node *next;
+	struct hlist_head *head;
+
+	/* No race here */
+	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
+		kretprobe_table_lock(hash, &flags);
+		head = &kretprobe_inst_table[hash];
+		hlist_for_each_entry_safe(ri, next, head, hlist) {
+			if (ri->rp == rp)
+				ri->rp = NULL;
+		}
+		kretprobe_table_unlock(hash, &flags);
+	}
+	free_rp_inst(rp);
+}
+
 void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
 {
 	int i;
@@ -1907,6 +1859,73 @@ void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
 }
 EXPORT_SYMBOL_GPL(unregister_kretprobes);
 
+void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
+				struct hlist_head *head)
+{
+	struct kretprobe *rp = ri->rp;
+
+	/* remove rp inst off the rprobe_inst_table */
+	hlist_del(&ri->hlist);
+	INIT_HLIST_NODE(&ri->hlist);
+	if (likely(rp)) {
+		raw_spin_lock(&rp->lock);
+		hlist_add_head(&ri->hlist, &rp->free_instances);
+		raw_spin_unlock(&rp->lock);
+	} else
+		/* Unregistering */
+		hlist_add_head(&ri->hlist, head);
+}
+
+static void __kprobes kretprobe_flush_task(struct task_struct *tk)
+{
+	struct kretprobe_instance *ri;
+	struct hlist_head *head, empty_rp;
+	struct hlist_node *tmp;
+	unsigned long hash, flags = 0;
+
+	if (unlikely(!kprobes_initialized))
+		/* Early boot.  kretprobe_table_locks not yet initialized. */
+		return;
+
+	INIT_HLIST_HEAD(&empty_rp);
+	hash = hash_ptr(tk, KPROBE_HASH_BITS);
+	head = &kretprobe_inst_table[hash];
+	kretprobe_table_lock(hash, &flags);
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task == tk)
+			recycle_rp_inst(ri, &empty_rp);
+	}
+	kretprobe_table_unlock(hash, &flags);
+	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
+}
+
+static void __init init_kretprobes(void)
+{
+	int i;
+
+	/* FIXME allocate the probe table, currently defined statically */
+	/* initialize all list heads */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
+		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
+	}
+
+	if (kretprobe_blacklist_size) {
+		/* lookup the function address from its name */
+		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+			kprobe_lookup_name(kretprobe_blacklist[i].name,
+					   kretprobe_blacklist[i].addr);
+			if (!kretprobe_blacklist[i].addr)
+				printk(KERN_WARNING
+					"kretprobe: lookup failed: %s\n",
+					kretprobe_blacklist[i].name);
+		}
+	}
+}
+
 #else /* CONFIG_KRETPROBES */
 int __kprobes register_kretprobe(struct kretprobe *rp)
 {
@@ -1936,8 +1955,44 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
 	return 0;
 }
 
+void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
+				struct hlist_head *head)
+{
+}
+
+void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
+			 struct hlist_head **head, unsigned long *flags)
+__acquires(hlist_lock)
+{
+}
+
+void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
+	unsigned long *flags)
+__releases(hlist_lock)
+{
+}
+
+static void __kprobes kretprobe_flush_task(struct task_struct *tk)
+{
+}
+
+static void __init init_kretprobes(void)
+{
+}
+
 #endif /* CONFIG_KRETPROBES */
 
+/*
+ * This function is called from finish_task_switch when task tk becomes dead,
+ * so that we can recycle any function-return probe instances associated
+ * with this task. These left over instances represent probed functions
+ * that have been called but will never return.
+ */
+void __kprobes kprobe_flush_task(struct task_struct *tk)
+{
+	kretprobe_flush_task(tk);
+}
+
 /* Set the kprobe gone and remove its instruction buffer. */
 static void __kprobes kill_kprobe(struct kprobe *p)
 {
@@ -2073,11 +2128,8 @@ static int __init init_kprobes(void)
 
 	/* FIXME allocate the probe table, currently defined statically */
 	/* initialize all list heads */
-	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++)
 		INIT_HLIST_HEAD(&kprobe_table[i]);
-		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
-		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
-	}
 
 	/*
 	 * Lookup and populate the kprobe_blacklist.
@@ -2101,16 +2153,8 @@ static int __init init_kprobes(void)
 			kb->range = size;
 	}
 
-	if (kretprobe_blacklist_size) {
-		/* lookup the function address from its name */
-		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
-			kprobe_lookup_name(kretprobe_blacklist[i].name,
-					   kretprobe_blacklist[i].addr);
-			if (!kretprobe_blacklist[i].addr)
-				printk("kretprobe: lookup failed: %s\n",
-				       kretprobe_blacklist[i].name);
-		}
-	}
+	/* Initialize kretprobes */
+	init_kretprobes();
 
 #if defined(CONFIG_OPTPROBES)
 #if defined(__ARCH_WANT_KPROBES_INSN_SLOT)
-- 
1.7.11.7

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Masami Hiramatsu]

(2014/02/04 14:16), Chen Gang wrote:
> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
> are useless, so need move them to CONFIG_KPROBES enabled area.
> 
> Now, *kretprobe* generic implementation are all implemented in 2 files:
> 
>  - in "include/linux/kprobes.h":
> 
>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>      move some *kprobe() declarations which kretprobe*() call, to front.
>      not touch kretprobe_blacklist[] which is architecture's variable.
> 
>  - in "kernel/kprobes.c":
> 
>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>      define init_kretprobes() to let init_kprobes() call.
> 
> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
> bzImage and Modpost modules) under x86_64 defconfig.

Thanks for the fix! and I have some comments below.

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  include/linux/kprobes.h |  58 +++++----
>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>  2 files changed, 222 insertions(+), 164 deletions(-)
> 
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 925eaf2..c0d1212 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>  	return 1;
>  }
>  
> +int disable_kprobe(struct kprobe *kp);
> +int enable_kprobe(struct kprobe *kp);
> +
> +void dump_kprobe(struct kprobe *kp);
> +
> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
> +
>  #ifdef CONFIG_KRETPROBES
>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  				   struct pt_regs *regs);
>  extern int arch_trampoline_kprobe(struct kprobe *p);
> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
> +	unsigned long orig_ret_address, unsigned long trampoline_address)
> +{
> +	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
> +		printk(KERN_ERR
> +			"kretprobe BUG!: Processing kretprobe %p @ %p\n",
> +			ri->rp, ri->rp->kp.addr);
> +		BUG();
> +	}
> +}
> +static inline int disable_kretprobe(struct kretprobe *rp)
> +{
> +	return disable_kprobe(&rp->kp);
> +}
> +static inline int enable_kretprobe(struct kretprobe *rp)
> +{
> +	return enable_kprobe(&rp->kp);
> +}
> +
>  #else /* CONFIG_KRETPROBES */
>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>  					struct pt_regs *regs)
> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>  {
>  	return 0;
>  }
> -#endif /* CONFIG_KRETPROBES */
> -
> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
> -
>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>  	unsigned long orig_ret_address, unsigned long trampoline_address)
>  {
> -	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
> -		printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
> -				ri->rp, ri->rp->kp.addr);
> -		BUG();
> -	}
>  }
> +static inline int disable_kretprobe(struct kretprobe *rp)
> +{
> +	return 0;
> +}
> +static inline int enable_kretprobe(struct kretprobe *rp)
> +{
> +	return 0;
> +}

No, these should returns -EINVAL or -ENOSYS, since these are user API.
Anyway, I don't think those inlined functions to be changed, because
most of them are internal functions. If CONFIG_KRETPROBES=n, it just
be ignored.

So, I think you don't need to change kprobes.h.


> +
> +#endif /* CONFIG_KRETPROBES */
>  
>  #ifdef CONFIG_KPROBES_SANITY_TEST
>  extern int init_test_probes(void);
> @@ -379,11 +406,6 @@ void unregister_kretprobes(struct kretprobe **rps, int num);
>  void kprobe_flush_task(struct task_struct *tk);
>  void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>  
> -int disable_kprobe(struct kprobe *kp);
> -int enable_kprobe(struct kprobe *kp);
> -
> -void dump_kprobe(struct kprobe *kp);
> -
>  #else /* !CONFIG_KPROBES: */
>  
>  static inline int kprobes_built_in(void)
> @@ -459,14 +481,6 @@ static inline int enable_kprobe(struct kprobe *kp)
>  	return -ENOSYS;
>  }
>  #endif /* CONFIG_KPROBES */
> -static inline int disable_kretprobe(struct kretprobe *rp)
> -{
> -	return disable_kprobe(&rp->kp);
> -}
> -static inline int enable_kretprobe(struct kretprobe *rp)
> -{
> -	return enable_kprobe(&rp->kp);
> -}
>  static inline int disable_jprobe(struct jprobe *jp)
>  {
>  	return disable_kprobe(&jp->kp);
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ceeadfc..e305a81 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
[...]
> @@ -1936,8 +1955,44 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>  	return 0;
>  }
>  
> +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
> +				struct hlist_head *head)
> +{
> +}
> +
> +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
> +			 struct hlist_head **head, unsigned long *flags)
> +__acquires(hlist_lock)
> +{
> +}
> +
> +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
> +	unsigned long *flags)
> +__releases(hlist_lock)
> +{
> +}
> +

> +static void __kprobes kretprobe_flush_task(struct task_struct *tk)
> +{
> +}
> +
> +static void __init init_kretprobes(void)
> +{
> +}

These should be macros, as I did for optprobe functions
with !CONFIG_OPTPROBES.

Other parts looks good to me!;)

Thank you!


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

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
> (2014/02/04 14:16), Chen Gang wrote:
>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>
>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>
>>  - in "include/linux/kprobes.h":
>>
>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>
>>  - in "kernel/kprobes.c":
>>
>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>      define init_kretprobes() to let init_kprobes() call.
>>
>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>> bzImage and Modpost modules) under x86_64 defconfig.
> 
> Thanks for the fix! and I have some comments below.
> 
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  include/linux/kprobes.h |  58 +++++----
>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>
>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>> index 925eaf2..c0d1212 100644
>> --- a/include/linux/kprobes.h
>> +++ b/include/linux/kprobes.h
>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>  	return 1;
>>  }
>>  
>> +int disable_kprobe(struct kprobe *kp);
>> +int enable_kprobe(struct kprobe *kp);
>> +
>> +void dump_kprobe(struct kprobe *kp);
>> +
>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>> +
>>  #ifdef CONFIG_KRETPROBES
>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>  				   struct pt_regs *regs);
>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>> +	unsigned long orig_ret_address, unsigned long trampoline_address)
>> +{
>> +	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>> +		printk(KERN_ERR
>> +			"kretprobe BUG!: Processing kretprobe %p @ %p\n",
>> +			ri->rp, ri->rp->kp.addr);
>> +		BUG();
>> +	}
>> +}
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return disable_kprobe(&rp->kp);
>> +}
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return enable_kprobe(&rp->kp);
>> +}
>> +
>>  #else /* CONFIG_KRETPROBES */
>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>  					struct pt_regs *regs)
>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>  {
>>  	return 0;
>>  }
>> -#endif /* CONFIG_KRETPROBES */
>> -
>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>> -
>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>  	unsigned long orig_ret_address, unsigned long trampoline_address)
>>  {
>> -	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>> -		printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>> -				ri->rp, ri->rp->kp.addr);
>> -		BUG();
>> -	}
>>  }
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return 0;
>> +}
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return 0;
>> +}
> 
> No, these should returns -EINVAL or -ENOSYS, since these are user API.

OK, thanks, it sounds reasonable to me.

> Anyway, I don't think those inlined functions to be changed, because
> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
> be ignored.
> 

In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
disable_kretprobe(), and enable_kretprobe() are not ignored.

> So, I think you don't need to change kprobes.h.
> 

So "kprobes.h" still need be changed.

>> +
>> +#endif /* CONFIG_KRETPROBES */
>>  
>>  #ifdef CONFIG_KPROBES_SANITY_TEST
>>  extern int init_test_probes(void);
>> @@ -379,11 +406,6 @@ void unregister_kretprobes(struct kretprobe **rps, int num);
>>  void kprobe_flush_task(struct task_struct *tk);
>>  void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>>  
>> -int disable_kprobe(struct kprobe *kp);
>> -int enable_kprobe(struct kprobe *kp);
>> -
>> -void dump_kprobe(struct kprobe *kp);
>> -
>>  #else /* !CONFIG_KPROBES: */
>>  
>>  static inline int kprobes_built_in(void)
>> @@ -459,14 +481,6 @@ static inline int enable_kprobe(struct kprobe *kp)
>>  	return -ENOSYS;
>>  }
>>  #endif /* CONFIG_KPROBES */
>> -static inline int disable_kretprobe(struct kretprobe *rp)
>> -{
>> -	return disable_kprobe(&rp->kp);
>> -}
>> -static inline int enable_kretprobe(struct kretprobe *rp)
>> -{
>> -	return enable_kprobe(&rp->kp);
>> -}
>>  static inline int disable_jprobe(struct jprobe *jp)
>>  {
>>  	return disable_kprobe(&jp->kp);
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index ceeadfc..e305a81 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
> [...]
>> @@ -1936,8 +1955,44 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>>  	return 0;
>>  }
>>  
>> +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
>> +				struct hlist_head *head)
>> +{
>> +}
>> +
>> +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
>> +			 struct hlist_head **head, unsigned long *flags)
>> +__acquires(hlist_lock)
>> +{
>> +}
>> +
>> +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
>> +	unsigned long *flags)
>> +__releases(hlist_lock)
>> +{
>> +}
>> +
> 
>> +static void __kprobes kretprobe_flush_task(struct task_struct *tk)
>> +{
>> +}
>> +
>> +static void __init init_kretprobes(void)
>> +{
>> +}
> 
> These should be macros, as I did for optprobe functions
> with !CONFIG_OPTPROBES.
>

OK, thanks, it sounds reasonable to me.

 - For new added static functions: kretprobe_flush_task(), and
   init_kretprobes() need be changed to macros

 - For extern functions: recycle_rp_inst(), kretprobe_hash_lock(), and
   kretprobe_has_unlock(), need use dummy functions.

 - For original static function: pre_handler_kretprobe(), need still
   use dummy function (for function pointer comparing).


> Other parts looks good to me!;)
> 
> Thank you!
> 
> 

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
> (2014/02/04 14:16), Chen Gang wrote:
>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>
>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>
>>  - in "include/linux/kprobes.h":
>>
>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>
>>  - in "kernel/kprobes.c":
>>
>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>      define init_kretprobes() to let init_kprobes() call.
>>
>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>> bzImage and Modpost modules) under x86_64 defconfig.
>
> Thanks for the fix! and I have some comments below.
>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  include/linux/kprobes.h |  58 +++++----
>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>
>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>> index 925eaf2..c0d1212 100644
>> --- a/include/linux/kprobes.h
>> +++ b/include/linux/kprobes.h
>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>   return 1;
>>  }
>>  
>> +int disable_kprobe(struct kprobe *kp);
>> +int enable_kprobe(struct kprobe *kp);
>> +
>> +void dump_kprobe(struct kprobe *kp);
>> +
>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>> +
>>  #ifdef CONFIG_KRETPROBES
>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>     struct pt_regs *regs);
>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>> +{
>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>> + printk(KERN_ERR
>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>> + ri->rp, ri->rp->kp.addr);
>> + BUG();
>> + }
>> +}
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>> +{
>> + return disable_kprobe(&rp->kp);
>> +}
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>> +{
>> + return enable_kprobe(&rp->kp);
>> +}
>> +
>>  #else /* CONFIG_KRETPROBES */
>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>   struct pt_regs *regs)
>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>  {
>>   return 0;
>>  }
>> -#endif /* CONFIG_KRETPROBES */
>> -
>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>> -
>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>  {
>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>> - ri->rp, ri->rp->kp.addr);
>> - BUG();
>> - }
>>  }
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>> +{
>> + return 0;
>> +}
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>> +{
>> + return 0;
>> +}
>
> No, these should returns -EINVAL or -ENOSYS, since these are user API.

OK, thanks, it sounds reasonable to me.

> Anyway, I don't think those inlined functions to be changed, because
> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
> be ignored.
>

In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
disable_kretprobe(), and enable_kretprobe() are not ignored.

> So, I think you don't need to change kprobes.h.
>

So "kprobes.h" still need be changed.

>> +
>> +#endif /* CONFIG_KRETPROBES */
>>  
>>  #ifdef CONFIG_KPROBES_SANITY_TEST
>>  extern int init_test_probes(void);
>> @@ -379,11 +406,6 @@ void unregister_kretprobes(struct kretprobe **rps, int num);
>>  void kprobe_flush_task(struct task_struct *tk);
>>  void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>>  
>> -int disable_kprobe(struct kprobe *kp);
>> -int enable_kprobe(struct kprobe *kp);
>> -
>> -void dump_kprobe(struct kprobe *kp);
>> -
>>  #else /* !CONFIG_KPROBES: */
>>  
>>  static inline int kprobes_built_in(void)
>> @@ -459,14 +481,6 @@ static inline int enable_kprobe(struct kprobe *kp)
>>   return -ENOSYS;
>>  }
>>  #endif /* CONFIG_KPROBES */
>> -static inline int disable_kretprobe(struct kretprobe *rp)
>> -{
>> - return disable_kprobe(&rp->kp);
>> -}
>> -static inline int enable_kretprobe(struct kretprobe *rp)
>> -{
>> - return enable_kprobe(&rp->kp);
>> -}
>>  static inline int disable_jprobe(struct jprobe *jp)
>>  {
>>   return disable_kprobe(&jp->kp);
>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> index ceeadfc..e305a81 100644
>> --- a/kernel/kprobes.c
>> +++ b/kernel/kprobes.c
> [...]
>> @@ -1936,8 +1955,44 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>>   return 0;
>>  }
>>  
>> +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
>> + struct hlist_head *head)
>> +{
>> +}
>> +
>> +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
>> + struct hlist_head **head, unsigned long *flags)
>> +__acquires(hlist_lock)
>> +{
>> +}
>> +
>> +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
>> + unsigned long *flags)
>> +__releases(hlist_lock)
>> +{
>> +}
>> +
>
>> +static void __kprobes kretprobe_flush_task(struct task_struct *tk)
>> +{
>> +}
>> +
>> +static void __init init_kretprobes(void)
>> +{
>> +}
>
> These should be macros, as I did for optprobe functions
> with !CONFIG_OPTPROBES.
>

OK, thanks, it sounds reasonable to me.

 - For new added static functions: kretprobe_flush_task(), and
   init_kretprobes() need be changed to macros

 - For extern functions: recycle_rp_inst(), kretprobe_hash_lock(), and
   kretprobe_has_unlock(), need use dummy functions.

 - For original static function: pre_handler_kretprobe(), need still
   use dummy function (for function pointer comparing).


> Other parts looks good to me!;)
>
> Thank you!
>
>

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Masami Hiramatsu]

(2014/02/04 21:07), Chen Gang wrote:
> On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
>> (2014/02/04 14:16), Chen Gang wrote:
>>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>>
>>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>>
>>>  - in "include/linux/kprobes.h":
>>>
>>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>>
>>>  - in "kernel/kprobes.c":
>>>
>>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>>      define init_kretprobes() to let init_kprobes() call.
>>>
>>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>>> bzImage and Modpost modules) under x86_64 defconfig.
>>
>> Thanks for the fix! and I have some comments below.
>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>> ---
>>>  include/linux/kprobes.h |  58 +++++----
>>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>>
>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>> index 925eaf2..c0d1212 100644
>>> --- a/include/linux/kprobes.h
>>> +++ b/include/linux/kprobes.h
>>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>>   return 1;
>>>  }
>>>  
>>> +int disable_kprobe(struct kprobe *kp);
>>> +int enable_kprobe(struct kprobe *kp);
>>> +
>>> +void dump_kprobe(struct kprobe *kp);
>>> +
>>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>> +
>>>  #ifdef CONFIG_KRETPROBES
>>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>>     struct pt_regs *regs);
>>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>>> +{
>>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>> + printk(KERN_ERR
>>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>> + ri->rp, ri->rp->kp.addr);
>>> + BUG();
>>> + }
>>> +}
>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>> +{
>>> + return disable_kprobe(&rp->kp);
>>> +}
>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>> +{
>>> + return enable_kprobe(&rp->kp);
>>> +}
>>> +
>>>  #else /* CONFIG_KRETPROBES */
>>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>>   struct pt_regs *regs)
>>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>>  {
>>>   return 0;
>>>  }
>>> -#endif /* CONFIG_KRETPROBES */
>>> -
>>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>> -
>>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>>  {
>>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>> - ri->rp, ri->rp->kp.addr);
>>> - BUG();
>>> - }
>>>  }
>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>> +{
>>> + return 0;
>>> +}
>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>> +{
>>> + return 0;
>>> +}
>>
>> No, these should returns -EINVAL or -ENOSYS, since these are user API.
> 
> OK, thanks, it sounds reasonable to me.
> 
>> Anyway, I don't think those inlined functions to be changed, because
>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>> be ignored.
>>
> 
> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
> disable_kretprobe(), and enable_kretprobe() are not ignored.

Really? where are they called? I mean, those functions do not have
any instance unless your module uses it (but that is not what the kernel
itself should help).

> 
>> So, I think you don't need to change kprobes.h.
>>
> 
> So "kprobes.h" still need be changed.

Is there any concrete problem you have?

> 
>>> +
>>> +#endif /* CONFIG_KRETPROBES */
>>>  
>>>  #ifdef CONFIG_KPROBES_SANITY_TEST
>>>  extern int init_test_probes(void);
>>> @@ -379,11 +406,6 @@ void unregister_kretprobes(struct kretprobe **rps, int num);
>>>  void kprobe_flush_task(struct task_struct *tk);
>>>  void recycle_rp_inst(struct kretprobe_instance *ri, struct hlist_head *head);
>>>  
>>> -int disable_kprobe(struct kprobe *kp);
>>> -int enable_kprobe(struct kprobe *kp);
>>> -
>>> -void dump_kprobe(struct kprobe *kp);
>>> -
>>>  #else /* !CONFIG_KPROBES: */
>>>  
>>>  static inline int kprobes_built_in(void)
>>> @@ -459,14 +481,6 @@ static inline int enable_kprobe(struct kprobe *kp)
>>>   return -ENOSYS;
>>>  }
>>>  #endif /* CONFIG_KPROBES */
>>> -static inline int disable_kretprobe(struct kretprobe *rp)
>>> -{
>>> - return disable_kprobe(&rp->kp);
>>> -}
>>> -static inline int enable_kretprobe(struct kretprobe *rp)
>>> -{
>>> - return enable_kprobe(&rp->kp);
>>> -}
>>>  static inline int disable_jprobe(struct jprobe *jp)
>>>  {
>>>   return disable_kprobe(&jp->kp);
>>> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>>> index ceeadfc..e305a81 100644
>>> --- a/kernel/kprobes.c
>>> +++ b/kernel/kprobes.c
>> [...]
>>> @@ -1936,8 +1955,44 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>>>   return 0;
>>>  }
>>>  
>>> +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
>>> + struct hlist_head *head)
>>> +{
>>> +}
>>> +
>>> +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
>>> + struct hlist_head **head, unsigned long *flags)
>>> +__acquires(hlist_lock)
>>> +{
>>> +}
>>> +
>>> +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
>>> + unsigned long *flags)
>>> +__releases(hlist_lock)
>>> +{
>>> +}
>>> +
>>
>>> +static void __kprobes kretprobe_flush_task(struct task_struct *tk)
>>> +{
>>> +}
>>> +
>>> +static void __init init_kretprobes(void)
>>> +{
>>> +}
>>
>> These should be macros, as I did for optprobe functions
>> with !CONFIG_OPTPROBES.
>>
> 
> OK, thanks, it sounds reasonable to me.
> 
>  - For new added static functions: kretprobe_flush_task(), and
>    init_kretprobes() need be changed to macros
> 
>  - For extern functions: recycle_rp_inst(), kretprobe_hash_lock(), and
>    kretprobe_has_unlock(), need use dummy functions.
> 
>  - For original static function: pre_handler_kretprobe(), need still
>    use dummy function (for function pointer comparing).

Right :)

Thanks!

> 
> 
>> Other parts looks good to me!;)
>>
>> Thank you!
>>
>>
> 
> Thanks.
> 


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

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

On 02/04/2014 09:29 PM, Masami Hiramatsu wrote:
> (2014/02/04 21:07), Chen Gang wrote:
>> On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
>>> (2014/02/04 14:16), Chen Gang wrote:
>>>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>>>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>>>
>>>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>>>
>>>>  - in "include/linux/kprobes.h":
>>>>
>>>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>>>
>>>>  - in "kernel/kprobes.c":
>>>>
>>>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>>>      define init_kretprobes() to let init_kprobes() call.
>>>>
>>>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>>>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>>>> bzImage and Modpost modules) under x86_64 defconfig.
>>>
>>> Thanks for the fix! and I have some comments below.
>>>
>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>> ---
>>>>  include/linux/kprobes.h |  58 +++++----
>>>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>>>
>>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>>> index 925eaf2..c0d1212 100644
>>>> --- a/include/linux/kprobes.h
>>>> +++ b/include/linux/kprobes.h
>>>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>>>   return 1;
>>>>  }
>>>>  
>>>> +int disable_kprobe(struct kprobe *kp);
>>>> +int enable_kprobe(struct kprobe *kp);
>>>> +
>>>> +void dump_kprobe(struct kprobe *kp);
>>>> +
>>>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>> +
>>>>  #ifdef CONFIG_KRETPROBES
>>>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>>>     struct pt_regs *regs);
>>>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>>>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>>>> +{
>>>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>> + printk(KERN_ERR
>>>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>> + ri->rp, ri->rp->kp.addr);
>>>> + BUG();
>>>> + }
>>>> +}
>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>> +{
>>>> + return disable_kprobe(&rp->kp);
>>>> +}
>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>> +{
>>>> + return enable_kprobe(&rp->kp);
>>>> +}
>>>> +
>>>>  #else /* CONFIG_KRETPROBES */
>>>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>>>   struct pt_regs *regs)
>>>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>>>  {
>>>>   return 0;
>>>>  }
>>>> -#endif /* CONFIG_KRETPROBES */
>>>> -
>>>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>> -
>>>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>  {
>>>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>> - ri->rp, ri->rp->kp.addr);
>>>> - BUG();
>>>> - }
>>>>  }
>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>> +{
>>>> + return 0;
>>>> +}
>>>
>>> No, these should returns -EINVAL or -ENOSYS, since these are user API.
>>
>> OK, thanks, it sounds reasonable to me.
>>
>>> Anyway, I don't think those inlined functions to be changed, because
>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>> be ignored.
>>>
>>
>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>> disable_kretprobe(), and enable_kretprobe() are not ignored.
> 
> Really? where are they called? I mean, those functions do not have
> any instance unless your module uses it (but that is not what the kernel
> itself should help).
> 

If what you said correct (I guess so), for me, we still need let them in
CONFIG_KRETPROBES area, and without any dummy outside, just like another
*kprobe* static inline functions have done in "include/linux/kprobes.h".


>>
>>> So, I think you don't need to change kprobes.h.
>>>
>>
>> So "kprobes.h" still need be changed.
> 
> Is there any concrete problem you have?
> 

No, I just read the code, no additional really issues.


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Masami Hiramatsu]

(2014/02/04 22:53), Chen Gang wrote:
> On 02/04/2014 09:29 PM, Masami Hiramatsu wrote:
>> (2014/02/04 21:07), Chen Gang wrote:
>>> On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
>>>> (2014/02/04 14:16), Chen Gang wrote:
>>>>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>>>>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>>>>
>>>>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>>>>
>>>>>  - in "include/linux/kprobes.h":
>>>>>
>>>>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>>>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>>>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>>>>
>>>>>  - in "kernel/kprobes.c":
>>>>>
>>>>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>>>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>>>>      define init_kretprobes() to let init_kprobes() call.
>>>>>
>>>>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>>>>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>>>>> bzImage and Modpost modules) under x86_64 defconfig.
>>>>
>>>> Thanks for the fix! and I have some comments below.
>>>>
>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>> ---
>>>>>  include/linux/kprobes.h |  58 +++++----
>>>>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>>>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>>>> index 925eaf2..c0d1212 100644
>>>>> --- a/include/linux/kprobes.h
>>>>> +++ b/include/linux/kprobes.h
>>>>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>>>>   return 1;
>>>>>  }
>>>>>  
>>>>> +int disable_kprobe(struct kprobe *kp);
>>>>> +int enable_kprobe(struct kprobe *kp);
>>>>> +
>>>>> +void dump_kprobe(struct kprobe *kp);
>>>>> +
>>>>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>> +
>>>>>  #ifdef CONFIG_KRETPROBES
>>>>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>>>>     struct pt_regs *regs);
>>>>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>>>>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>> +{
>>>>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>> + printk(KERN_ERR
>>>>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>> + ri->rp, ri->rp->kp.addr);
>>>>> + BUG();
>>>>> + }
>>>>> +}
>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>> +{
>>>>> + return disable_kprobe(&rp->kp);
>>>>> +}
>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>> +{
>>>>> + return enable_kprobe(&rp->kp);
>>>>> +}
>>>>> +
>>>>>  #else /* CONFIG_KRETPROBES */
>>>>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>>>>   struct pt_regs *regs)
>>>>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>>>>  {
>>>>>   return 0;
>>>>>  }
>>>>> -#endif /* CONFIG_KRETPROBES */
>>>>> -
>>>>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>> -
>>>>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>  {
>>>>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>> - ri->rp, ri->rp->kp.addr);
>>>>> - BUG();
>>>>> - }
>>>>>  }
>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>> +{
>>>>> + return 0;
>>>>> +}
>>>>
>>>> No, these should returns -EINVAL or -ENOSYS, since these are user API.
>>>
>>> OK, thanks, it sounds reasonable to me.
>>>
>>>> Anyway, I don't think those inlined functions to be changed, because
>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>> be ignored.
>>>>
>>>
>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>
>> Really? where are they called? I mean, those functions do not have
>> any instance unless your module uses it (but that is not what the kernel
>> itself should help).
>>
> 
> If what you said correct (I guess so), for me, we still need let them in
> CONFIG_KRETPROBES area, and without any dummy outside, just like another
> *kprobe* static inline functions have done in "include/linux/kprobes.h".

kretprobe_assert() is only for the internal check. So we don't need to care
about, and disable/enable_kretprobe() are anyway returns -EINVAL because
kretprobe can not be registered. And all of them are inlined functions.
In that case, we don't need to care about it.
I just concerned that it is a waste of memory if there are useless kretprobe
related instances are built when CONFIG_KRETPROBES=n.

Thank you,

>>>
>>>> So, I think you don't need to change kprobes.h.
>>>>
>>>
>>> So "kprobes.h" still need be changed.
>>
>> Is there any concrete problem you have?
>>
> 
> No, I just read the code, no additional really issues.
> 
> 
> Thanks.
> 


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

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

On 02/04/2014 11:39 PM, Masami Hiramatsu wrote:
> (2014/02/04 22:53), Chen Gang wrote:
>> On 02/04/2014 09:29 PM, Masami Hiramatsu wrote:
>>> (2014/02/04 21:07), Chen Gang wrote:
>>>> On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
>>>>> (2014/02/04 14:16), Chen Gang wrote:
>>>>>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>>>>>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>>>>>
>>>>>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>>>>>
>>>>>>  - in "include/linux/kprobes.h":
>>>>>>
>>>>>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>>>>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>>>>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>>>>>
>>>>>>  - in "kernel/kprobes.c":
>>>>>>
>>>>>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>>>>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>>>>>      define init_kretprobes() to let init_kprobes() call.
>>>>>>
>>>>>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>>>>>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>>>>>> bzImage and Modpost modules) under x86_64 defconfig.
>>>>>
>>>>> Thanks for the fix! and I have some comments below.
>>>>>
>>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>> ---
>>>>>>  include/linux/kprobes.h |  58 +++++----
>>>>>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>>>>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>>>>> index 925eaf2..c0d1212 100644
>>>>>> --- a/include/linux/kprobes.h
>>>>>> +++ b/include/linux/kprobes.h
>>>>>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>>>>>   return 1;
>>>>>>  }
>>>>>>  
>>>>>> +int disable_kprobe(struct kprobe *kp);
>>>>>> +int enable_kprobe(struct kprobe *kp);
>>>>>> +
>>>>>> +void dump_kprobe(struct kprobe *kp);
>>>>>> +
>>>>>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>>> +
>>>>>>  #ifdef CONFIG_KRETPROBES
>>>>>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>>>>>     struct pt_regs *regs);
>>>>>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>>>>>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>> +{
>>>>>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>>> + printk(KERN_ERR
>>>>>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>>> + ri->rp, ri->rp->kp.addr);
>>>>>> + BUG();
>>>>>> + }
>>>>>> +}
>>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>>> +{
>>>>>> + return disable_kprobe(&rp->kp);
>>>>>> +}
>>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>>> +{
>>>>>> + return enable_kprobe(&rp->kp);
>>>>>> +}
>>>>>> +
>>>>>>  #else /* CONFIG_KRETPROBES */
>>>>>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>>>>>   struct pt_regs *regs)
>>>>>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>>>>>  {
>>>>>>   return 0;
>>>>>>  }
>>>>>> -#endif /* CONFIG_KRETPROBES */
>>>>>> -
>>>>>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>>> -
>>>>>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>>  {
>>>>>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>>> - ri->rp, ri->rp->kp.addr);
>>>>>> - BUG();
>>>>>> - }
>>>>>>  }
>>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>
>>>>> No, these should returns -EINVAL or -ENOSYS, since these are user API.
>>>>
>>>> OK, thanks, it sounds reasonable to me.
>>>>
>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>> be ignored.
>>>>>
>>>>
>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>
>>> Really? where are they called? I mean, those functions do not have
>>> any instance unless your module uses it (but that is not what the kernel
>>> itself should help).
>>>
>>
>> If what you said correct (I guess so), for me, we still need let them in
>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
> 
> kretprobe_assert() is only for the internal check. So we don't need to care
> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
> kretprobe can not be registered. And all of them are inlined functions.
> In that case, we don't need to care about it.

Hmm... it is related with code 'consistency':

 - these static inline functions are kretprobe generic implementation,
   and we are trying to let all kretprobe generic implementation within
   CONFIG_KRETPROBES area.

 - And original kprobe static inline functions have done like that,
   in same header file, if no obvious reasons, we can try to follow.


> I just concerned that it is a waste of memory if there are useless kretprobe
> related instances are built when CONFIG_KRETPROBES=n.
> 

Yeah, that is also one of reason (3rd reason).


And if necessary, please help check what we have done whether already
"let all kretprobe generic implementation within CONFIG_KRETPROBES area"
(exclude declaration, struct/union definition, and architecture
implementation).


> Thank you,
> 

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Masami Hiramatsu]

(2014/02/05 9:18), Chen Gang wrote:
> On 02/04/2014 11:39 PM, Masami Hiramatsu wrote:
>> (2014/02/04 22:53), Chen Gang wrote:
>>> On 02/04/2014 09:29 PM, Masami Hiramatsu wrote:
>>>> (2014/02/04 21:07), Chen Gang wrote:
>>>>> On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
>>>>>> (2014/02/04 14:16), Chen Gang wrote:
>>>>>>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>>>>>>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>>>>>>
>>>>>>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>>>>>>
>>>>>>>  - in "include/linux/kprobes.h":
>>>>>>>
>>>>>>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>>>>>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>>>>>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>>>>>>
>>>>>>>  - in "kernel/kprobes.c":
>>>>>>>
>>>>>>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>>>>>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>>>>>>      define init_kretprobes() to let init_kprobes() call.
>>>>>>>
>>>>>>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>>>>>>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>>>>>>> bzImage and Modpost modules) under x86_64 defconfig.
>>>>>>
>>>>>> Thanks for the fix! and I have some comments below.
>>>>>>
>>>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>>> ---
>>>>>>>  include/linux/kprobes.h |  58 +++++----
>>>>>>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>>>>>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>>>>>> index 925eaf2..c0d1212 100644
>>>>>>> --- a/include/linux/kprobes.h
>>>>>>> +++ b/include/linux/kprobes.h
>>>>>>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>>>>>>   return 1;
>>>>>>>  }
>>>>>>>  
>>>>>>> +int disable_kprobe(struct kprobe *kp);
>>>>>>> +int enable_kprobe(struct kprobe *kp);
>>>>>>> +
>>>>>>> +void dump_kprobe(struct kprobe *kp);
>>>>>>> +
>>>>>>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>>>> +
>>>>>>>  #ifdef CONFIG_KRETPROBES
>>>>>>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>>>>>>     struct pt_regs *regs);
>>>>>>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>>>>>>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>>> +{
>>>>>>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>>>> + printk(KERN_ERR
>>>>>>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>>>> + ri->rp, ri->rp->kp.addr);
>>>>>>> + BUG();
>>>>>>> + }
>>>>>>> +}
>>>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>>>> +{
>>>>>>> + return disable_kprobe(&rp->kp);
>>>>>>> +}
>>>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>>>> +{
>>>>>>> + return enable_kprobe(&rp->kp);
>>>>>>> +}
>>>>>>> +
>>>>>>>  #else /* CONFIG_KRETPROBES */
>>>>>>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>>>>>>   struct pt_regs *regs)
>>>>>>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>>>>>>  {
>>>>>>>   return 0;
>>>>>>>  }
>>>>>>> -#endif /* CONFIG_KRETPROBES */
>>>>>>> -
>>>>>>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>>>> -
>>>>>>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>>>  {
>>>>>>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>>>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>>>> - ri->rp, ri->rp->kp.addr);
>>>>>>> - BUG();
>>>>>>> - }
>>>>>>>  }
>>>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>>>> +{
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>>>> +{
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>
>>>>>> No, these should returns -EINVAL or -ENOSYS, since these are user API.
>>>>>
>>>>> OK, thanks, it sounds reasonable to me.
>>>>>
>>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>>> be ignored.
>>>>>>
>>>>>
>>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>>
>>>> Really? where are they called? I mean, those functions do not have
>>>> any instance unless your module uses it (but that is not what the kernel
>>>> itself should help).
>>>>
>>>
>>> If what you said correct (I guess so), for me, we still need let them in
>>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
>>
>> kretprobe_assert() is only for the internal check. So we don't need to care
>> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
>> kretprobe can not be registered. And all of them are inlined functions.
>> In that case, we don't need to care about it.
> 
> Hmm... it is related with code 'consistency':
> 
>  - these static inline functions are kretprobe generic implementation,
>    and we are trying to let all kretprobe generic implementation within
>    CONFIG_KRETPROBES area.

No, actually, kretprobe is just built on the kprobe. enable/disable_kretprobe
just wrapped the kprobe methods. And kretprobe_assert() is just for kretprobe
internal use. that is not an API. Moving only the kretprobe_assert() into the
CONFIG_KRETPROBE area is not bad, but since it is just a static inline function,
if there is no caller, it just be ignored, no side effect.

> 
>  - And original kprobe static inline functions have done like that,
>    in same header file, if no obvious reasons, we can try to follow.

It is no reasons to follow that too. Please keep your patch simple as much
as possible.

>> I just concerned that it is a waste of memory if there are useless kretprobe
>> related instances are built when CONFIG_KRETPROBES=n.
>>
> 
> Yeah, that is also one of reason (3rd reason).
> 
> 
> And if necessary, please help check what we have done whether already
> "let all kretprobe generic implementation within CONFIG_KRETPROBES area"
> (exclude declaration, struct/union definition, and architecture
> implementation).

As I commented, your changes in kernel/kprobes.c are good to me except
two functions. That's all what we need to fix :)

Thank you!

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

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

On 02/05/2014 09:21 AM, Masami Hiramatsu wrote:
> (2014/02/05 9:18), Chen Gang wrote:
>> On 02/04/2014 11:39 PM, Masami Hiramatsu wrote:
>>> (2014/02/04 22:53), Chen Gang wrote:
>>>> On 02/04/2014 09:29 PM, Masami Hiramatsu wrote:
>>>>> (2014/02/04 21:07), Chen Gang wrote:
>>>>>> On 02/04/2014 03:17 PM, Masami Hiramatsu wrote:
>>>>>>> (2014/02/04 14:16), Chen Gang wrote:
>>>>>>>> When CONFIG_KRETPROBES disabled, all *kretprobe* generic implementation
>>>>>>>> are useless, so need move them to CONFIG_KPROBES enabled area.
>>>>>>>>
>>>>>>>> Now, *kretprobe* generic implementation are all implemented in 2 files:
>>>>>>>>
>>>>>>>>  - in "include/linux/kprobes.h":
>>>>>>>>
>>>>>>>>      move inline kretprobe*() to CONFIG_KPROBES area and dummy outside.
>>>>>>>>      move some *kprobe() declarations which kretprobe*() call, to front.
>>>>>>>>      not touch kretprobe_blacklist[] which is architecture's variable.
>>>>>>>>
>>>>>>>>  - in "kernel/kprobes.c":
>>>>>>>>
>>>>>>>>      move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>>>>>>>>      define kretprobe_flush_task() to let kprobe_flush_task() call.
>>>>>>>>      define init_kretprobes() to let init_kprobes() call.
>>>>>>>>
>>>>>>>> The patch passes compiling (get "kernel/kprobes.o" and "kernel/built-
>>>>>>>> in.o") under avr32 and x86_64 allmodconfig, and passes building (get
>>>>>>>> bzImage and Modpost modules) under x86_64 defconfig.
>>>>>>>
>>>>>>> Thanks for the fix! and I have some comments below.
>>>>>>>
>>>>>>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>>>>>>> ---
>>>>>>>>  include/linux/kprobes.h |  58 +++++----
>>>>>>>>  kernel/kprobes.c        | 328 +++++++++++++++++++++++++++---------------------
>>>>>>>>  2 files changed, 222 insertions(+), 164 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>>>>>>>> index 925eaf2..c0d1212 100644
>>>>>>>> --- a/include/linux/kprobes.h
>>>>>>>> +++ b/include/linux/kprobes.h
>>>>>>>> @@ -223,10 +223,36 @@ static inline int kprobes_built_in(void)
>>>>>>>>   return 1;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +int disable_kprobe(struct kprobe *kp);
>>>>>>>> +int enable_kprobe(struct kprobe *kp);
>>>>>>>> +
>>>>>>>> +void dump_kprobe(struct kprobe *kp);
>>>>>>>> +
>>>>>>>> +extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>>>>> +
>>>>>>>>  #ifdef CONFIG_KRETPROBES
>>>>>>>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>>>>>>>     struct pt_regs *regs);
>>>>>>>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>>>>>>>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>>>> + unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>>>> +{
>>>>>>>> + if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>>>>> + printk(KERN_ERR
>>>>>>>> + "kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>>>>> + ri->rp, ri->rp->kp.addr);
>>>>>>>> + BUG();
>>>>>>>> + }
>>>>>>>> +}
>>>>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>>>>> +{
>>>>>>>> + return disable_kprobe(&rp->kp);
>>>>>>>> +}
>>>>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>>>>> +{
>>>>>>>> + return enable_kprobe(&rp->kp);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  #else /* CONFIG_KRETPROBES */
>>>>>>>>  static inline void arch_prepare_kretprobe(struct kretprobe *rp,
>>>>>>>>   struct pt_regs *regs)
>>>>>>>> @@ -236,19 +262,20 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>>>>>>>  {
>>>>>>>>   return 0;
>>>>>>>>  }
>>>>>>>> -#endif /* CONFIG_KRETPROBES */
>>>>>>>> -
>>>>>>>> -extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>>>>>>> -
>>>>>>>>  static inline void kretprobe_assert(struct kretprobe_instance *ri,
>>>>>>>>   unsigned long orig_ret_address, unsigned long trampoline_address)
>>>>>>>>  {
>>>>>>>> - if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>>>>>>>> - printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>>>>>>>> - ri->rp, ri->rp->kp.addr);
>>>>>>>> - BUG();
>>>>>>>> - }
>>>>>>>>  }
>>>>>>>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>>>>>>> +{
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>>>>>>> +{
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>
>>>>>>> No, these should returns -EINVAL or -ENOSYS, since these are user API.
>>>>>>
>>>>>> OK, thanks, it sounds reasonable to me.
>>>>>>
>>>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>>>> be ignored.
>>>>>>>
>>>>>>
>>>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>>>
>>>>> Really? where are they called? I mean, those functions do not have
>>>>> any instance unless your module uses it (but that is not what the kernel
>>>>> itself should help).
>>>>>
>>>>
>>>> If what you said correct (I guess so), for me, we still need let them in
>>>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>>>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
>>>
>>> kretprobe_assert() is only for the internal check. So we don't need to care
>>> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
>>> kretprobe can not be registered. And all of them are inlined functions.
>>> In that case, we don't need to care about it.
>>
>> Hmm... it is related with code 'consistency':
>>
>>  - these static inline functions are kretprobe generic implementation,
>>    and we are trying to let all kretprobe generic implementation within
>>    CONFIG_KRETPROBES area.
> 
> No, actually, kretprobe is just built on the kprobe. enable/disable_kretprobe
> just wrapped the kprobe methods. And kretprobe_assert() is just for kretprobe
> internal use. that is not an API. Moving only the kretprobe_assert() into the
> CONFIG_KRETPROBE area is not bad, but since it is just a static inline function,
> if there is no caller, it just be ignored, no side effect.
> 

OK, I can understand.

And do you mean enable/disable_kretprobe() are API? if so, we have to
implement them whether CONFIG_KRETPROBES enabled or disabled.

And when CONFIG_KRETPROBES=n, just like what you originally said: we
need returns -EINVAL directly (either, I am not quite sure whether the
input parameter will be NULL, in this case).

>>
>>  - And original kprobe static inline functions have done like that,
>>    in same header file, if no obvious reasons, we can try to follow.
> 
> It is no reasons to follow that too. Please keep your patch simple as much
> as possible.
> 

"keep our patch simple as much as posssible" sounds reasonable to me.
After skip "include/linux/kprobe.h", our patch's subject (include
comments) also need be changed (I will/should change it).

For me, "include/linux/kprobe.h" can also be improved, but it can be
another patch for it (not only for kretprobe, but also for jpobe).


>>> I just concerned that it is a waste of memory if there are useless kretprobe
>>> related instances are built when CONFIG_KRETPROBES=n.
>>>
>>
>> Yeah, that is also one of reason (3rd reason).
>>
>>
>> And if necessary, please help check what we have done whether already
>> "let all kretprobe generic implementation within CONFIG_KRETPROBES area"
>> (exclude declaration, struct/union definition, and architecture
>> implementation).
> 
> As I commented, your changes in kernel/kprobes.c are good to me except
> two functions. That's all what we need to fix :)
> 

I will send a patch for it (since subject changed, we need not mark
"patch v2"), thanks.  :-)

> Thank you!
> 

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

[PATCH] kernel/kprobes.c: move kretprobe implementation to CONFIG_KRETPROBES area

[Chen Gang]

When CONFIG_KRETPROBES disabled, kretprobe implementation are useless,
so need move them to CONFIG_KPROBES area.

 - move all kretprobe* to CONFIG_KPROBES area and dummy outside.
 - define kretprobe_flush_task() to let kprobe_flush_task() call.
 - define init_kretprobes() to let init_kprobes() call.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 kernel/kprobes.c | 323 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 181 insertions(+), 142 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ceeadfc..0619536 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -69,7 +69,6 @@
 
 static int kprobes_initialized;
 static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
-static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
 
 /* NOTE: change this value only with kprobe_mutex held */
 static bool kprobes_all_disarmed;
@@ -77,14 +76,6 @@ static bool kprobes_all_disarmed;
 /* This protects kprobe_table and optimizing_list */
 static DEFINE_MUTEX(kprobe_mutex);
 static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
-static struct {
-	raw_spinlock_t lock ____cacheline_aligned_in_smp;
-} kretprobe_table_locks[KPROBE_TABLE_SIZE];
-
-static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
-{
-	return &(kretprobe_table_locks[hash].lock);
-}
 
 /*
  * Normally, functions that we'd want to prohibit kprobes in, are marked
@@ -1079,125 +1070,6 @@ void __kprobes kprobes_inc_nmissed_count(struct kprobe *p)
 	return;
 }
 
-void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
-				struct hlist_head *head)
-{
-	struct kretprobe *rp = ri->rp;
-
-	/* remove rp inst off the rprobe_inst_table */
-	hlist_del(&ri->hlist);
-	INIT_HLIST_NODE(&ri->hlist);
-	if (likely(rp)) {
-		raw_spin_lock(&rp->lock);
-		hlist_add_head(&ri->hlist, &rp->free_instances);
-		raw_spin_unlock(&rp->lock);
-	} else
-		/* Unregistering */
-		hlist_add_head(&ri->hlist, head);
-}
-
-void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
-			 struct hlist_head **head, unsigned long *flags)
-__acquires(hlist_lock)
-{
-	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
-	raw_spinlock_t *hlist_lock;
-
-	*head = &kretprobe_inst_table[hash];
-	hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_lock_irqsave(hlist_lock, *flags);
-}
-
-static void __kprobes kretprobe_table_lock(unsigned long hash,
-	unsigned long *flags)
-__acquires(hlist_lock)
-{
-	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_lock_irqsave(hlist_lock, *flags);
-}
-
-void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
-	unsigned long *flags)
-__releases(hlist_lock)
-{
-	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
-	raw_spinlock_t *hlist_lock;
-
-	hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_unlock_irqrestore(hlist_lock, *flags);
-}
-
-static void __kprobes kretprobe_table_unlock(unsigned long hash,
-       unsigned long *flags)
-__releases(hlist_lock)
-{
-	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_unlock_irqrestore(hlist_lock, *flags);
-}
-
-/*
- * This function is called from finish_task_switch when task tk becomes dead,
- * so that we can recycle any function-return probe instances associated
- * with this task. These left over instances represent probed functions
- * that have been called but will never return.
- */
-void __kprobes kprobe_flush_task(struct task_struct *tk)
-{
-	struct kretprobe_instance *ri;
-	struct hlist_head *head, empty_rp;
-	struct hlist_node *tmp;
-	unsigned long hash, flags = 0;
-
-	if (unlikely(!kprobes_initialized))
-		/* Early boot.  kretprobe_table_locks not yet initialized. */
-		return;
-
-	INIT_HLIST_HEAD(&empty_rp);
-	hash = hash_ptr(tk, KPROBE_HASH_BITS);
-	head = &kretprobe_inst_table[hash];
-	kretprobe_table_lock(hash, &flags);
-	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
-		if (ri->task == tk)
-			recycle_rp_inst(ri, &empty_rp);
-	}
-	kretprobe_table_unlock(hash, &flags);
-	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
-		hlist_del(&ri->hlist);
-		kfree(ri);
-	}
-}
-
-static inline void free_rp_inst(struct kretprobe *rp)
-{
-	struct kretprobe_instance *ri;
-	struct hlist_node *next;
-
-	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
-		hlist_del(&ri->hlist);
-		kfree(ri);
-	}
-}
-
-static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
-{
-	unsigned long flags, hash;
-	struct kretprobe_instance *ri;
-	struct hlist_node *next;
-	struct hlist_head *head;
-
-	/* No race here */
-	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
-		kretprobe_table_lock(hash, &flags);
-		head = &kretprobe_inst_table[hash];
-		hlist_for_each_entry_safe(ri, next, head, hlist) {
-			if (ri->rp == rp)
-				ri->rp = NULL;
-		}
-		kretprobe_table_unlock(hash, &flags);
-	}
-	free_rp_inst(rp);
-}
-
 /*
 * Add the new probe to ap->list. Fail if this is the
 * second jprobe at the address - two jprobes can't coexist
@@ -1764,6 +1636,55 @@ void __kprobes unregister_jprobes(struct jprobe **jps, int num)
 EXPORT_SYMBOL_GPL(unregister_jprobes);
 
 #ifdef CONFIG_KRETPROBES
+static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
+static struct {
+	raw_spinlock_t lock ____cacheline_aligned_in_smp;
+} kretprobe_table_locks[KPROBE_TABLE_SIZE];
+
+static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
+{
+	return &(kretprobe_table_locks[hash].lock);
+}
+
+void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
+			 struct hlist_head **head, unsigned long *flags)
+__acquires(hlist_lock)
+{
+	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
+	raw_spinlock_t *hlist_lock;
+
+	*head = &kretprobe_inst_table[hash];
+	hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_lock_irqsave(hlist_lock, *flags);
+}
+
+void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
+	unsigned long *flags)
+__releases(hlist_lock)
+{
+	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
+	raw_spinlock_t *hlist_lock;
+
+	hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_unlock_irqrestore(hlist_lock, *flags);
+}
+
+static void __kprobes kretprobe_table_lock(unsigned long hash,
+	unsigned long *flags)
+__acquires(hlist_lock)
+{
+	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_lock_irqsave(hlist_lock, *flags);
+}
+
+static void __kprobes kretprobe_table_unlock(unsigned long hash,
+	unsigned long *flags)
+__releases(hlist_lock)
+{
+	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
+	raw_spin_unlock_irqrestore(hlist_lock, *flags);
+}
+
 /*
  * This kprobe pre_handler is registered with every kretprobe. When probe
  * hits it will set up the return probe.
@@ -1808,6 +1729,17 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
 	return 0;
 }
 
+static inline void free_rp_inst(struct kretprobe *rp)
+{
+	struct kretprobe_instance *ri;
+	struct hlist_node *next;
+
+	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
+}
+
 int __kprobes register_kretprobe(struct kretprobe *rp)
 {
 	int ret = 0;
@@ -1885,6 +1817,26 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
 }
 EXPORT_SYMBOL_GPL(unregister_kretprobe);
 
+static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
+{
+	unsigned long flags, hash;
+	struct kretprobe_instance *ri;
+	struct hlist_node *next;
+	struct hlist_head *head;
+
+	/* No race here */
+	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
+		kretprobe_table_lock(hash, &flags);
+		head = &kretprobe_inst_table[hash];
+		hlist_for_each_entry_safe(ri, next, head, hlist) {
+			if (ri->rp == rp)
+				ri->rp = NULL;
+		}
+		kretprobe_table_unlock(hash, &flags);
+	}
+	free_rp_inst(rp);
+}
+
 void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
 {
 	int i;
@@ -1907,7 +1859,78 @@ void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
 }
 EXPORT_SYMBOL_GPL(unregister_kretprobes);
 
+void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
+				struct hlist_head *head)
+{
+	struct kretprobe *rp = ri->rp;
+
+	/* remove rp inst off the rprobe_inst_table */
+	hlist_del(&ri->hlist);
+	INIT_HLIST_NODE(&ri->hlist);
+	if (likely(rp)) {
+		raw_spin_lock(&rp->lock);
+		hlist_add_head(&ri->hlist, &rp->free_instances);
+		raw_spin_unlock(&rp->lock);
+	} else
+		/* Unregistering */
+		hlist_add_head(&ri->hlist, head);
+}
+
+static void __kprobes kretprobe_flush_task(struct task_struct *tk)
+{
+	struct kretprobe_instance *ri;
+	struct hlist_head *head, empty_rp;
+	struct hlist_node *tmp;
+	unsigned long hash, flags = 0;
+
+	if (unlikely(!kprobes_initialized))
+		/* Early boot.  kretprobe_table_locks not yet initialized. */
+		return;
+
+	INIT_HLIST_HEAD(&empty_rp);
+	hash = hash_ptr(tk, KPROBE_HASH_BITS);
+	head = &kretprobe_inst_table[hash];
+	kretprobe_table_lock(hash, &flags);
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task == tk)
+			recycle_rp_inst(ri, &empty_rp);
+	}
+	kretprobe_table_unlock(hash, &flags);
+	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
+		hlist_del(&ri->hlist);
+		kfree(ri);
+	}
+}
+
+static void __init init_kretprobes(void)
+{
+	int i;
+
+	/* FIXME allocate the probe table, currently defined statically */
+	/* initialize all list heads */
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
+		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
+	}
+
+	if (kretprobe_blacklist_size) {
+		/* lookup the function address from its name */
+		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+			kprobe_lookup_name(kretprobe_blacklist[i].name,
+					   kretprobe_blacklist[i].addr);
+			if (!kretprobe_blacklist[i].addr)
+				printk(KERN_WARNING
+					"kretprobe: lookup failed: %s\n",
+					kretprobe_blacklist[i].name);
+		}
+	}
+}
+
 #else /* CONFIG_KRETPROBES */
+
+#define kretprobe_flush_task(p)			do {} while (0)
+#define init_kretprobes()			do {} while (0)
+
 int __kprobes register_kretprobe(struct kretprobe *rp)
 {
 	return -ENOSYS;
@@ -1936,8 +1959,35 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
 	return 0;
 }
 
+void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
+				struct hlist_head *head)
+{
+}
+
+void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
+			 struct hlist_head **head, unsigned long *flags)
+__acquires(hlist_lock)
+{
+}
+
+void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
+	unsigned long *flags)
+__releases(hlist_lock)
+{
+}
 #endif /* CONFIG_KRETPROBES */
 
+/*
+ * This function is called from finish_task_switch when task tk becomes dead,
+ * so that we can recycle any function-return probe instances associated
+ * with this task. These left over instances represent probed functions
+ * that have been called but will never return.
+ */
+void __kprobes kprobe_flush_task(struct task_struct *tk)
+{
+	kretprobe_flush_task(tk);
+}
+
 /* Set the kprobe gone and remove its instruction buffer. */
 static void __kprobes kill_kprobe(struct kprobe *p)
 {
@@ -2073,11 +2123,8 @@ static int __init init_kprobes(void)
 
 	/* FIXME allocate the probe table, currently defined statically */
 	/* initialize all list heads */
-	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
+	for (i = 0; i < KPROBE_TABLE_SIZE; i++)
 		INIT_HLIST_HEAD(&kprobe_table[i]);
-		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
-		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
-	}
 
 	/*
 	 * Lookup and populate the kprobe_blacklist.
@@ -2101,16 +2148,8 @@ static int __init init_kprobes(void)
 			kb->range = size;
 	}
 
-	if (kretprobe_blacklist_size) {
-		/* lookup the function address from its name */
-		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
-			kprobe_lookup_name(kretprobe_blacklist[i].name,
-					   kretprobe_blacklist[i].addr);
-			if (!kretprobe_blacklist[i].addr)
-				printk("kretprobe: lookup failed: %s\n",
-				       kretprobe_blacklist[i].name);
-		}
-	}
+	/* Initialize kretprobes */
+	init_kretprobes();
 
 #if defined(CONFIG_OPTPROBES)
 #if defined(__ARCH_WANT_KPROBES_INSN_SLOT)
-- 
1.7.11.7

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Masami Hiramatsu]

(2014/02/05 12:08), Chen Gang wrote:
>>>>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>>>>> be ignored.
>>>>>>>>
>>>>>>>
>>>>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>>>>
>>>>>> Really? where are they called? I mean, those functions do not have
>>>>>> any instance unless your module uses it (but that is not what the kernel
>>>>>> itself should help).
>>>>>>
>>>>>
>>>>> If what you said correct (I guess so), for me, we still need let them in
>>>>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>>>>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
>>>>
>>>> kretprobe_assert() is only for the internal check. So we don't need to care
>>>> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
>>>> kretprobe can not be registered. And all of them are inlined functions.
>>>> In that case, we don't need to care about it.
>>>
>>> Hmm... it is related with code 'consistency':
>>>
>>>  - these static inline functions are kretprobe generic implementation,
>>>    and we are trying to let all kretprobe generic implementation within
>>>    CONFIG_KRETPROBES area.
>>
>> No, actually, kretprobe is just built on the kprobe. enable/disable_kretprobe
>> just wrapped the kprobe methods. And kretprobe_assert() is just for kretprobe
>> internal use. that is not an API. Moving only the kretprobe_assert() into the
>> CONFIG_KRETPROBE area is not bad, but since it is just a static inline function,
>> if there is no caller, it just be ignored, no side effect.
>>
> 
> OK, I can understand.
> 
> And do you mean enable/disable_kretprobe() are API? if so, we have to
> implement them whether CONFIG_KRETPROBES enabled or disabled.
> 
> And when CONFIG_KRETPROBES=n, just like what you originally said: we
> need returns -EINVAL directly (either, I am not quite sure whether the
> input parameter will be NULL, in this case).

Both are API, and when implementing it I had also considered that, but
I decided to stay it in inline-function wrapper. The reason why is,
that enable/disable_k*probe require the registered k*probes. If the
kernel hacker uses those functions, they must ensure registering his
k*probes, otherwise it does not work correctly. If the CONFIG_KRETPROBES=n,
register_kretprobe() always fails, this means that the code has
no chance to call those functions (it must be).

>>>  - And original kprobe static inline functions have done like that,
>>>    in same header file, if no obvious reasons, we can try to follow.
>>
>> It is no reasons to follow that too. Please keep your patch simple as much
>> as possible.
>>
> 
> "keep our patch simple as much as posssible" sounds reasonable to me.
> After skip "include/linux/kprobe.h", our patch's subject (include
> comments) also need be changed (I will/should change it).
> 
> For me, "include/linux/kprobe.h" can also be improved, but it can be
> another patch for it (not only for kretprobe, but also for jpobe).

if that "improvement" means "simplify", it is acceptable. Now I don't like
ifdefs of CONFIG_KPROBES and dummy functions, since if CONFIG_KPROBES=n,
other kernel modules can also check the kconfig and decide what they do
(or don't).
Perhaps, what we've really needed is "just enough able to compile",
not the fully covered dummy APIs.

>>>> I just concerned that it is a waste of memory if there are useless kretprobe
>>>> related instances are built when CONFIG_KRETPROBES=n.
>>>>
>>>
>>> Yeah, that is also one of reason (3rd reason).
>>>
>>>
>>> And if necessary, please help check what we have done whether already
>>> "let all kretprobe generic implementation within CONFIG_KRETPROBES area"
>>> (exclude declaration, struct/union definition, and architecture
>>> implementation).
>>
>> As I commented, your changes in kernel/kprobes.c are good to me except
>> two functions. That's all what we need to fix :)
>>
> 
> I will send a patch for it (since subject changed, we need not mark
> "patch v2"), thanks.  :-)

OK, I'll review that.

Thank you!


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

Re: [PATCH] kernel/kprobes.c: move kretprobe implementation to CONFIG_KRETPROBES area

[Masami Hiramatsu]

(2014/02/05 12:36), Chen Gang wrote:
> When CONFIG_KRETPROBES disabled, kretprobe implementation are useless,
> so need move them to CONFIG_KPROBES area.
> 
>  - move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>  - define kretprobe_flush_task() to let kprobe_flush_task() call.
>  - define init_kretprobes() to let init_kprobes() call.
> 
> 

Looks good to me ;)

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

> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  kernel/kprobes.c | 323 +++++++++++++++++++++++++++++++------------------------
>  1 file changed, 181 insertions(+), 142 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ceeadfc..0619536 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -69,7 +69,6 @@
>  
>  static int kprobes_initialized;
>  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
> -static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
>  
>  /* NOTE: change this value only with kprobe_mutex held */
>  static bool kprobes_all_disarmed;
> @@ -77,14 +76,6 @@ static bool kprobes_all_disarmed;
>  /* This protects kprobe_table and optimizing_list */
>  static DEFINE_MUTEX(kprobe_mutex);
>  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
> -static struct {
> -	raw_spinlock_t lock ____cacheline_aligned_in_smp;
> -} kretprobe_table_locks[KPROBE_TABLE_SIZE];
> -
> -static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
> -{
> -	return &(kretprobe_table_locks[hash].lock);
> -}
>  
>  /*
>   * Normally, functions that we'd want to prohibit kprobes in, are marked
> @@ -1079,125 +1070,6 @@ void __kprobes kprobes_inc_nmissed_count(struct kprobe *p)
>  	return;
>  }
>  
> -void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
> -				struct hlist_head *head)
> -{
> -	struct kretprobe *rp = ri->rp;
> -
> -	/* remove rp inst off the rprobe_inst_table */
> -	hlist_del(&ri->hlist);
> -	INIT_HLIST_NODE(&ri->hlist);
> -	if (likely(rp)) {
> -		raw_spin_lock(&rp->lock);
> -		hlist_add_head(&ri->hlist, &rp->free_instances);
> -		raw_spin_unlock(&rp->lock);
> -	} else
> -		/* Unregistering */
> -		hlist_add_head(&ri->hlist, head);
> -}
> -
> -void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
> -			 struct hlist_head **head, unsigned long *flags)
> -__acquires(hlist_lock)
> -{
> -	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> -	raw_spinlock_t *hlist_lock;
> -
> -	*head = &kretprobe_inst_table[hash];
> -	hlist_lock = kretprobe_table_lock_ptr(hash);
> -	raw_spin_lock_irqsave(hlist_lock, *flags);
> -}
> -
> -static void __kprobes kretprobe_table_lock(unsigned long hash,
> -	unsigned long *flags)
> -__acquires(hlist_lock)
> -{
> -	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> -	raw_spin_lock_irqsave(hlist_lock, *flags);
> -}
> -
> -void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
> -	unsigned long *flags)
> -__releases(hlist_lock)
> -{
> -	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> -	raw_spinlock_t *hlist_lock;
> -
> -	hlist_lock = kretprobe_table_lock_ptr(hash);
> -	raw_spin_unlock_irqrestore(hlist_lock, *flags);
> -}
> -
> -static void __kprobes kretprobe_table_unlock(unsigned long hash,
> -       unsigned long *flags)
> -__releases(hlist_lock)
> -{
> -	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> -	raw_spin_unlock_irqrestore(hlist_lock, *flags);
> -}
> -
> -/*
> - * This function is called from finish_task_switch when task tk becomes dead,
> - * so that we can recycle any function-return probe instances associated
> - * with this task. These left over instances represent probed functions
> - * that have been called but will never return.
> - */
> -void __kprobes kprobe_flush_task(struct task_struct *tk)
> -{
> -	struct kretprobe_instance *ri;
> -	struct hlist_head *head, empty_rp;
> -	struct hlist_node *tmp;
> -	unsigned long hash, flags = 0;
> -
> -	if (unlikely(!kprobes_initialized))
> -		/* Early boot.  kretprobe_table_locks not yet initialized. */
> -		return;
> -
> -	INIT_HLIST_HEAD(&empty_rp);
> -	hash = hash_ptr(tk, KPROBE_HASH_BITS);
> -	head = &kretprobe_inst_table[hash];
> -	kretprobe_table_lock(hash, &flags);
> -	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> -		if (ri->task == tk)
> -			recycle_rp_inst(ri, &empty_rp);
> -	}
> -	kretprobe_table_unlock(hash, &flags);
> -	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> -		hlist_del(&ri->hlist);
> -		kfree(ri);
> -	}
> -}
> -
> -static inline void free_rp_inst(struct kretprobe *rp)
> -{
> -	struct kretprobe_instance *ri;
> -	struct hlist_node *next;
> -
> -	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
> -		hlist_del(&ri->hlist);
> -		kfree(ri);
> -	}
> -}
> -
> -static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
> -{
> -	unsigned long flags, hash;
> -	struct kretprobe_instance *ri;
> -	struct hlist_node *next;
> -	struct hlist_head *head;
> -
> -	/* No race here */
> -	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
> -		kretprobe_table_lock(hash, &flags);
> -		head = &kretprobe_inst_table[hash];
> -		hlist_for_each_entry_safe(ri, next, head, hlist) {
> -			if (ri->rp == rp)
> -				ri->rp = NULL;
> -		}
> -		kretprobe_table_unlock(hash, &flags);
> -	}
> -	free_rp_inst(rp);
> -}
> -
>  /*
>  * Add the new probe to ap->list. Fail if this is the
>  * second jprobe at the address - two jprobes can't coexist
> @@ -1764,6 +1636,55 @@ void __kprobes unregister_jprobes(struct jprobe **jps, int num)
>  EXPORT_SYMBOL_GPL(unregister_jprobes);
>  
>  #ifdef CONFIG_KRETPROBES
> +static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
> +static struct {
> +	raw_spinlock_t lock ____cacheline_aligned_in_smp;
> +} kretprobe_table_locks[KPROBE_TABLE_SIZE];
> +
> +static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
> +{
> +	return &(kretprobe_table_locks[hash].lock);
> +}
> +
> +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
> +			 struct hlist_head **head, unsigned long *flags)
> +__acquires(hlist_lock)
> +{
> +	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> +	raw_spinlock_t *hlist_lock;
> +
> +	*head = &kretprobe_inst_table[hash];
> +	hlist_lock = kretprobe_table_lock_ptr(hash);
> +	raw_spin_lock_irqsave(hlist_lock, *flags);
> +}
> +
> +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
> +	unsigned long *flags)
> +__releases(hlist_lock)
> +{
> +	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
> +	raw_spinlock_t *hlist_lock;
> +
> +	hlist_lock = kretprobe_table_lock_ptr(hash);
> +	raw_spin_unlock_irqrestore(hlist_lock, *flags);
> +}
> +
> +static void __kprobes kretprobe_table_lock(unsigned long hash,
> +	unsigned long *flags)
> +__acquires(hlist_lock)
> +{
> +	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> +	raw_spin_lock_irqsave(hlist_lock, *flags);
> +}
> +
> +static void __kprobes kretprobe_table_unlock(unsigned long hash,
> +	unsigned long *flags)
> +__releases(hlist_lock)
> +{
> +	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
> +	raw_spin_unlock_irqrestore(hlist_lock, *flags);
> +}
> +
>  /*
>   * This kprobe pre_handler is registered with every kretprobe. When probe
>   * hits it will set up the return probe.
> @@ -1808,6 +1729,17 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>  	return 0;
>  }
>  
> +static inline void free_rp_inst(struct kretprobe *rp)
> +{
> +	struct kretprobe_instance *ri;
> +	struct hlist_node *next;
> +
> +	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
> +		hlist_del(&ri->hlist);
> +		kfree(ri);
> +	}
> +}
> +
>  int __kprobes register_kretprobe(struct kretprobe *rp)
>  {
>  	int ret = 0;
> @@ -1885,6 +1817,26 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
>  }
>  EXPORT_SYMBOL_GPL(unregister_kretprobe);
>  
> +static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
> +{
> +	unsigned long flags, hash;
> +	struct kretprobe_instance *ri;
> +	struct hlist_node *next;
> +	struct hlist_head *head;
> +
> +	/* No race here */
> +	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
> +		kretprobe_table_lock(hash, &flags);
> +		head = &kretprobe_inst_table[hash];
> +		hlist_for_each_entry_safe(ri, next, head, hlist) {
> +			if (ri->rp == rp)
> +				ri->rp = NULL;
> +		}
> +		kretprobe_table_unlock(hash, &flags);
> +	}
> +	free_rp_inst(rp);
> +}
> +
>  void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
>  {
>  	int i;
> @@ -1907,7 +1859,78 @@ void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
>  }
>  EXPORT_SYMBOL_GPL(unregister_kretprobes);
>  
> +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
> +				struct hlist_head *head)
> +{
> +	struct kretprobe *rp = ri->rp;
> +
> +	/* remove rp inst off the rprobe_inst_table */
> +	hlist_del(&ri->hlist);
> +	INIT_HLIST_NODE(&ri->hlist);
> +	if (likely(rp)) {
> +		raw_spin_lock(&rp->lock);
> +		hlist_add_head(&ri->hlist, &rp->free_instances);
> +		raw_spin_unlock(&rp->lock);
> +	} else
> +		/* Unregistering */
> +		hlist_add_head(&ri->hlist, head);
> +}
> +
> +static void __kprobes kretprobe_flush_task(struct task_struct *tk)
> +{
> +	struct kretprobe_instance *ri;
> +	struct hlist_head *head, empty_rp;
> +	struct hlist_node *tmp;
> +	unsigned long hash, flags = 0;
> +
> +	if (unlikely(!kprobes_initialized))
> +		/* Early boot.  kretprobe_table_locks not yet initialized. */
> +		return;
> +
> +	INIT_HLIST_HEAD(&empty_rp);
> +	hash = hash_ptr(tk, KPROBE_HASH_BITS);
> +	head = &kretprobe_inst_table[hash];
> +	kretprobe_table_lock(hash, &flags);
> +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
> +		if (ri->task == tk)
> +			recycle_rp_inst(ri, &empty_rp);
> +	}
> +	kretprobe_table_unlock(hash, &flags);
> +	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
> +		hlist_del(&ri->hlist);
> +		kfree(ri);
> +	}
> +}
> +
> +static void __init init_kretprobes(void)
> +{
> +	int i;
> +
> +	/* FIXME allocate the probe table, currently defined statically */
> +	/* initialize all list heads */
> +	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> +		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
> +		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
> +	}
> +
> +	if (kretprobe_blacklist_size) {
> +		/* lookup the function address from its name */
> +		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> +			kprobe_lookup_name(kretprobe_blacklist[i].name,
> +					   kretprobe_blacklist[i].addr);
> +			if (!kretprobe_blacklist[i].addr)
> +				printk(KERN_WARNING
> +					"kretprobe: lookup failed: %s\n",
> +					kretprobe_blacklist[i].name);
> +		}
> +	}
> +}
> +
>  #else /* CONFIG_KRETPROBES */
> +
> +#define kretprobe_flush_task(p)			do {} while (0)
> +#define init_kretprobes()			do {} while (0)
> +
>  int __kprobes register_kretprobe(struct kretprobe *rp)
>  {
>  	return -ENOSYS;
> @@ -1936,8 +1959,35 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>  	return 0;
>  }
>  
> +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
> +				struct hlist_head *head)
> +{
> +}
> +
> +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
> +			 struct hlist_head **head, unsigned long *flags)
> +__acquires(hlist_lock)
> +{
> +}
> +
> +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
> +	unsigned long *flags)
> +__releases(hlist_lock)
> +{
> +}
>  #endif /* CONFIG_KRETPROBES */
>  
> +/*
> + * This function is called from finish_task_switch when task tk becomes dead,
> + * so that we can recycle any function-return probe instances associated
> + * with this task. These left over instances represent probed functions
> + * that have been called but will never return.
> + */
> +void __kprobes kprobe_flush_task(struct task_struct *tk)
> +{
> +	kretprobe_flush_task(tk);
> +}
> +
>  /* Set the kprobe gone and remove its instruction buffer. */
>  static void __kprobes kill_kprobe(struct kprobe *p)
>  {
> @@ -2073,11 +2123,8 @@ static int __init init_kprobes(void)
>  
>  	/* FIXME allocate the probe table, currently defined statically */
>  	/* initialize all list heads */
> -	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
> +	for (i = 0; i < KPROBE_TABLE_SIZE; i++)
>  		INIT_HLIST_HEAD(&kprobe_table[i]);
> -		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
> -		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
> -	}
>  
>  	/*
>  	 * Lookup and populate the kprobe_blacklist.
> @@ -2101,16 +2148,8 @@ static int __init init_kprobes(void)
>  			kb->range = size;
>  	}
>  
> -	if (kretprobe_blacklist_size) {
> -		/* lookup the function address from its name */
> -		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
> -			kprobe_lookup_name(kretprobe_blacklist[i].name,
> -					   kretprobe_blacklist[i].addr);
> -			if (!kretprobe_blacklist[i].addr)
> -				printk("kretprobe: lookup failed: %s\n",
> -				       kretprobe_blacklist[i].name);
> -		}
> -	}
> +	/* Initialize kretprobes */
> +	init_kretprobes();
>  
>  #if defined(CONFIG_OPTPROBES)
>  #if defined(__ARCH_WANT_KPROBES_INSN_SLOT)
> 


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

Re: [PATCH] kernel/kprobes.c: move kretprobe implementation to CONFIG_KRETPROBES area

[Chen Gang]

On 02/05/2014 01:00 PM, Masami Hiramatsu wrote:
> (2014/02/05 12:36), Chen Gang wrote:
>> > When CONFIG_KRETPROBES disabled, kretprobe implementation are useless,
>> > so need move them to CONFIG_KPROBES area.
>> > 
>> >  - move all kretprobe* to CONFIG_KPROBES area and dummy outside.
>> >  - define kretprobe_flush_task() to let kprobe_flush_task() call.
>> >  - define init_kretprobes() to let init_kprobes() call.
>> > 
>> > 
> Looks good to me ;)
> 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 

Thank you very much !!

:-)


>> > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> > ---
>> >  kernel/kprobes.c | 323 +++++++++++++++++++++++++++++++------------------------
>> >  1 file changed, 181 insertions(+), 142 deletions(-)
>> > 
>> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
>> > index ceeadfc..0619536 100644
>> > --- a/kernel/kprobes.c
>> > +++ b/kernel/kprobes.c
>> > @@ -69,7 +69,6 @@
>> >  
>> >  static int kprobes_initialized;
>> >  static struct hlist_head kprobe_table[KPROBE_TABLE_SIZE];
>> > -static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
>> >  
>> >  /* NOTE: change this value only with kprobe_mutex held */
>> >  static bool kprobes_all_disarmed;
>> > @@ -77,14 +76,6 @@ static bool kprobes_all_disarmed;
>> >  /* This protects kprobe_table and optimizing_list */
>> >  static DEFINE_MUTEX(kprobe_mutex);
>> >  static DEFINE_PER_CPU(struct kprobe *, kprobe_instance) = NULL;
>> > -static struct {
>> > -	raw_spinlock_t lock ____cacheline_aligned_in_smp;
>> > -} kretprobe_table_locks[KPROBE_TABLE_SIZE];
>> > -
>> > -static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
>> > -{
>> > -	return &(kretprobe_table_locks[hash].lock);
>> > -}
>> >  
>> >  /*
>> >   * Normally, functions that we'd want to prohibit kprobes in, are marked
>> > @@ -1079,125 +1070,6 @@ void __kprobes kprobes_inc_nmissed_count(struct kprobe *p)
>> >  	return;
>> >  }
>> >  
>> > -void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
>> > -				struct hlist_head *head)
>> > -{
>> > -	struct kretprobe *rp = ri->rp;
>> > -
>> > -	/* remove rp inst off the rprobe_inst_table */
>> > -	hlist_del(&ri->hlist);
>> > -	INIT_HLIST_NODE(&ri->hlist);
>> > -	if (likely(rp)) {
>> > -		raw_spin_lock(&rp->lock);
>> > -		hlist_add_head(&ri->hlist, &rp->free_instances);
>> > -		raw_spin_unlock(&rp->lock);
>> > -	} else
>> > -		/* Unregistering */
>> > -		hlist_add_head(&ri->hlist, head);
>> > -}
>> > -
>> > -void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
>> > -			 struct hlist_head **head, unsigned long *flags)
>> > -__acquires(hlist_lock)
>> > -{
>> > -	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
>> > -	raw_spinlock_t *hlist_lock;
>> > -
>> > -	*head = &kretprobe_inst_table[hash];
>> > -	hlist_lock = kretprobe_table_lock_ptr(hash);
>> > -	raw_spin_lock_irqsave(hlist_lock, *flags);
>> > -}
>> > -
>> > -static void __kprobes kretprobe_table_lock(unsigned long hash,
>> > -	unsigned long *flags)
>> > -__acquires(hlist_lock)
>> > -{
>> > -	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
>> > -	raw_spin_lock_irqsave(hlist_lock, *flags);
>> > -}
>> > -
>> > -void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
>> > -	unsigned long *flags)
>> > -__releases(hlist_lock)
>> > -{
>> > -	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
>> > -	raw_spinlock_t *hlist_lock;
>> > -
>> > -	hlist_lock = kretprobe_table_lock_ptr(hash);
>> > -	raw_spin_unlock_irqrestore(hlist_lock, *flags);
>> > -}
>> > -
>> > -static void __kprobes kretprobe_table_unlock(unsigned long hash,
>> > -       unsigned long *flags)
>> > -__releases(hlist_lock)
>> > -{
>> > -	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
>> > -	raw_spin_unlock_irqrestore(hlist_lock, *flags);
>> > -}
>> > -
>> > -/*
>> > - * This function is called from finish_task_switch when task tk becomes dead,
>> > - * so that we can recycle any function-return probe instances associated
>> > - * with this task. These left over instances represent probed functions
>> > - * that have been called but will never return.
>> > - */
>> > -void __kprobes kprobe_flush_task(struct task_struct *tk)
>> > -{
>> > -	struct kretprobe_instance *ri;
>> > -	struct hlist_head *head, empty_rp;
>> > -	struct hlist_node *tmp;
>> > -	unsigned long hash, flags = 0;
>> > -
>> > -	if (unlikely(!kprobes_initialized))
>> > -		/* Early boot.  kretprobe_table_locks not yet initialized. */
>> > -		return;
>> > -
>> > -	INIT_HLIST_HEAD(&empty_rp);
>> > -	hash = hash_ptr(tk, KPROBE_HASH_BITS);
>> > -	head = &kretprobe_inst_table[hash];
>> > -	kretprobe_table_lock(hash, &flags);
>> > -	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
>> > -		if (ri->task == tk)
>> > -			recycle_rp_inst(ri, &empty_rp);
>> > -	}
>> > -	kretprobe_table_unlock(hash, &flags);
>> > -	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
>> > -		hlist_del(&ri->hlist);
>> > -		kfree(ri);
>> > -	}
>> > -}
>> > -
>> > -static inline void free_rp_inst(struct kretprobe *rp)
>> > -{
>> > -	struct kretprobe_instance *ri;
>> > -	struct hlist_node *next;
>> > -
>> > -	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
>> > -		hlist_del(&ri->hlist);
>> > -		kfree(ri);
>> > -	}
>> > -}
>> > -
>> > -static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
>> > -{
>> > -	unsigned long flags, hash;
>> > -	struct kretprobe_instance *ri;
>> > -	struct hlist_node *next;
>> > -	struct hlist_head *head;
>> > -
>> > -	/* No race here */
>> > -	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
>> > -		kretprobe_table_lock(hash, &flags);
>> > -		head = &kretprobe_inst_table[hash];
>> > -		hlist_for_each_entry_safe(ri, next, head, hlist) {
>> > -			if (ri->rp == rp)
>> > -				ri->rp = NULL;
>> > -		}
>> > -		kretprobe_table_unlock(hash, &flags);
>> > -	}
>> > -	free_rp_inst(rp);
>> > -}
>> > -
>> >  /*
>> >  * Add the new probe to ap->list. Fail if this is the
>> >  * second jprobe at the address - two jprobes can't coexist
>> > @@ -1764,6 +1636,55 @@ void __kprobes unregister_jprobes(struct jprobe **jps, int num)
>> >  EXPORT_SYMBOL_GPL(unregister_jprobes);
>> >  
>> >  #ifdef CONFIG_KRETPROBES
>> > +static struct hlist_head kretprobe_inst_table[KPROBE_TABLE_SIZE];
>> > +static struct {
>> > +	raw_spinlock_t lock ____cacheline_aligned_in_smp;
>> > +} kretprobe_table_locks[KPROBE_TABLE_SIZE];
>> > +
>> > +static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
>> > +{
>> > +	return &(kretprobe_table_locks[hash].lock);
>> > +}
>> > +
>> > +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
>> > +			 struct hlist_head **head, unsigned long *flags)
>> > +__acquires(hlist_lock)
>> > +{
>> > +	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
>> > +	raw_spinlock_t *hlist_lock;
>> > +
>> > +	*head = &kretprobe_inst_table[hash];
>> > +	hlist_lock = kretprobe_table_lock_ptr(hash);
>> > +	raw_spin_lock_irqsave(hlist_lock, *flags);
>> > +}
>> > +
>> > +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
>> > +	unsigned long *flags)
>> > +__releases(hlist_lock)
>> > +{
>> > +	unsigned long hash = hash_ptr(tsk, KPROBE_HASH_BITS);
>> > +	raw_spinlock_t *hlist_lock;
>> > +
>> > +	hlist_lock = kretprobe_table_lock_ptr(hash);
>> > +	raw_spin_unlock_irqrestore(hlist_lock, *flags);
>> > +}
>> > +
>> > +static void __kprobes kretprobe_table_lock(unsigned long hash,
>> > +	unsigned long *flags)
>> > +__acquires(hlist_lock)
>> > +{
>> > +	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
>> > +	raw_spin_lock_irqsave(hlist_lock, *flags);
>> > +}
>> > +
>> > +static void __kprobes kretprobe_table_unlock(unsigned long hash,
>> > +	unsigned long *flags)
>> > +__releases(hlist_lock)
>> > +{
>> > +	raw_spinlock_t *hlist_lock = kretprobe_table_lock_ptr(hash);
>> > +	raw_spin_unlock_irqrestore(hlist_lock, *flags);
>> > +}
>> > +
>> >  /*
>> >   * This kprobe pre_handler is registered with every kretprobe. When probe
>> >   * hits it will set up the return probe.
>> > @@ -1808,6 +1729,17 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>> >  	return 0;
>> >  }
>> >  
>> > +static inline void free_rp_inst(struct kretprobe *rp)
>> > +{
>> > +	struct kretprobe_instance *ri;
>> > +	struct hlist_node *next;
>> > +
>> > +	hlist_for_each_entry_safe(ri, next, &rp->free_instances, hlist) {
>> > +		hlist_del(&ri->hlist);
>> > +		kfree(ri);
>> > +	}
>> > +}
>> > +
>> >  int __kprobes register_kretprobe(struct kretprobe *rp)
>> >  {
>> >  	int ret = 0;
>> > @@ -1885,6 +1817,26 @@ void __kprobes unregister_kretprobe(struct kretprobe *rp)
>> >  }
>> >  EXPORT_SYMBOL_GPL(unregister_kretprobe);
>> >  
>> > +static void __kprobes cleanup_rp_inst(struct kretprobe *rp)
>> > +{
>> > +	unsigned long flags, hash;
>> > +	struct kretprobe_instance *ri;
>> > +	struct hlist_node *next;
>> > +	struct hlist_head *head;
>> > +
>> > +	/* No race here */
>> > +	for (hash = 0; hash < KPROBE_TABLE_SIZE; hash++) {
>> > +		kretprobe_table_lock(hash, &flags);
>> > +		head = &kretprobe_inst_table[hash];
>> > +		hlist_for_each_entry_safe(ri, next, head, hlist) {
>> > +			if (ri->rp == rp)
>> > +				ri->rp = NULL;
>> > +		}
>> > +		kretprobe_table_unlock(hash, &flags);
>> > +	}
>> > +	free_rp_inst(rp);
>> > +}
>> > +
>> >  void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
>> >  {
>> >  	int i;
>> > @@ -1907,7 +1859,78 @@ void __kprobes unregister_kretprobes(struct kretprobe **rps, int num)
>> >  }
>> >  EXPORT_SYMBOL_GPL(unregister_kretprobes);
>> >  
>> > +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
>> > +				struct hlist_head *head)
>> > +{
>> > +	struct kretprobe *rp = ri->rp;
>> > +
>> > +	/* remove rp inst off the rprobe_inst_table */
>> > +	hlist_del(&ri->hlist);
>> > +	INIT_HLIST_NODE(&ri->hlist);
>> > +	if (likely(rp)) {
>> > +		raw_spin_lock(&rp->lock);
>> > +		hlist_add_head(&ri->hlist, &rp->free_instances);
>> > +		raw_spin_unlock(&rp->lock);
>> > +	} else
>> > +		/* Unregistering */
>> > +		hlist_add_head(&ri->hlist, head);
>> > +}
>> > +
>> > +static void __kprobes kretprobe_flush_task(struct task_struct *tk)
>> > +{
>> > +	struct kretprobe_instance *ri;
>> > +	struct hlist_head *head, empty_rp;
>> > +	struct hlist_node *tmp;
>> > +	unsigned long hash, flags = 0;
>> > +
>> > +	if (unlikely(!kprobes_initialized))
>> > +		/* Early boot.  kretprobe_table_locks not yet initialized. */
>> > +		return;
>> > +
>> > +	INIT_HLIST_HEAD(&empty_rp);
>> > +	hash = hash_ptr(tk, KPROBE_HASH_BITS);
>> > +	head = &kretprobe_inst_table[hash];
>> > +	kretprobe_table_lock(hash, &flags);
>> > +	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
>> > +		if (ri->task == tk)
>> > +			recycle_rp_inst(ri, &empty_rp);
>> > +	}
>> > +	kretprobe_table_unlock(hash, &flags);
>> > +	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
>> > +		hlist_del(&ri->hlist);
>> > +		kfree(ri);
>> > +	}
>> > +}
>> > +
>> > +static void __init init_kretprobes(void)
>> > +{
>> > +	int i;
>> > +
>> > +	/* FIXME allocate the probe table, currently defined statically */
>> > +	/* initialize all list heads */
>> > +	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>> > +		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
>> > +		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
>> > +	}
>> > +
>> > +	if (kretprobe_blacklist_size) {
>> > +		/* lookup the function address from its name */
>> > +		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
>> > +			kprobe_lookup_name(kretprobe_blacklist[i].name,
>> > +					   kretprobe_blacklist[i].addr);
>> > +			if (!kretprobe_blacklist[i].addr)
>> > +				printk(KERN_WARNING
>> > +					"kretprobe: lookup failed: %s\n",
>> > +					kretprobe_blacklist[i].name);
>> > +		}
>> > +	}
>> > +}
>> > +
>> >  #else /* CONFIG_KRETPROBES */
>> > +
>> > +#define kretprobe_flush_task(p)			do {} while (0)
>> > +#define init_kretprobes()			do {} while (0)
>> > +
>> >  int __kprobes register_kretprobe(struct kretprobe *rp)
>> >  {
>> >  	return -ENOSYS;
>> > @@ -1936,8 +1959,35 @@ static int __kprobes pre_handler_kretprobe(struct kprobe *p,
>> >  	return 0;
>> >  }
>> >  
>> > +void __kprobes recycle_rp_inst(struct kretprobe_instance *ri,
>> > +				struct hlist_head *head)
>> > +{
>> > +}
>> > +
>> > +void __kprobes kretprobe_hash_lock(struct task_struct *tsk,
>> > +			 struct hlist_head **head, unsigned long *flags)
>> > +__acquires(hlist_lock)
>> > +{
>> > +}
>> > +
>> > +void __kprobes kretprobe_hash_unlock(struct task_struct *tsk,
>> > +	unsigned long *flags)
>> > +__releases(hlist_lock)
>> > +{
>> > +}
>> >  #endif /* CONFIG_KRETPROBES */
>> >  
>> > +/*
>> > + * This function is called from finish_task_switch when task tk becomes dead,
>> > + * so that we can recycle any function-return probe instances associated
>> > + * with this task. These left over instances represent probed functions
>> > + * that have been called but will never return.
>> > + */
>> > +void __kprobes kprobe_flush_task(struct task_struct *tk)
>> > +{
>> > +	kretprobe_flush_task(tk);
>> > +}
>> > +
>> >  /* Set the kprobe gone and remove its instruction buffer. */
>> >  static void __kprobes kill_kprobe(struct kprobe *p)
>> >  {
>> > @@ -2073,11 +2123,8 @@ static int __init init_kprobes(void)
>> >  
>> >  	/* FIXME allocate the probe table, currently defined statically */
>> >  	/* initialize all list heads */
>> > -	for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
>> > +	for (i = 0; i < KPROBE_TABLE_SIZE; i++)
>> >  		INIT_HLIST_HEAD(&kprobe_table[i]);
>> > -		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
>> > -		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
>> > -	}
>> >  
>> >  	/*
>> >  	 * Lookup and populate the kprobe_blacklist.
>> > @@ -2101,16 +2148,8 @@ static int __init init_kprobes(void)
>> >  			kb->range = size;
>> >  	}
>> >  
>> > -	if (kretprobe_blacklist_size) {
>> > -		/* lookup the function address from its name */
>> > -		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
>> > -			kprobe_lookup_name(kretprobe_blacklist[i].name,
>> > -					   kretprobe_blacklist[i].addr);
>> > -			if (!kretprobe_blacklist[i].addr)
>> > -				printk("kretprobe: lookup failed: %s\n",
>> > -				       kretprobe_blacklist[i].name);
>> > -		}
>> > -	}
>> > +	/* Initialize kretprobes */
>> > +	init_kretprobes();
>> >  
>> >  #if defined(CONFIG_OPTPROBES)
>> >  #if defined(__ARCH_WANT_KPROBES_INSN_SLOT)
>> > 
> 
> -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory E-mail:
> masami.hiramatsu.pt@hitachi.com
> 


-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation to CONFIG_KRETPROBES enabled area

[Chen Gang]

On 02/05/2014 12:57 PM, Masami Hiramatsu wrote:
> (2014/02/05 12:08), Chen Gang wrote:
>>>>>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>>>>>> be ignored.
>>>>>>>>>
>>>>>>>>
>>>>>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>>>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>>>>>
>>>>>>> Really? where are they called? I mean, those functions do not have
>>>>>>> any instance unless your module uses it (but that is not what the kernel
>>>>>>> itself should help).
>>>>>>>
>>>>>>
>>>>>> If what you said correct (I guess so), for me, we still need let them in
>>>>>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>>>>>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
>>>>>
>>>>> kretprobe_assert() is only for the internal check. So we don't need to care
>>>>> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
>>>>> kretprobe can not be registered. And all of them are inlined functions.
>>>>> In that case, we don't need to care about it.
>>>>
>>>> Hmm... it is related with code 'consistency':
>>>>
>>>>  - these static inline functions are kretprobe generic implementation,
>>>>    and we are trying to let all kretprobe generic implementation within
>>>>    CONFIG_KRETPROBES area.
>>>
>>> No, actually, kretprobe is just built on the kprobe. enable/disable_kretprobe
>>> just wrapped the kprobe methods. And kretprobe_assert() is just for kretprobe
>>> internal use. that is not an API. Moving only the kretprobe_assert() into the
>>> CONFIG_KRETPROBE area is not bad, but since it is just a static inline function,
>>> if there is no caller, it just be ignored, no side effect.
>>>
>>
>> OK, I can understand.
>>
>> And do you mean enable/disable_kretprobe() are API? if so, we have to
>> implement them whether CONFIG_KRETPROBES enabled or disabled.
>>
>> And when CONFIG_KRETPROBES=n, just like what you originally said: we
>> need returns -EINVAL directly (either, I am not quite sure whether the
>> input parameter will be NULL, in this case).
> 
> Both are API, and when implementing it I had also considered that, but
> I decided to stay it in inline-function wrapper. The reason why is,
> that enable/disable_k*probe require the registered k*probes. If the
> kernel hacker uses those functions, they must ensure registering his
> k*probes, otherwise it does not work correctly. If the CONFIG_KRETPROBES=n,
> register_kretprobe() always fails, this means that the code has
> no chance to call those functions (it must be).
>

OK, thank you for your explanations.


>>>>  - And original kprobe static inline functions have done like that,
>>>>    in same header file, if no obvious reasons, we can try to follow.
>>>
>>> It is no reasons to follow that too. Please keep your patch simple as much
>>> as possible.
>>>
>>
>> "keep our patch simple as much as posssible" sounds reasonable to me.
>> After skip "include/linux/kprobe.h", our patch's subject (include
>> comments) also need be changed (I will/should change it).
>>
>> For me, "include/linux/kprobe.h" can also be improved, but it can be
>> another patch for it (not only for kretprobe, but also for jpobe).
> 
> if that "improvement" means "simplify", it is acceptable. Now I don't like
> ifdefs of CONFIG_KPROBES and dummy functions, since if CONFIG_KPROBES=n,
> other kernel modules can also check the kconfig and decide what they do
> (or don't).
> Perhaps, what we've really needed is "just enough able to compile",
> not the fully covered dummy APIs.
> 

Hmm... for me, I still try to send a patch for "include/linux/kprobe.h".

For API (although it is kernel internal API), I have a hobby to try to
let it 'beautiful' as much as possible.


>>>>> I just concerned that it is a waste of memory if there are useless kretprobe
>>>>> related instances are built when CONFIG_KRETPROBES=n.
>>>>>
>>>>
>>>> Yeah, that is also one of reason (3rd reason).
>>>>
>>>>
>>>> And if necessary, please help check what we have done whether already
>>>> "let all kretprobe generic implementation within CONFIG_KRETPROBES area"
>>>> (exclude declaration, struct/union definition, and architecture
>>>> implementation).
>>>
>>> As I commented, your changes in kernel/kprobes.c are good to me except
>>> two functions. That's all what we need to fix :)
>>>
>>
>> I will send a patch for it (since subject changed, we need not mark
>> "patch v2"), thanks.  :-)
> 
> OK, I'll review that.
> 

Thanks.

> Thank you!
> 
> 

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

[PATCH] include/linux/kprobes.h: move all functions to their matched area

[Chen Gang]

For dummy functions, it is not a good idea to still use the input
parameters (not a good idea to assume they are still effect).

 - let kprobe* static inline functions in CONFIG_KPROBES, dummy outside.

 - let (en/dis)able_jprobe() in CONFIG_KPROBES, dummy outside.

 - for kretprobe:

   - let kretprobe_assert() only in CONFIG_KRETPROBES (internal use).

   - remove kretprobe_inst_table_head() (cannot grep it in kernel wide).

   - for (en/dis)able_kretprobe():

       if CONFIG_KRETPROBES enabled, they use (en/dis)able_kprobe().
       else if CONFIG_KPROBES enabled, return -EINVAL (not registered).
       else, return -ENOSYS, just like (en/dis)able_kprobe() have done.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 include/linux/kprobes.h | 136 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 92 insertions(+), 44 deletions(-)

diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 925eaf2..860313d 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -129,30 +129,6 @@ struct kprobe {
 				   */
 #define KPROBE_FLAG_FTRACE	8 /* probe is using ftrace */
 
-/* Has this kprobe gone ? */
-static inline int kprobe_gone(struct kprobe *p)
-{
-	return p->flags & KPROBE_FLAG_GONE;
-}
-
-/* Is this kprobe disabled ? */
-static inline int kprobe_disabled(struct kprobe *p)
-{
-	return p->flags & (KPROBE_FLAG_DISABLED | KPROBE_FLAG_GONE);
-}
-
-/* Is this kprobe really running optimized path ? */
-static inline int kprobe_optimized(struct kprobe *p)
-{
-	return p->flags & KPROBE_FLAG_OPTIMIZED;
-}
-
-/* Is this kprobe uses ftrace ? */
-static inline int kprobe_ftrace(struct kprobe *p)
-{
-	return p->flags & KPROBE_FLAG_FTRACE;
-}
-
 /*
  * Special probe type that uses setjmp-longjmp type tricks to resume
  * execution at a specified entry with a matching prototype corresponding
@@ -223,7 +199,42 @@ static inline int kprobes_built_in(void)
 	return 1;
 }
 
+/* Has this kprobe gone ? */
+static inline int kprobe_gone(struct kprobe *p)
+{
+	return p->flags & KPROBE_FLAG_GONE;
+}
+
+/* Is this kprobe disabled ? */
+static inline int kprobe_disabled(struct kprobe *p)
+{
+	return p->flags & (KPROBE_FLAG_DISABLED | KPROBE_FLAG_GONE);
+}
+
+/* Is this kprobe really running optimized path ? */
+static inline int kprobe_optimized(struct kprobe *p)
+{
+	return p->flags & KPROBE_FLAG_OPTIMIZED;
+}
+
+/* Is this kprobe uses ftrace ? */
+static inline int kprobe_ftrace(struct kprobe *p)
+{
+	return p->flags & KPROBE_FLAG_FTRACE;
+}
+
 #ifdef CONFIG_KRETPROBES
+static inline void kretprobe_assert(struct kretprobe_instance *ri,
+	unsigned long orig_ret_address, unsigned long trampoline_address)
+{
+	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
+		printk(KERN_ERR
+			"kretprobe BUG!: Processing kretprobe %p @ %p\n",
+			ri->rp, ri->rp->kp.addr);
+		BUG();
+	}
+}
+
 extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
 				   struct pt_regs *regs);
 extern int arch_trampoline_kprobe(struct kprobe *p);
@@ -240,16 +251,6 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
 
 extern struct kretprobe_blackpoint kretprobe_blacklist[];
 
-static inline void kretprobe_assert(struct kretprobe_instance *ri,
-	unsigned long orig_ret_address, unsigned long trampoline_address)
-{
-	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
-		printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
-				ri->rp, ri->rp->kp.addr);
-		BUG();
-	}
-}
-
 #ifdef CONFIG_KPROBES_SANITY_TEST
 extern int init_test_probes(void);
 #else
@@ -340,7 +341,6 @@ struct kprobe *get_kprobe(void *addr);
 void kretprobe_hash_lock(struct task_struct *tsk,
 			 struct hlist_head **head, unsigned long *flags);
 void kretprobe_hash_unlock(struct task_struct *tsk, unsigned long *flags);
-struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
 
 /* kprobe_running() will just return the current_kprobe on this CPU */
 static inline struct kprobe *kprobe_running(void)
@@ -384,12 +384,60 @@ int enable_kprobe(struct kprobe *kp);
 
 void dump_kprobe(struct kprobe *kp);
 
+static inline int disable_jprobe(struct jprobe *jp)
+{
+	return disable_kprobe(&jp->kp);
+}
+
+static inline int enable_jprobe(struct jprobe *jp)
+{
+	return enable_kprobe(&jp->kp);
+}
+
+#ifdef CONFIG_KRETPROBES
+static inline int disable_kretprobe(struct kretprobe *rp)
+{
+	return disable_kprobe(&rp->kp);
+}
+
+static inline int enable_kretprobe(struct kretprobe *rp)
+{
+	return enable_kprobe(&rp->kp);
+}
+
+#else /* CONFIG_KRETPROBES */
+static inline int disable_kretprobe(struct kretprobe *rp)
+{
+	return -EINVAL;
+}
+static inline int enable_kretprobe(struct kretprobe *rp)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_KRETPROBES */
+
 #else /* !CONFIG_KPROBES: */
 
 static inline int kprobes_built_in(void)
 {
 	return 0;
 }
+static inline int kprobe_gone(struct kprobe *p)
+{
+	return 1;
+}
+static inline int kprobe_disabled(struct kprobe *p)
+{
+	return 1;
+}
+static inline int kprobe_optimized(struct kprobe *p)
+{
+	return 0;
+}
+static inline int kprobe_ftrace(struct kprobe *p)
+{
+	return 0;
+}
 static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 {
 	return 0;
@@ -458,22 +506,22 @@ static inline int enable_kprobe(struct kprobe *kp)
 {
 	return -ENOSYS;
 }
-#endif /* CONFIG_KPROBES */
-static inline int disable_kretprobe(struct kretprobe *rp)
+static inline int disable_jprobe(struct jprobe *jp)
 {
-	return disable_kprobe(&rp->kp);
+	return -ENOSYS;
 }
-static inline int enable_kretprobe(struct kretprobe *rp)
+static inline int enable_jprobe(struct jprobe *jp)
 {
-	return enable_kprobe(&rp->kp);
+	return -ENOSYS;
 }
-static inline int disable_jprobe(struct jprobe *jp)
+static inline int disable_kretprobe(struct kretprobe *rp)
 {
-	return disable_kprobe(&jp->kp);
+	return -ENOSYS;
 }
-static inline int enable_jprobe(struct jprobe *jp)
+static inline int enable_kretprobe(struct kretprobe *rp)
 {
-	return enable_kprobe(&jp->kp);
+	return -ENOSYS;
 }
+#endif /* CONFIG_KPROBES */
 
 #endif /* _LINUX_KPROBES_H */
-- 
1.7.11.7

Re: [PATCH] include/linux/kprobes.h: move all functions to their matched area

[Masami Hiramatsu]

(2014/02/05 14:27), Chen Gang wrote:
> For dummy functions, it is not a good idea to still use the input
> parameters (not a good idea to assume they are still effect).
> 
>  - let kprobe* static inline functions in CONFIG_KPROBES, dummy outside.
> 
>  - let (en/dis)able_jprobe() in CONFIG_KPROBES, dummy outside.
> 
>  - for kretprobe:
> 
>    - let kretprobe_assert() only in CONFIG_KRETPROBES (internal use).
> 
>    - remove kretprobe_inst_table_head() (cannot grep it in kernel wide).
> 
>    - for (en/dis)able_kretprobe():
> 
>        if CONFIG_KRETPROBES enabled, they use (en/dis)able_kprobe().
>        else if CONFIG_KPROBES enabled, return -EINVAL (not registered).
>        else, return -ENOSYS, just like (en/dis)able_kprobe() have done.

NAK. I don't increase complexity in this header file anymore,
unless we have any actual issue with sane usage.

Thank you,

> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  include/linux/kprobes.h | 136 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 92 insertions(+), 44 deletions(-)
> 
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 925eaf2..860313d 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -129,30 +129,6 @@ struct kprobe {
>  				   */
>  #define KPROBE_FLAG_FTRACE	8 /* probe is using ftrace */
>  
> -/* Has this kprobe gone ? */
> -static inline int kprobe_gone(struct kprobe *p)
> -{
> -	return p->flags & KPROBE_FLAG_GONE;
> -}
> -
> -/* Is this kprobe disabled ? */
> -static inline int kprobe_disabled(struct kprobe *p)
> -{
> -	return p->flags & (KPROBE_FLAG_DISABLED | KPROBE_FLAG_GONE);
> -}
> -
> -/* Is this kprobe really running optimized path ? */
> -static inline int kprobe_optimized(struct kprobe *p)
> -{
> -	return p->flags & KPROBE_FLAG_OPTIMIZED;
> -}
> -
> -/* Is this kprobe uses ftrace ? */
> -static inline int kprobe_ftrace(struct kprobe *p)
> -{
> -	return p->flags & KPROBE_FLAG_FTRACE;
> -}
> -
>  /*
>   * Special probe type that uses setjmp-longjmp type tricks to resume
>   * execution at a specified entry with a matching prototype corresponding
> @@ -223,7 +199,42 @@ static inline int kprobes_built_in(void)
>  	return 1;
>  }
>  
> +/* Has this kprobe gone ? */
> +static inline int kprobe_gone(struct kprobe *p)
> +{
> +	return p->flags & KPROBE_FLAG_GONE;
> +}
> +
> +/* Is this kprobe disabled ? */
> +static inline int kprobe_disabled(struct kprobe *p)
> +{
> +	return p->flags & (KPROBE_FLAG_DISABLED | KPROBE_FLAG_GONE);
> +}
> +
> +/* Is this kprobe really running optimized path ? */
> +static inline int kprobe_optimized(struct kprobe *p)
> +{
> +	return p->flags & KPROBE_FLAG_OPTIMIZED;
> +}
> +
> +/* Is this kprobe uses ftrace ? */
> +static inline int kprobe_ftrace(struct kprobe *p)
> +{
> +	return p->flags & KPROBE_FLAG_FTRACE;
> +}
> +
>  #ifdef CONFIG_KRETPROBES
> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
> +	unsigned long orig_ret_address, unsigned long trampoline_address)
> +{
> +	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
> +		printk(KERN_ERR
> +			"kretprobe BUG!: Processing kretprobe %p @ %p\n",
> +			ri->rp, ri->rp->kp.addr);
> +		BUG();
> +	}
> +}
> +
>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>  				   struct pt_regs *regs);
>  extern int arch_trampoline_kprobe(struct kprobe *p);
> @@ -240,16 +251,6 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>  
>  extern struct kretprobe_blackpoint kretprobe_blacklist[];
>  
> -static inline void kretprobe_assert(struct kretprobe_instance *ri,
> -	unsigned long orig_ret_address, unsigned long trampoline_address)
> -{
> -	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
> -		printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
> -				ri->rp, ri->rp->kp.addr);
> -		BUG();
> -	}
> -}
> -
>  #ifdef CONFIG_KPROBES_SANITY_TEST
>  extern int init_test_probes(void);
>  #else
> @@ -340,7 +341,6 @@ struct kprobe *get_kprobe(void *addr);
>  void kretprobe_hash_lock(struct task_struct *tsk,
>  			 struct hlist_head **head, unsigned long *flags);
>  void kretprobe_hash_unlock(struct task_struct *tsk, unsigned long *flags);
> -struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
>  
>  /* kprobe_running() will just return the current_kprobe on this CPU */
>  static inline struct kprobe *kprobe_running(void)
> @@ -384,12 +384,60 @@ int enable_kprobe(struct kprobe *kp);
>  
>  void dump_kprobe(struct kprobe *kp);
>  
> +static inline int disable_jprobe(struct jprobe *jp)
> +{
> +	return disable_kprobe(&jp->kp);
> +}
> +
> +static inline int enable_jprobe(struct jprobe *jp)
> +{
> +	return enable_kprobe(&jp->kp);
> +}
> +
> +#ifdef CONFIG_KRETPROBES
> +static inline int disable_kretprobe(struct kretprobe *rp)
> +{
> +	return disable_kprobe(&rp->kp);
> +}
> +
> +static inline int enable_kretprobe(struct kretprobe *rp)
> +{
> +	return enable_kprobe(&rp->kp);
> +}
> +
> +#else /* CONFIG_KRETPROBES */
> +static inline int disable_kretprobe(struct kretprobe *rp)
> +{
> +	return -EINVAL;
> +}
> +static inline int enable_kretprobe(struct kretprobe *rp)
> +{
> +	return -EINVAL;
> +}
> +#endif /* CONFIG_KRETPROBES */
> +
>  #else /* !CONFIG_KPROBES: */
>  
>  static inline int kprobes_built_in(void)
>  {
>  	return 0;
>  }
> +static inline int kprobe_gone(struct kprobe *p)
> +{
> +	return 1;
> +}
> +static inline int kprobe_disabled(struct kprobe *p)
> +{
> +	return 1;
> +}
> +static inline int kprobe_optimized(struct kprobe *p)
> +{
> +	return 0;
> +}
> +static inline int kprobe_ftrace(struct kprobe *p)
> +{
> +	return 0;
> +}
>  static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>  {
>  	return 0;
> @@ -458,22 +506,22 @@ static inline int enable_kprobe(struct kprobe *kp)
>  {
>  	return -ENOSYS;
>  }
> -#endif /* CONFIG_KPROBES */
> -static inline int disable_kretprobe(struct kretprobe *rp)
> +static inline int disable_jprobe(struct jprobe *jp)
>  {
> -	return disable_kprobe(&rp->kp);
> +	return -ENOSYS;
>  }
> -static inline int enable_kretprobe(struct kretprobe *rp)
> +static inline int enable_jprobe(struct jprobe *jp)
>  {
> -	return enable_kprobe(&rp->kp);
> +	return -ENOSYS;
>  }
> -static inline int disable_jprobe(struct jprobe *jp)
> +static inline int disable_kretprobe(struct kretprobe *rp)
>  {
> -	return disable_kprobe(&jp->kp);
> +	return -ENOSYS;
>  }
> -static inline int enable_jprobe(struct jprobe *jp)
> +static inline int enable_kretprobe(struct kretprobe *rp)
>  {
> -	return enable_kprobe(&jp->kp);
> +	return -ENOSYS;
>  }
> +#endif /* CONFIG_KPROBES */
>  
>  #endif /* _LINUX_KPROBES_H */
> 


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

Re: [PATCH] include/linux/kprobes.h: move all functions to their matched area

[Chen Gang]

On 02/05/2014 03:51 PM, Masami Hiramatsu wrote:
> (2014/02/05 14:27), Chen Gang wrote:
>> For dummy functions, it is not a good idea to still use the input
>> parameters (not a good idea to assume they are still effect).
>>
>>  - let kprobe* static inline functions in CONFIG_KPROBES, dummy outside.
>>
>>  - let (en/dis)able_jprobe() in CONFIG_KPROBES, dummy outside.
>>
>>  - for kretprobe:
>>
>>    - let kretprobe_assert() only in CONFIG_KRETPROBES (internal use).
>>
>>    - remove kretprobe_inst_table_head() (cannot grep it in kernel wide).
>>
>>    - for (en/dis)able_kretprobe():
>>
>>        if CONFIG_KRETPROBES enabled, they use (en/dis)able_kprobe().
>>        else if CONFIG_KPROBES enabled, return -EINVAL (not registered).
>>        else, return -ENOSYS, just like (en/dis)able_kprobe() have done.
> 
> NAK. I don't increase complexity in this header file anymore,
> unless we have any actual issue with sane usage.
> 
> Thank you,
> 

OK, I can understand, thanks. :-)


>>
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> ---
>>  include/linux/kprobes.h | 136 ++++++++++++++++++++++++++++++++----------------
>>  1 file changed, 92 insertions(+), 44 deletions(-)
>>
>> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
>> index 925eaf2..860313d 100644
>> --- a/include/linux/kprobes.h
>> +++ b/include/linux/kprobes.h
>> @@ -129,30 +129,6 @@ struct kprobe {
>>  				   */
>>  #define KPROBE_FLAG_FTRACE	8 /* probe is using ftrace */
>>  
>> -/* Has this kprobe gone ? */
>> -static inline int kprobe_gone(struct kprobe *p)
>> -{
>> -	return p->flags & KPROBE_FLAG_GONE;
>> -}
>> -
>> -/* Is this kprobe disabled ? */
>> -static inline int kprobe_disabled(struct kprobe *p)
>> -{
>> -	return p->flags & (KPROBE_FLAG_DISABLED | KPROBE_FLAG_GONE);
>> -}
>> -
>> -/* Is this kprobe really running optimized path ? */
>> -static inline int kprobe_optimized(struct kprobe *p)
>> -{
>> -	return p->flags & KPROBE_FLAG_OPTIMIZED;
>> -}
>> -
>> -/* Is this kprobe uses ftrace ? */
>> -static inline int kprobe_ftrace(struct kprobe *p)
>> -{
>> -	return p->flags & KPROBE_FLAG_FTRACE;
>> -}
>> -
>>  /*
>>   * Special probe type that uses setjmp-longjmp type tricks to resume
>>   * execution at a specified entry with a matching prototype corresponding
>> @@ -223,7 +199,42 @@ static inline int kprobes_built_in(void)
>>  	return 1;
>>  }
>>  
>> +/* Has this kprobe gone ? */
>> +static inline int kprobe_gone(struct kprobe *p)
>> +{
>> +	return p->flags & KPROBE_FLAG_GONE;
>> +}
>> +
>> +/* Is this kprobe disabled ? */
>> +static inline int kprobe_disabled(struct kprobe *p)
>> +{
>> +	return p->flags & (KPROBE_FLAG_DISABLED | KPROBE_FLAG_GONE);
>> +}
>> +
>> +/* Is this kprobe really running optimized path ? */
>> +static inline int kprobe_optimized(struct kprobe *p)
>> +{
>> +	return p->flags & KPROBE_FLAG_OPTIMIZED;
>> +}
>> +
>> +/* Is this kprobe uses ftrace ? */
>> +static inline int kprobe_ftrace(struct kprobe *p)
>> +{
>> +	return p->flags & KPROBE_FLAG_FTRACE;
>> +}
>> +
>>  #ifdef CONFIG_KRETPROBES
>> +static inline void kretprobe_assert(struct kretprobe_instance *ri,
>> +	unsigned long orig_ret_address, unsigned long trampoline_address)
>> +{
>> +	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>> +		printk(KERN_ERR
>> +			"kretprobe BUG!: Processing kretprobe %p @ %p\n",
>> +			ri->rp, ri->rp->kp.addr);
>> +		BUG();
>> +	}
>> +}
>> +
>>  extern void arch_prepare_kretprobe(struct kretprobe_instance *ri,
>>  				   struct pt_regs *regs);
>>  extern int arch_trampoline_kprobe(struct kprobe *p);
>> @@ -240,16 +251,6 @@ static inline int arch_trampoline_kprobe(struct kprobe *p)
>>  
>>  extern struct kretprobe_blackpoint kretprobe_blacklist[];
>>  
>> -static inline void kretprobe_assert(struct kretprobe_instance *ri,
>> -	unsigned long orig_ret_address, unsigned long trampoline_address)
>> -{
>> -	if (!orig_ret_address || (orig_ret_address == trampoline_address)) {
>> -		printk("kretprobe BUG!: Processing kretprobe %p @ %p\n",
>> -				ri->rp, ri->rp->kp.addr);
>> -		BUG();
>> -	}
>> -}
>> -
>>  #ifdef CONFIG_KPROBES_SANITY_TEST
>>  extern int init_test_probes(void);
>>  #else
>> @@ -340,7 +341,6 @@ struct kprobe *get_kprobe(void *addr);
>>  void kretprobe_hash_lock(struct task_struct *tsk,
>>  			 struct hlist_head **head, unsigned long *flags);
>>  void kretprobe_hash_unlock(struct task_struct *tsk, unsigned long *flags);
>> -struct hlist_head * kretprobe_inst_table_head(struct task_struct *tsk);
>>  
>>  /* kprobe_running() will just return the current_kprobe on this CPU */
>>  static inline struct kprobe *kprobe_running(void)
>> @@ -384,12 +384,60 @@ int enable_kprobe(struct kprobe *kp);
>>  
>>  void dump_kprobe(struct kprobe *kp);
>>  
>> +static inline int disable_jprobe(struct jprobe *jp)
>> +{
>> +	return disable_kprobe(&jp->kp);
>> +}
>> +
>> +static inline int enable_jprobe(struct jprobe *jp)
>> +{
>> +	return enable_kprobe(&jp->kp);
>> +}
>> +
>> +#ifdef CONFIG_KRETPROBES
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return disable_kprobe(&rp->kp);
>> +}
>> +
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return enable_kprobe(&rp->kp);
>> +}
>> +
>> +#else /* CONFIG_KRETPROBES */
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return -EINVAL;
>> +}
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>> +{
>> +	return -EINVAL;
>> +}
>> +#endif /* CONFIG_KRETPROBES */
>> +
>>  #else /* !CONFIG_KPROBES: */
>>  
>>  static inline int kprobes_built_in(void)
>>  {
>>  	return 0;
>>  }
>> +static inline int kprobe_gone(struct kprobe *p)
>> +{
>> +	return 1;
>> +}
>> +static inline int kprobe_disabled(struct kprobe *p)
>> +{
>> +	return 1;
>> +}
>> +static inline int kprobe_optimized(struct kprobe *p)
>> +{
>> +	return 0;
>> +}
>> +static inline int kprobe_ftrace(struct kprobe *p)
>> +{
>> +	return 0;
>> +}
>>  static inline int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
>>  {
>>  	return 0;
>> @@ -458,22 +506,22 @@ static inline int enable_kprobe(struct kprobe *kp)
>>  {
>>  	return -ENOSYS;
>>  }
>> -#endif /* CONFIG_KPROBES */
>> -static inline int disable_kretprobe(struct kretprobe *rp)
>> +static inline int disable_jprobe(struct jprobe *jp)
>>  {
>> -	return disable_kprobe(&rp->kp);
>> +	return -ENOSYS;
>>  }
>> -static inline int enable_kretprobe(struct kretprobe *rp)
>> +static inline int enable_jprobe(struct jprobe *jp)
>>  {
>> -	return enable_kprobe(&rp->kp);
>> +	return -ENOSYS;
>>  }
>> -static inline int disable_jprobe(struct jprobe *jp)
>> +static inline int disable_kretprobe(struct kretprobe *rp)
>>  {
>> -	return disable_kprobe(&jp->kp);
>> +	return -ENOSYS;
>>  }
>> -static inline int enable_jprobe(struct jprobe *jp)
>> +static inline int enable_kretprobe(struct kretprobe *rp)
>>  {
>> -	return enable_kprobe(&jp->kp);
>> +	return -ENOSYS;
>>  }
>> +#endif /* CONFIG_KPROBES */
>>  
>>  #endif /* _LINUX_KPROBES_H */
>>
> 
> 


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed