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

From: Anton Arapov
Date: Mon Mar 25 2013 - 11:50:36 EST


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