[PATCH] uprobes: Fix limiting un-nested return probes

[PATCH] uprobes: Fix limiting un-nested return probes

[Hemant Kumar Shaw]

Here is a sample program which shows a problem in uretprobes:
#include <stdlib.h>

int some_work(int num)
{
        while (num != 0)
                num--;
        return 0;
};

int main(int argc, char **argv)
{
        if (argc != 2)
                return EXIT_FAILURE;

        int num = atoi(argv[1]);
        while(num != 0) {
                some_work(100);
                num--;
        };
        return EXIT_SUCCESS;
}


$ gcc -o sample sample.c

- Add probe for returning from some_work():
$ sudo perf probe -x ./sample -a ret=some_work%return

Added new event:
     probe_sample:ret     (on 0x530%return)

You can now use it in all perf tools, such as:

       perf record -e probe_sample:ret -aR sleep 1

- Record events :
$ sudo perf record -e probe_sample:ret -aR ./sample 134

- View report :
$ sudo perf report --stdio

# captured on: Wed Aug 14 17:03:42 2013
# hostname : hemant-fedora
# os release : 3.11.0-rc3+
# perf version : 3.9.4-200.fc18.x86_64
# arch : x86_64
# nrcpus online : 2
# nrcpus avail : 2
# cpudesc : QEMU Virtual CPU version 1.2.2
# cpuid : GenuineIntel,6,2,3
# total memory : 2051912 kB
# cmdline : /usr/bin/perf record -e probe_sample:ret -aR ./sample 134
# event : name = probe_sample:ret, type = 2, config = 0x38c, config1 =
0x0, config2 = 0x0,
# HEADER_CPU_TOPOLOGY info available, use -I to display
# HEADER_NUMA_TOPOLOGY info available, use -I to display
# pmu mappings: software = 1, tracepoint = 2, breakpoint = 5
# ========
#
# Samples: 64  of event 'probe_sample:ret'
# Event count (approx.): 64
#
# Overhead  Command  Shared Object    Symbol
# ........  .......  .............  ........
#
      100.00%   sample  sample         [.] main


#
# (For a higher level overview, try: perf report --sort comm,dso)
#


From report we can see that there were only 64 return events, but
actually it should be 134. It looks like uprobes identified return
events as recursive events and used restrictions for number of such events.
So, here is a patch which fixes this issue.

--->8---

There exists a limit to the number of nested return probes. The current limit is 64.
However this limit is getting enforced on even non nested return probes.
Hence, registering 64 independent non nested return probes results in failure of
return probes on the same task. The problem is utask->depth is getting incremented
unconditionally but decremented only if chained. So, utask->depth should be 
incremented only if chained. This should fix the issue.

Signed-off-by: Hemant Kumar Shaw <hkshaw@linux.vnet.ibm.com>
Reported-by: Mikhail Kulemin <Mikhail.Kulemin@ru.ibm.com>
---
 kernel/events/uprobes.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index f356974..4fb20fe 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1442,7 +1442,8 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
 	ri->orig_ret_vaddr = orig_ret_vaddr;
 	ri->chained = chained;
 
-	utask->depth++;
+	if (chained)
+		utask->depth++;
 
 	/* add instance to the stack */
 	ri->next = utask->return_instances;

Re: [PATCH] uprobes: Fix limiting un-nested return probes

[Oleg Nesterov]

Sorry for delay, vacation.

On 09/03, Hemant Kumar Shaw wrote:
>
> There exists a limit to the number of nested return probes. The current limit is 64.
> However this limit is getting enforced on even non nested return probes.
> Hence, registering 64 independent non nested return probes results in failure of
> return probes on the same task. The problem is utask->depth is getting incremented
> unconditionally but decremented only if chained.

Hmm. I'll try to recheck later, but at first glance this logic is indeed
wrong, thanks.

> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1442,7 +1442,8 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
>  	ri->orig_ret_vaddr = orig_ret_vaddr;
>  	ri->chained = chained;
>  
> -	utask->depth++;
> +	if (chained)
> +		utask->depth++;

Not sure, but I can be easily wrong... afaics we need something like below, no?

Anton?

Oleg.

--- x/kernel/events/uprobes.c
+++ x/kernel/events/uprobes.c
@@ -1682,12 +1682,10 @@ static bool handle_trampoline(struct pt_
 		tmp = ri;
 		ri = ri->next;
 		kfree(tmp);
+		utask->depth--;
 
 		if (!chained)
 			break;
-
-		utask->depth--;
-
 		BUG_ON(!ri);
 	}
 

Re: [PATCH] uprobes: Fix limiting un-nested return probes

[Anton Arapov]

