[RFC PATCH tip/master 0/3] kprobes: tracing: kretprobe_instance dynamic allocation

[RFC PATCH tip/master 0/3] kprobes: tracing: kretprobe_instance dynamic allocation

[Masami Hiramatsu]

Here is a correction of patches to introduce kretprobe_instance
dynamic allocation for avoiding kretprobe silently miss-hits.

Original issue was reported by Lukasz on linux-trace ml.
 https://www.spinics.net/lists/linux-trace/msg00448.html

Also Alban is working on kprobe-tracer side because of
iovisor's issue.
 https://www.spinics.net/lists/netdev/msg427149.html
(Note that this series is independently applicable, no conflict)

This series is a kind of complementary patches for
Alban's patch. So I think both of them are needed.

 [1/3]: Add kretprobe's miss-hit counter to miss-hit
        column on kprobe_profile. This helps user to
        see what happened.
 [2/3]: Introduce kretprobe_instance dynamic allocation.
        This will help user not to miss the ret probes
        even it has low number of maxactive.
 [3/3]: Set maximum limitation for pre-allocated maxactive.
        This can avoid miss configuration of struct kretprobe.

The downside of this patch is, dynamic allocation will
involve memory allocation, which sometimes traced by
kprobes. In that case those nested kprobes are missed.
To avoid this kind of miss-hit, user may need to make
maxactive enough large when registering kretprobes.

However, in other case, this can reduce the possibility
of miss-hit of kretprobes. Since the maxactive increased
automatically, user will not need to retry tracing with
larger maxactive.

Alban, you can reuse KRETPROBE_MAXACTIVE_ALLOC for checking
upper limiation in trace_kprobe.c too.

Thank you,

---

Masami Hiramatsu (3):
      trace: kprobes: Show sum of probe/retprobe nmissed count
      kprobes: Allocate kretprobe instance if its free list is empty
      kprobes: Limit kretprobe maximum instances


 Documentation/kprobes.txt   |   25 +++++++++++++++---------
 include/linux/kprobes.h     |    2 ++
 kernel/kprobes.c            |   44 +++++++++++++++++++++++++++++++------------
 kernel/trace/trace_kprobe.c |    2 +-
 4 files changed, 50 insertions(+), 23 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[RFC PATCH tip/master 1/3] trace: kprobes: Show sum of probe/retprobe nmissed count

[Masami Hiramatsu]

Show sum of probe and retprobe nmissed count in
kprobe_profile, since retprobe can be missed even
if the kprobe itself succeeeded.
This explains user why their return probe didn't hit
sometimes.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/trace/trace_kprobe.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index 013f4e7..bbdc3de 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -896,7 +896,7 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
 	seq_printf(m, "  %-44s %15lu %15lu\n",
 		   trace_event_name(&tk->tp.call),
 		   trace_kprobe_nhit(tk),
-		   tk->rp.kp.nmissed);
+		   tk->rp.kp.nmissed + tk->rp.nmissed);
 
 	return 0;
 }

[RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Masami Hiramatsu]

Try to allocate kretprobe instance by GFP_ATOMIC if kretprobe's
free list is empty. This can prevent kretprobe miss-hit on the
function which can be heavily invoked and slept inside (like
locking syscall entries.)

NOTE: This may easily cause nested kprobe call which will be
just skipped (but nmissed count is incremented), if someone
probe functions on the memory allocation path.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/kprobes.txt |   25 +++++++++++++++----------
 include/linux/kprobes.h   |    2 ++
 kernel/kprobes.c          |   41 +++++++++++++++++++++++++++++------------
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/Documentation/kprobes.txt b/Documentation/kprobes.txt
index 1f6d45a..2de6533 100644
--- a/Documentation/kprobes.txt
+++ b/Documentation/kprobes.txt
@@ -131,11 +131,13 @@ kretprobe, then sets the saved instruction pointer to the saved return
 address, and that's where execution resumes upon return from the trap.
 
 While the probed function is executing, its return address is
-stored in an object of type kretprobe_instance.  Before calling
-register_kretprobe(), the user sets the maxactive field of the
-kretprobe struct to specify how many instances of the specified
-function can be probed simultaneously.  register_kretprobe()
-pre-allocates the indicated number of kretprobe_instance objects.
+stored in an object of type kretprobe_instance. Usually, this
+kretprobe_instance will be allocated dynamically.
+Since the dynamic allocation can cause mis-hit of other probes
+on memory allocation path, user can set maxactive field for pooling
+pre-allocated instances before calling register_kretprobe().
+This maxactive indicates how many instances of the specified
+function can be probed simultaneously.
 
 For example, if the function is non-recursive and is called with a
 spinlock held, maxactive = 1 should be enough.  If the function is
