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...
> +
> + 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.
> + 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.
> + 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?
> @@ -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.
> + if (bp_vaddr == get_trampoline_vaddr(area)) {
I just noticed get_trampoline_vaddr() takes an argument... It should
not, I think.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
