Re: [PATCH 5/7] uretprobes: return probe exit, invoke handlers

[Anton Arapov <anton@redhat.com>]

On Sun, Mar 24, 2013 at 05:28:17PM +0100, Oleg Nesterov wrote:
> On 03/22, Anton Arapov wrote:
> >
> > +static void handle_uretprobe(struct xol_area *area, struct pt_regs *regs)
> > +{
> > +	struct uprobe_task *utask;
> > +	struct return_instance *ri, *tmp;
> > +	unsigned long prev_ret_vaddr;
> > +
> > +	utask = get_utask();
> > +	if (!utask)
> > +		return;
> > +
> > +	ri = utask->return_instances;
> > +	if (!ri)
> > +		return;
> Hmm. I am wondering what should the caller (handle_swbp) do in this
> case...
Not sure as well... Will look into it.

> > +
> > +	instruction_pointer_set(regs, ri->orig_ret_vaddr);
> > +
> > +	while (ri) {
> > +		if (ri->uprobe->consumers)
> > +			handler_uretprobe_chain(ri->uprobe, regs);
> I'd suggest to either remove this check or move it into
> handler_uretprobe_chain().
> 
> > +
> > +		put_uprobe(ri->uprobe);
> > +		tmp = ri;
> > +		prev_ret_vaddr = tmp->orig_ret_vaddr;
> For what? It seems that prev_ret_vaddr should be simply killed.
Both above are leftovers I've overlooked before git-send... :(

> > +		ri = ri->next;
> > +		kfree(tmp);
> Another case when you do put_uprobe/kfree using the different vars...
> Once again, the code is correct but imho a bit confusing.
I agree will change it and align with the code in uprobe_free_utask()

> > +		if (!ri || ri->dirty == false) {
> > +			/*
> > +			 * This is the first return uprobe (chronologically)
> > +			 * pushed for this particular instance of the probed
> > +			 * function.
> > +			 */
> > +			utask->return_instances = ri;
> > +			return;
> > +		}
> 
> Else? we simply return without updating ->return_instances which
> points to the freed element(s) ? OK, this must not be possible but
> this is not obvious...
> 
> And the fact you check "ri != NULL" twice doesn't look very nice.
> We already checked ri != NULL before while(ri), we have to do this
> anyway for instruction_pointer_set(). Perhaps do/whild or even
> for (;;) + break would be more clean. But this is minor.
> 
> 
> I am not sure the logic is correct. OK. suppose that
> ->return_instances = NULL.
> 
> The task hits the rp breakoint. After that
> 
> 	return_instances -> { .dirty = false }
> 
> The task hits the same breakoint before return (tail call), now
> we have
> 
> 	return_instances -> { .dirty = true } -> { .dirty = false }
> 
> Then it returns and handle_uretprobe() should unwind the whole stack.
> But, it seems, the main loop will stop after the 1st iteration?
> 
> Ignoring the fact you need put_uprobe/kfree, it seems that we should
> do something like this,
> 
> 	do {
> 		handler_uretprobe_chain(...);
> 
> 		if (!ri->dirty)	// not chained
> 			break;
> 
> 		ri = ri->next;		
> 	} while (ri);
> 
> 	utask->return_instances = ri;
> No?

Oleg, Do you mean 

 	do {
 		handler_uretprobe_chain(...);

        ri = ri->next;		

 		if (!ri->dirty)	// not chained
 			break;
  	} while (ri);
 
 	utask->return_instances = ri;

otherwise we stuck with the first instance in stack.
...and perhaps for(;;) would be 'more beautiful' here?


> 
> > @@ -1631,11 +1681,19 @@ static void handle_swbp(struct pt_regs *regs)
> >  {
> >  	struct uprobe *uprobe;
> >  	unsigned long bp_vaddr;
> > +	struct xol_area *area;
> >  	int uninitialized_var(is_swbp);
> >  
> >  	bp_vaddr = uprobe_get_swbp_addr(regs);
> > -	uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> > +	area = get_xol_area();
> 
> Why?
> No, we do not want this heavy and potentially unnecessary get_xol_area(),
>
> > +	if (area) {
> 
> Just check uprobes_state.xol_area != NULL instead. If it is NULL
> we simply should not call handle_uretprobe().
> 
> Or perhaps get_trampoline_vaddr() should simply return -1 if
> ->xol_area == NULL.

right.

> 
> > +		if (bp_vaddr == get_trampoline_vaddr(area)) {
> 
> I just noticed get_trampoline_vaddr() takes an argument... It should
> not, I think.
> 

  Yes, at this place we must have *area allocated. And I agree with
your arguments, I will remove *area argument from
get_trampoline_vaddr() and handle_uretprobe() it makes sense to me as
well.

Anton

> Oleg.
>