@@ -144,11 +146,14 @@ or preemption), NR_CPUS should be enough.  If maxactive <= 0, it is
 set to a default value.  If CONFIG_PREEMPT is enabled, the default
 is max(10, 2*NR_CPUS).  Otherwise, the default is NR_CPUS.
 
-It's not a disaster if you set maxactive too low; you'll just miss
-some probes.  In the kretprobe struct, the nmissed field is set to
-zero when the return probe is registered, and is incremented every
-time the probed function is entered but there is no kretprobe_instance
-object available for establishing the return probe.
+It's not a disaster if you set maxactive too low; you'll just see
+some probes on memory allocation path missed if it exists.
+
+In the kretprobe struct, the nmissed field is set to zero when the
+return probe is registered, and is incremented every time the probed
+function is entered but there is no kretprobe_instance object available
+and it fails to allocate new one, or hit the upper limit of maxactive
+(KRETPROBE_MAXACTIVE_ALLOC, currently it is 4096.)
 
 1.3.2 Kretprobe entry-handler
 
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 47e4da5..8064e14 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -192,6 +192,8 @@ struct kretprobe {
 	struct hlist_head free_instances;
 	raw_spinlock_t lock;
 };
+/* Upper limit of maxactive for dynamic allocation */
+#define KRETPROBE_MAXACTIVE_ALLOC 4096
 
 struct kretprobe_instance {
 	struct hlist_node hlist;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index d733479..75c5390 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -57,7 +57,6 @@
 #define KPROBE_HASH_BITS 6
 #define KPROBE_TABLE_SIZE (1 << KPROBE_HASH_BITS)
 
-
 /*
  * Some oddball architectures like 64bit powerpc have function descriptors
  * so this must be overridable.
@@ -1824,6 +1823,30 @@ void unregister_jprobes(struct jprobe **jps, int num)
 EXPORT_SYMBOL_GPL(unregister_jprobes);
 
 #ifdef CONFIG_KRETPROBES
+
+/* Try to use free instance first, if failed, try to allocate new instance */
+struct kretprobe_instance *kretprobe_alloc_instance(struct kretprobe *rp)
+{
+	struct kretprobe_instance *ri = NULL;
+	unsigned long flags = 0;
+
+	raw_spin_lock_irqsave(&rp->lock, flags);
+	if (!hlist_empty(&rp->free_instances)) {
+		ri = hlist_entry(rp->free_instances.first,
+				struct kretprobe_instance, hlist);
+		hlist_del(&ri->hlist);
+	}
+	raw_spin_unlock_irqrestore(&rp->lock, flags);
+
+	/* Populate max active instance if possible */
+	if (!ri && rp->maxactive < KRETPROBE_MAXACTIVE_ALLOC) {
+		ri = kmalloc(sizeof(*ri) + rp->data_size, GFP_ATOMIC);
+		if (ri)
+			rp->maxactive++;
+	}
+
+	return ri;
+}
 /*
  * This kprobe pre_handler is registered with every kretprobe. When probe
  * hits it will set up the return probe.
@@ -1846,14 +1869,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 	}
 
 	/* TODO: consider to only swap the RA after the last pre_handler fired */
-	hash = hash_ptr(current, KPROBE_HASH_BITS);
-	raw_spin_lock_irqsave(&rp->lock, flags);
-	if (!hlist_empty(&rp->free_instances)) {
-		ri = hlist_entry(rp->free_instances.first,
-				struct kretprobe_instance, hlist);
-		hlist_del(&ri->hlist);
-		raw_spin_unlock_irqrestore(&rp->lock, flags);
-
+	ri = kretprobe_alloc_instance(rp);
+	if (ri) {
 		ri->rp = rp;
 		ri->task = current;
 
@@ -1868,13 +1885,13 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 
 		/* XXX(hch): why is there no hlist_move_head? */
 		INIT_HLIST_NODE(&ri->hlist);
+		hash = hash_ptr(current, KPROBE_HASH_BITS);
 		kretprobe_table_lock(hash, &flags);
 		hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
 		kretprobe_table_unlock(hash, &flags);
-	} else {
+	} else
 		rp->nmissed++;
-		raw_spin_unlock_irqrestore(&rp->lock, flags);
-	}
+
 	return 0;
 }
 NOKPROBE_SYMBOL(pre_handler_kretprobe);