On Sun, Sep 08, 2013 at 06:32:32PM +0200, Oleg Nesterov wrote:
> Sorry for delay, vacation.
> 
> On 09/03, Hemant Kumar Shaw wrote:
> >
> > There exists a limit to the number of nested return probes. The current limit is 64.
> > However this limit is getting enforced on even non nested return probes.
> > Hence, registering 64 independent non nested return probes results in failure of
> > return probes on the same task. The problem is utask->depth is getting incremented
> > unconditionally but decremented only if chained.
> 
> Hmm. I'll try to recheck later, but at first glance this logic is indeed
> wrong, thanks.
> 
> > --- a/kernel/events/uprobes.c
> > +++ b/kernel/events/uprobes.c
> > @@ -1442,7 +1442,8 @@ static void prepare_uretprobe(struct uprobe *uprobe, struct pt_regs *regs)
> >  	ri->orig_ret_vaddr = orig_ret_vaddr;
> >  	ri->chained = chained;
> >  
> > -	utask->depth++;
> > +	if (chained)
> > +		utask->depth++;
> 
> Not sure, but I can be easily wrong... afaics we need something like below, no?
> Anton?

Oleg, your guess is correct. 

My original intention was to limit by depth the chained only probes. But later,
after your review, we've decided /based on safety concerns/ to limit it hard.

The decrement 'utask->depth--;' in my own tree is above the 'if (!chained)' 
check. I think it got mangled somehow when I rebased the code before I sent it
to lkml.

Anton.


> Oleg.
> 
> --- x/kernel/events/uprobes.c
> +++ x/kernel/events/uprobes.c
> @@ -1682,12 +1682,10 @@ static bool handle_trampoline(struct pt_
>  		tmp = ri;
>  		ri = ri->next;
>  		kfree(tmp);
> +		utask->depth--;
>  
>  		if (!chained)
>  			break;
> -
> -		utask->depth--;
> -
>  		BUG_ON(!ri);
>  	}

Re: [PATCH] uprobes: Fix limiting un-nested return probes

[Oleg Nesterov]

On 09/09, Anton Arapov wrote:
>
> On Sun, Sep 08, 2013 at 06:32:32PM +0200, Oleg Nesterov wrote:
> >
> > Not sure, but I can be easily wrong... afaics we need something like below, no?
> > Anton?
>
> Oleg, your guess is correct.
>
> My original intention was to limit by depth the chained only probes. But later,
> after your review, we've decided /based on safety concerns/ to limit it hard.

Chained or not, we allocate return_instance every time, so we certainly
need to account to limit the depth unconditionally. Unless we reuse the
same return_instance if chained, but this is another story.

> The decrement 'utask->depth--;' in my own tree is above the 'if (!chained)'
> check. I think it got mangled somehow when I rebased the code before I sent it
> to lkml.

OK, thanks, I'll write the changelog and re-send the patch below.

> Anton.
>
>
> > Oleg.
> >
> > --- x/kernel/events/uprobes.c
> > +++ x/kernel/events/uprobes.c
> > @@ -1682,12 +1682,10 @@ static bool handle_trampoline(struct pt_
> >  		tmp = ri;
> >  		ri = ri->next;
> >  		kfree(tmp);
> > +		utask->depth--;
> >
> >  		if (!chained)
> >  			break;
> > -
> > -		utask->depth--;
> > -
> >  		BUG_ON(!ri);
> >  	}

Re: [PATCH] uprobes: Fix limiting un-nested return probes

[Hemant]

Hi Oleg,

On 09/09/2013 08:25 PM, Oleg Nesterov wrote:
> On 09/09, Anton Arapov wrote:
>> On Sun, Sep 08, 2013 at 06:32:32PM +0200, Oleg Nesterov wrote:
>>> Not sure, but I can be easily wrong... afaics we need something like below, no?
>>> Anton?
>> Oleg, your guess is correct.
>>
>> My original intention was to limit by depth the chained only probes. But later,
>> after your review, we've decided /based on safety concerns/ to limit it hard.
> Chained or not, we allocate return_instance every time, so we certainly
> need to account to limit the depth unconditionally. Unless we reuse the
> same return_instance if chained, but this is another story.

Hmm, agreed. Thanks for the description.

>
>> The decrement 'utask->depth--;' in my own tree is above the 'if (!chained)'
>> check. I think it got mangled somehow when I rebased the code before I sent it
>> to lkml.
> OK, thanks, I'll write the changelog and re-send the patch below.
>
>> Anton.
>>
>>
>>> Oleg.
>>>
>>> --- x/kernel/events/uprobes.c
>>> +++ x/kernel/events/uprobes.c
>>> @@ -1682,12 +1682,10 @@ static bool handle_trampoline(struct pt_
>>>   		tmp = ri;
>>>   		ri = ri->next;
>>>   		kfree(tmp);
>>> +		utask->depth--;
>>>
>>>   		if (!chained)
>>>   			break;
>>> -
>>> -		utask->depth--;
>>> -
>>>   		BUG_ON(!ri);
>>>   	}


-- 
Thanks
Hemant