[RFC PATCH tip/master 3/3] kprobes: Limit kretprobe maximum instances

[Masami Hiramatsu]

Limit kretprobe maximum instance up to MAXACTIVE_ALLOC.
Without this limit, kretprobe user can specify huge number
(e.g. forget to zero-fill struct kretprobe) to maxactive
and may cause out-of-memory.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 75c5390..f1bebcf 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1942,6 +1942,9 @@ int register_kretprobe(struct kretprobe *rp)
 	rp->kp.break_handler = NULL;
 
 	/* Pre-allocate memory for max kretprobe instances */
+	if (rp->maxactive > KRETPROBE_MAXACTIVE_ALLOC)
+		return -E2BIG;
+
 	if (rp->maxactive <= 0) {
 #ifdef CONFIG_PREEMPT
 		rp->maxactive = max_t(unsigned int, 10, 2*num_possible_cpus());

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> @@ -1824,6 +1823,30 @@ void unregister_jprobes(struct jprobe **jps, int num)
>  EXPORT_SYMBOL_GPL(unregister_jprobes);
>  
>  #ifdef CONFIG_KRETPROBES
> +
> +/* Try to use free instance first, if failed, try to allocate new instance */
> +struct kretprobe_instance *kretprobe_alloc_instance(struct kretprobe *rp)
> +{
> +	struct kretprobe_instance *ri = NULL;
> +	unsigned long flags = 0;
> +
> +	raw_spin_lock_irqsave(&rp->lock, flags);
> +	if (!hlist_empty(&rp->free_instances)) {
> +		ri = hlist_entry(rp->free_instances.first,
> +				struct kretprobe_instance, hlist);
> +		hlist_del(&ri->hlist);
> +	}
> +	raw_spin_unlock_irqrestore(&rp->lock, flags);
> +
> +	/* Populate max active instance if possible */
> +	if (!ri && rp->maxactive < KRETPROBE_MAXACTIVE_ALLOC) {
> +		ri = kmalloc(sizeof(*ri) + rp->data_size, GFP_ATOMIC);
> +		if (ri)
> +			rp->maxactive++;
> +	}
> +
> +	return ri;
> +}
>  /*
>   * This kprobe pre_handler is registered with every kretprobe. When probe
>   * hits it will set up the return probe.
> @@ -1846,14 +1869,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
>  	}
>  
>  	/* TODO: consider to only swap the RA after the last pre_handler fired */
> -	hash = hash_ptr(current, KPROBE_HASH_BITS);
> -	raw_spin_lock_irqsave(&rp->lock, flags);
> -	if (!hlist_empty(&rp->free_instances)) {
> -		ri = hlist_entry(rp->free_instances.first,
> -				struct kretprobe_instance, hlist);
> -		hlist_del(&ri->hlist);
> -		raw_spin_unlock_irqrestore(&rp->lock, flags);
> -
> +	ri = kretprobe_alloc_instance(rp);
> +	if (ri) {
>  		ri->rp = rp;
>  		ri->task = current;
>  
> @@ -1868,13 +1885,13 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
>  
>  		/* XXX(hch): why is there no hlist_move_head? */
>  		INIT_HLIST_NODE(&ri->hlist);
> +		hash = hash_ptr(current, KPROBE_HASH_BITS);
>  		kretprobe_table_lock(hash, &flags);
>  		hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
>  		kretprobe_table_unlock(hash, &flags);
> -	} else {
> +	} else
>  		rp->nmissed++;
> -		raw_spin_unlock_irqrestore(&rp->lock, flags);
> -	}
> +
>  	return 0;
>  }
>  NOKPROBE_SYMBOL(pre_handler_kretprobe);

So this is something I missed while the original code was merged, but the concept 
looks a bit weird: why do we do any "allocation" while a handler is executing?

That's fundamentally fragile. What's the maximum number of parallel 
'kretprobe_instance' required per kretprobe - one per CPU?

If so then we should preallocate all of them when they are installed and not do 
any alloc/free dance when executing them.

This will also speed them up, and increase robustness all around.

Thanks,

	Ingo

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Masami Hiramatsu]

On Wed, 29 Mar 2017 08:30:05 +0200
Ingo Molnar <mingo@kernel.org> wrote:
> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > @@ -1824,6 +1823,30 @@ void unregister_jprobes(struct jprobe **jps, int num)
> >  EXPORT_SYMBOL_GPL(unregister_jprobes);
> >  
> >  #ifdef CONFIG_KRETPROBES
> > +
> > +/* Try to use free instance first, if failed, try to allocate new instance */
> > +struct kretprobe_instance *kretprobe_alloc_instance(struct kretprobe *rp)
> > +{
> > +	struct kretprobe_instance *ri = NULL;
> > +	unsigned long flags = 0;
> > +
> > +	raw_spin_lock_irqsave(&rp->lock, flags);
> > +	if (!hlist_empty(&rp->free_instances)) {
> > +		ri = hlist_entry(rp->free_instances.first,
> > +				struct kretprobe_instance, hlist);
> > +		hlist_del(&ri->hlist);
> > +	}
> > +	raw_spin_unlock_irqrestore(&rp->lock, flags);
> > +
> > +	/* Populate max active instance if possible */
> > +	if (!ri && rp->maxactive < KRETPROBE_MAXACTIVE_ALLOC) {
> > +		ri = kmalloc(sizeof(*ri) + rp->data_size, GFP_ATOMIC);
> > +		if (ri)
> > +			rp->maxactive++;
> > +	}
> > +
> > +	return ri;
> > +}
> >  /*
> >   * This kprobe pre_handler is registered with every kretprobe. When probe
> >   * hits it will set up the return probe.
> > @@ -1846,14 +1869,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> >  	}
> >  
> >  	/* TODO: consider to only swap the RA after the last pre_handler fired */
> > -	hash = hash_ptr(current, KPROBE_HASH_BITS);
> > -	raw_spin_lock_irqsave(&rp->lock, flags);
> > -	if (!hlist_empty(&rp->free_instances)) {
> > -		ri = hlist_entry(rp->free_instances.first,
> > -				struct kretprobe_instance, hlist);
> > -		hlist_del(&ri->hlist);
> > -		raw_spin_unlock_irqrestore(&rp->lock, flags);
> > -
> > +	ri = kretprobe_alloc_instance(rp);
> > +	if (ri) {
> >  		ri->rp = rp;
> >  		ri->task = current;
> >  
> > @@ -1868,13 +1885,13 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> >  
> >  		/* XXX(hch): why is there no hlist_move_head? */
> >  		INIT_HLIST_NODE(&ri->hlist);
> > +		hash = hash_ptr(current, KPROBE_HASH_BITS);
> >  		kretprobe_table_lock(hash, &flags);
> >  		hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
> >  		kretprobe_table_unlock(hash, &flags);
> > -	} else {
> > +	} else
> >  		rp->nmissed++;
> > -		raw_spin_unlock_irqrestore(&rp->lock, flags);
> > -	}
> > +
> >  	return 0;
> >  }
> >  NOKPROBE_SYMBOL(pre_handler_kretprobe);
> 
> So this is something I missed while the original code was merged, but the concept 
> looks a bit weird: why do we do any "allocation" while a handler is executing?
> 
> That's fundamentally fragile. What's the maximum number of parallel 
> 'kretprobe_instance' required per kretprobe - one per CPU?

It depends on the place where we put the probe. If the probed function will be
blocked (yield to other tasks), then we need a same number of threads on
the system which can invoke the function. So, ultimately, it is same
as function_graph tracer, we need it for each thread.

> 
> If so then we should preallocate all of them when they are installed and not do 
> any alloc/free dance when executing them.
> 
> This will also speed them up, and increase robustness all around.

I see, kretprobe already do that, and I keep it on the code.

By default, kretprobe will allocate NR_CPU of kretprobe_instance for each
kretprobe. For usual usecase (deeper inside functions in kernel) that is OK.
However, as Lukasz reported, for the function near the syscall entry, it may
require more instances. In that case, kretprobe user needs to increase
maxactive before registering kretprobes, which will be done by Alban's patch.

However, the next question is, how many instances are actually needed.
User may have to do trial & error loop to find that. For professional users,
they will do that, but for the light users, they may not want to do that.

I'm also considering to provide a "knob" of disabing this dynamic allocation
feature on debugfs, which will help users who would like to avoid memory
allocation on kretprobe.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC PATCH tip/master 0/3] kprobes: tracing: kretprobe_instance dynamic allocation

[Frank Ch. Eigler]

mhiramat wrote:

> Here is a correction of patches to introduce kretprobe_instance
> dynamic allocation for avoiding kretprobe silently miss-hits.
> [...]

Thanks, this looks automatically useful also to systemtap users.

- FChE

Re: [RFC PATCH tip/master 0/3] kprobes: tracing: kretprobe_instance dynamic allocation

[Frank Ch. Eigler]

mhiramat wrote:

> Here is a correction of patches to introduce kretprobe_instance
> dynamic allocation for avoiding kretprobe silently miss-hits.
> [...]

Thanks, this looks automatically useful also to systemtap users.

- FChE

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Josh Stone]

On 03/29/2017 01:25 AM, Masami Hiramatsu wrote:
> On Wed, 29 Mar 2017 08:30:05 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
>>
>> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>
>>> @@ -1824,6 +1823,30 @@ void unregister_jprobes(struct jprobe **jps, int num)
>>>  EXPORT_SYMBOL_GPL(unregister_jprobes);
>>>  
>>>  #ifdef CONFIG_KRETPROBES
>>> +
>>> +/* Try to use free instance first, if failed, try to allocate new instance */
>>> +struct kretprobe_instance *kretprobe_alloc_instance(struct kretprobe *rp)
>>> +{
>>> +	struct kretprobe_instance *ri = NULL;
>>> +	unsigned long flags = 0;
>>> +
>>> +	raw_spin_lock_irqsave(&rp->lock, flags);
>>> +	if (!hlist_empty(&rp->free_instances)) {
>>> +		ri = hlist_entry(rp->free_instances.first,
>>> +				struct kretprobe_instance, hlist);
>>> +		hlist_del(&ri->hlist);
>>> +	}
>>> +	raw_spin_unlock_irqrestore(&rp->lock, flags);
>>> +
>>> +	/* Populate max active instance if possible */
>>> +	if (!ri && rp->maxactive < KRETPROBE_MAXACTIVE_ALLOC) {
>>> +		ri = kmalloc(sizeof(*ri) + rp->data_size, GFP_ATOMIC);
>>> +		if (ri)
>>> +			rp->maxactive++;
>>> +	}
>>> +
>>> +	return ri;
>>> +}
>>>  /*
>>>   * This kprobe pre_handler is registered with every kretprobe. When probe
>>>   * hits it will set up the return probe.
>>> @@ -1846,14 +1869,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
>>>  	}
>>>  
>>>  	/* TODO: consider to only swap the RA after the last pre_handler fired */
>>> -	hash = hash_ptr(current, KPROBE_HASH_BITS);
>>> -	raw_spin_lock_irqsave(&rp->lock, flags);
>>> -	if (!hlist_empty(&rp->free_instances)) {
>>> -		ri = hlist_entry(rp->free_instances.first,
>>> -				struct kretprobe_instance, hlist);
>>> -		hlist_del(&ri->hlist);
>>> -		raw_spin_unlock_irqrestore(&rp->lock, flags);
>>> -
>>> +	ri = kretprobe_alloc_instance(rp);
>>> +	if (ri) {
>>>  		ri->rp = rp;
>>>  		ri->task = current;
>>>  
>>> @@ -1868,13 +1885,13 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
>>>  
>>>  		/* XXX(hch): why is there no hlist_move_head? */
>>>  		INIT_HLIST_NODE(&ri->hlist);
>>> +		hash = hash_ptr(current, KPROBE_HASH_BITS);
>>>  		kretprobe_table_lock(hash, &flags);
>>>  		hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
>>>  		kretprobe_table_unlock(hash, &flags);
>>> -	} else {
>>> +	} else
>>>  		rp->nmissed++;
>>> -		raw_spin_unlock_irqrestore(&rp->lock, flags);
>>> -	}
>>> +
>>>  	return 0;
>>>  }
>>>  NOKPROBE_SYMBOL(pre_handler_kretprobe);
>>
>> So this is something I missed while the original code was merged, but the concept 
>> looks a bit weird: why do we do any "allocation" while a handler is executing?
>>
>> That's fundamentally fragile. What's the maximum number of parallel 
>> 'kretprobe_instance' required per kretprobe - one per CPU?
> 
> It depends on the place where we put the probe. If the probed function will be
> blocked (yield to other tasks), then we need a same number of threads on
> the system which can invoke the function. So, ultimately, it is same
> as function_graph tracer, we need it for each thread.

Isn't it also possible that the function may be reentrant?  Whether by
plain recursion or an interrupt call, this leads to multiple live
instances even for a given thread.

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Masami Hiramatsu]

On Wed, 29 Mar 2017 10:18:48 -0700
Josh Stone <jistone@redhat.com> wrote:

> On 03/29/2017 01:25 AM, Masami Hiramatsu wrote:
> > On Wed, 29 Mar 2017 08:30:05 +0200
> > Ingo Molnar <mingo@kernel.org> wrote:
> >>
> >> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >>
> >>> @@ -1824,6 +1823,30 @@ void unregister_jprobes(struct jprobe **jps, int num)
> >>>  EXPORT_SYMBOL_GPL(unregister_jprobes);
> >>>  
> >>>  #ifdef CONFIG_KRETPROBES
> >>> +
> >>> +/* Try to use free instance first, if failed, try to allocate new instance */
> >>> +struct kretprobe_instance *kretprobe_alloc_instance(struct kretprobe *rp)
> >>> +{
> >>> +	struct kretprobe_instance *ri = NULL;
> >>> +	unsigned long flags = 0;
> >>> +
> >>> +	raw_spin_lock_irqsave(&rp->lock, flags);
> >>> +	if (!hlist_empty(&rp->free_instances)) {
> >>> +		ri = hlist_entry(rp->free_instances.first,
> >>> +				struct kretprobe_instance, hlist);
> >>> +		hlist_del(&ri->hlist);
> >>> +	}
> >>> +	raw_spin_unlock_irqrestore(&rp->lock, flags);
> >>> +
> >>> +	/* Populate max active instance if possible */
> >>> +	if (!ri && rp->maxactive < KRETPROBE_MAXACTIVE_ALLOC) {
> >>> +		ri = kmalloc(sizeof(*ri) + rp->data_size, GFP_ATOMIC);
> >>> +		if (ri)
> >>> +			rp->maxactive++;
> >>> +	}
> >>> +
> >>> +	return ri;
> >>> +}
> >>>  /*
> >>>   * This kprobe pre_handler is registered with every kretprobe. When probe
> >>>   * hits it will set up the return probe.
> >>> @@ -1846,14 +1869,8 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> >>>  	}
> >>>  
> >>>  	/* TODO: consider to only swap the RA after the last pre_handler fired */
> >>> -	hash = hash_ptr(current, KPROBE_HASH_BITS);
> >>> -	raw_spin_lock_irqsave(&rp->lock, flags);
> >>> -	if (!hlist_empty(&rp->free_instances)) {
> >>> -		ri = hlist_entry(rp->free_instances.first,
> >>> -				struct kretprobe_instance, hlist);
> >>> -		hlist_del(&ri->hlist);
> >>> -		raw_spin_unlock_irqrestore(&rp->lock, flags);
> >>> -
> >>> +	ri = kretprobe_alloc_instance(rp);
> >>> +	if (ri) {
> >>>  		ri->rp = rp;
> >>>  		ri->task = current;
> >>>  
> >>> @@ -1868,13 +1885,13 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
> >>>  
> >>>  		/* XXX(hch): why is there no hlist_move_head? */
> >>>  		INIT_HLIST_NODE(&ri->hlist);
> >>> +		hash = hash_ptr(current, KPROBE_HASH_BITS);
> >>>  		kretprobe_table_lock(hash, &flags);
> >>>  		hlist_add_head(&ri->hlist, &kretprobe_inst_table[hash]);
> >>>  		kretprobe_table_unlock(hash, &flags);
> >>> -	} else {
> >>> +	} else
> >>>  		rp->nmissed++;
> >>> -		raw_spin_unlock_irqrestore(&rp->lock, flags);
> >>> -	}
> >>> +
> >>>  	return 0;
> >>>  }
> >>>  NOKPROBE_SYMBOL(pre_handler_kretprobe);
> >>
> >> So this is something I missed while the original code was merged, but the concept 
> >> looks a bit weird: why do we do any "allocation" while a handler is executing?
> >>
> >> That's fundamentally fragile. What's the maximum number of parallel 
> >> 'kretprobe_instance' required per kretprobe - one per CPU?
> > 
> > It depends on the place where we put the probe. If the probed function will be
> > blocked (yield to other tasks), then we need a same number of threads on
> > the system which can invoke the function. So, ultimately, it is same
> > as function_graph tracer, we need it for each thread.
> 
> Isn't it also possible that the function may be reentrant?  Whether by
> plain recursion or an interrupt call, this leads to multiple live
> instances even for a given thread.

Yes, that's another possible case, but I don't think that's so serious in kernel
because we have very limited kernel stack, which means the recursion may not
so deep.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > So this is something I missed while the original code was merged, but the concept 
> > looks a bit weird: why do we do any "allocation" while a handler is executing?
> > 
> > That's fundamentally fragile. What's the maximum number of parallel 
> > 'kretprobe_instance' required per kretprobe - one per CPU?
> 
> It depends on the place where we put the probe. If the probed function will be
> blocked (yield to other tasks), then we need a same number of threads on
> the system which can invoke the function. So, ultimately, it is same
> as function_graph tracer, we need it for each thread.

So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe 
nesting allowed). There's just no way in hell we should be calling any complex 
kernel function from kernel probes!

I mean, think about it, a kretprobe can be installed in a lot of places, and now 
we want to call get_free_pages() from it?? This would add a massive amount of 
fragility.

Instrumentation must be _simple_, every patch that adds more complexity to the 
most fundamental code path of it should raise a red flag ...

So let's make this more robust, ok?

Thanks,

	Ingo

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > So this is something I missed while the original code was merged, but the concept 
> > looks a bit weird: why do we do any "allocation" while a handler is executing?
> > 
> > That's fundamentally fragile. What's the maximum number of parallel 
> > 'kretprobe_instance' required per kretprobe - one per CPU?
> 
> It depends on the place where we put the probe. If the probed function will be
> blocked (yield to other tasks), then we need a same number of threads on
> the system which can invoke the function. So, ultimately, it is same
> as function_graph tracer, we need it for each thread.

So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe 
nesting allowed). There's just no way in hell we should be calling any complex 
kernel function from kernel probes!

I mean, think about it, a kretprobe can be installed in a lot of places, and now 
we want to call get_free_pages() from it?? This would add a massive amount of 
fragility.

Instrumentation must be _simple_, every patch that adds more complexity to the 
most fundamental code path of it should raise a red flag ...

So let's make this more robust, ok?

Thanks,

	Ingo

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Masami Hiramatsu]

On Thu, 30 Mar 2017 08:53:32 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > So this is something I missed while the original code was merged, but the concept 
> > > looks a bit weird: why do we do any "allocation" while a handler is executing?
> > > 
> > > That's fundamentally fragile. What's the maximum number of parallel 
> > > 'kretprobe_instance' required per kretprobe - one per CPU?
> > 
> > It depends on the place where we put the probe. If the probed function will be
> > blocked (yield to other tasks), then we need a same number of threads on
> > the system which can invoke the function. So, ultimately, it is same
> > as function_graph tracer, we need it for each thread.
> 
> So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe 
> nesting allowed).

No, that is possible to put several kretprobes on same thread, e.g.
the func1() is called from func2(), user can put kretprobes for each
function at same time.
So the possible solution is to allocate new return-stack for each task_struct,
and that is what the function-graph tracer did.

Anyway, I'm considering to integrate kretprobe_instance with the ret_stack.
It will increase memory usage for kretprobes, but can provide safer way
to allocate kretprobe_instance.

> There's just no way in hell we should be calling any complex 
> kernel function from kernel probes!

OK, so let's drop this, since it may easily cause deadlock... 


> I mean, think about it, a kretprobe can be installed in a lot of places, and now 
> we want to call get_free_pages() from it?? This would add a massive amount of 
> fragility.

I thought it was safe because GFP_ATOMIC is safe at interrupt handler.

> Instrumentation must be _simple_, every patch that adds more complexity to the 
> most fundamental code path of it should raise a red flag ...
> 
> So let's make this more robust, ok?

Yeah, in that case, I think Alban's patch is enough at this point since
it gives user to tune their kretprobe events not to be missed.

Thank you,

> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Alban Crequy]

On Thu, Mar 30, 2017 at 8:53 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
>> > So this is something I missed while the original code was merged, but the concept
>> > looks a bit weird: why do we do any "allocation" while a handler is executing?
>> >
>> > That's fundamentally fragile. What's the maximum number of parallel
>> > 'kretprobe_instance' required per kretprobe - one per CPU?
>>
>> It depends on the place where we put the probe. If the probed function will be
>> blocked (yield to other tasks), then we need a same number of threads on
>> the system which can invoke the function. So, ultimately, it is same
>> as function_graph tracer, we need it for each thread.
>
> So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe
> nesting allowed). There's just no way in hell we should be calling any complex
> kernel function from kernel probes!

Some kprobes are called from an interruption context. We have a kprobe
on tcp_set_state() and this is sometimes called when the network card
receives a packet.

> I mean, think about it, a kretprobe can be installed in a lot of places, and now
> we want to call get_free_pages() from it?? This would add a massive amount of
> fragility.
>
> Instrumentation must be _simple_, every patch that adds more complexity to the
> most fundamental code path of it should raise a red flag ...
>
> So let's make this more robust, ok?
>
> Thanks,
>
>         Ingo

Thanks,
Alban

Re: [RFC PATCH tip/master 1/3] trace: kprobes: Show sum of probe/retprobe nmissed count

[Alban Crequy]

On Wed, Mar 29, 2017 at 7:22 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote:
> Show sum of probe and retprobe nmissed count in
> kprobe_profile, since retprobe can be missed even
> if the kprobe itself succeeeded.
> This explains user why their return probe didn't hit
> sometimes.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

I tested this patch with my kretprobe on "inet_csk_accept" when there
are many processes waiting in the accept() syscall. I can now
successfully see the nmissed counter in
/sys/kernel/debug/tracing/kprobe_profile being incremented when the
kretprobe is missed.

Tested-by: Alban Crequy <alban@kinvolk.io>


> ---
>  kernel/trace/trace_kprobe.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> index 013f4e7..bbdc3de 100644
> --- a/kernel/trace/trace_kprobe.c
> +++ b/kernel/trace/trace_kprobe.c
> @@ -896,7 +896,7 @@ static int probes_profile_seq_show(struct seq_file *m, void *v)
>         seq_printf(m, "  %-44s %15lu %15lu\n",
>                    trace_event_name(&tk->tp.call),
>                    trace_kprobe_nhit(tk),
> -                  tk->rp.kp.nmissed);
> +                  tk->rp.kp.nmissed + tk->rp.nmissed);
>
>         return 0;
>  }
>

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 30 Mar 2017 08:53:32 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > > So this is something I missed while the original code was merged, but the concept 
> > > > looks a bit weird: why do we do any "allocation" while a handler is executing?
> > > > 
> > > > That's fundamentally fragile. What's the maximum number of parallel 
> > > > 'kretprobe_instance' required per kretprobe - one per CPU?
> > > 
> > > It depends on the place where we put the probe. If the probed function will be
> > > blocked (yield to other tasks), then we need a same number of threads on
> > > the system which can invoke the function. So, ultimately, it is same
> > > as function_graph tracer, we need it for each thread.
> > 
> > So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe 
> > nesting allowed).
> 
> No, that is possible to put several kretprobes on same thread, e.g.
> the func1() is called from func2(), user can put kretprobes for each
> function at same time.
> So the possible solution is to allocate new return-stack for each task_struct,
> and that is what the function-graph tracer did.
> 
> Anyway, I'm considering to integrate kretprobe_instance with the ret_stack.
> It will increase memory usage for kretprobes, but can provide safer way
> to allocate kretprobe_instance.

Ok, that sounds good to me.

Thanks,

	Ingo

Re: [RFC PATCH tip/master 2/3] kprobes: Allocate kretprobe instance if its free list is empty

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 30 Mar 2017 08:53:32 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > 
> > > > So this is something I missed while the original code was merged, but the concept 
> > > > looks a bit weird: why do we do any "allocation" while a handler is executing?
> > > > 
> > > > That's fundamentally fragile. What's the maximum number of parallel 
> > > > 'kretprobe_instance' required per kretprobe - one per CPU?
> > > 
> > > It depends on the place where we put the probe. If the probed function will be
> > > blocked (yield to other tasks), then we need a same number of threads on
> > > the system which can invoke the function. So, ultimately, it is same
> > > as function_graph tracer, we need it for each thread.
> > 
> > So then put it into task_struct (assuming there's no kretprobe-inside-kretprobe 
> > nesting allowed).
> 
> No, that is possible to put several kretprobes on same thread, e.g.
> the func1() is called from func2(), user can put kretprobes for each
> function at same time.
> So the possible solution is to allocate new return-stack for each task_struct,
> and that is what the function-graph tracer did.
> 
> Anyway, I'm considering to integrate kretprobe_instance with the ret_stack.
> It will increase memory usage for kretprobes, but can provide safer way
> to allocate kretprobe_instance.

Ok, that sounds good to me.

Thanks,

	Ingo