Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers
On 04/13, Srikar Dronamraju wrote: > > > > Oh yes, this should be documented more explicitly in the changelog of > > > this patch or 7/9 (which tries to document the limitations but should > > > be more clear). > > > > > > Currently we do not support longjmp() and we assume that the probed > > > function should do the regular return. We should certainly try to improve > > > this, but I really think that this should go into the next series. > > > > > > Because this is nontrivial, needs more discussion, and I'm afraid should > > > be per-arch. Even on x86 (which can check the stack) this is not simple, > > > in general we can't know how to check that (to simplify) the first frame > > > is already invalid. Just for example, we could check regs->sp and detect > > > that longjmp() was called but sigaltstack() can easily fool this logic. > > > > > Yes, its perfectly fine to keep this logic for the next patchset. OK, great. > Can you tell me why sigaltstack() can fool us if we rely on regs->sp. Because we can't simply compare resg->sp and ret_instance->sp and decide if we should ignore this ri or not, the task can hit retprobe, then take a signal, switch to altstack and hit another rp. I'll write another email (hopefully patches) later. > Acked-by: Srikar Dronamraju Thanks Srikar. OK. Everything is acked, I'll send git-pull-request. 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/
Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers
* Oleg Nesterov [2013-04-09 22:13:02]:
> On 04/09, Oleg Nesterov wrote:
> >
> > > Should we a check here before using top most ri.
> > > What if the application had done a longjmp and the trampoline he hit
> > > corresponds to something thats below in the stack?
> > >
> > > Not sure if this what you meant by leaking return instances in your next
> > > patch.
> >
> > Oh yes, this should be documented more explicitly in the changelog of
> > this patch or 7/9 (which tries to document the limitations but should
> > be more clear).
> >
> > Currently we do not support longjmp() and we assume that the probed
> > function should do the regular return. We should certainly try to improve
> > this, but I really think that this should go into the next series.
> >
> > Because this is nontrivial, needs more discussion, and I'm afraid should
> > be per-arch. Even on x86 (which can check the stack) this is not simple,
> > in general we can't know how to check that (to simplify) the first frame
> > is already invalid. Just for example, we could check regs->sp and detect
> > that longjmp() was called but sigaltstack() can easily fool this logic.
> >
Yes, its perfectly fine to keep this logic for the next patchset.
Can you tell me why sigaltstack() can fool us if we rely on regs->sp.
I should admit that I am not too familiar with sigaltstack.
> > Or we can change prepare_uretprobe() to alloc the new slot for the
> > trampoline every time (and mark it as "trampoline" for handle_swbp() of
> > course), this way we can easily discard the invalid ret_instance's in
> > handler_uretprobe(). But this doesn't solve all problems and this is
> > not really nice/simple.
> >
> > In short. I think we should document the limitations more clearly, push
> > this functionality, then try to improve things. Do you agree?
>
> IOW. Will you agree with v2 below?
>
> Changes:
>
> - s/handler_uretprobe/handle_trampoline/
>
> - s/handler_uretprobe_chain/handle_uretprobe_chain/
>
> - add the TODO: comments into the changelog and
> handle_trampoline().
>
>
> ---
> [PATCH v2] uretprobes: Return probe exit, invoke handlers
>
> Uretprobe handlers are invoked when the trampoline is hit, on completion
> the trampoline is replaced with the saved return address and the uretprobe
> instance deleted.
>
> TODO: handle_trampoline() assumes that ->return_instances is always valid.
> We should teach it to handle longjmp() which can invalidate the pending
> return_instance's. This is nontrivial, we will try to do this in a separate
> series.
>
> Signed-off-by: Anton Arapov
> Signed-off-by: Oleg Nesterov
Acked-by: Srikar Dronamraju
> ---
> kernel/events/uprobes.c | 65
> ++-
> 1 files changed, 64 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index e352e18..b9c4325 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1633,6 +1633,62 @@ static void handler_chain(struct uprobe *uprobe,
> struct pt_regs *regs)
> up_read(&uprobe->register_rwsem);
> }
>
> +static void
> +handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> +{
> + struct uprobe *uprobe = ri->uprobe;
> + struct uprobe_consumer *uc;
> +
> + down_read(&uprobe->register_rwsem);
> + for (uc = uprobe->consumers; uc; uc = uc->next) {
> + if (uc->ret_handler)
> + uc->ret_handler(uc, ri->func, regs);
> + }
> + up_read(&uprobe->register_rwsem);
> +}
> +
> +static bool handle_trampoline(struct pt_regs *regs)
> +{
> + struct uprobe_task *utask;
> + struct return_instance *ri, *tmp;
> + bool chained;
> +
> + utask = current->utask;
> + if (!utask)
> + return false;
> +
> + ri = utask->return_instances;
> + if (!ri)
> + return false;
> +
> + /*
> + * TODO: we should throw out return_instance's invalidated by
> + * longjmp(), currently we assume that the probed function always
> + * returns.
> + */
> + instruction_pointer_set(regs, ri->orig_ret_vaddr);
> +
> + for (;;) {
> + handle_uretprobe_chain(ri, regs);
> +
> + chained = ri->chained;
> + put_uprobe(ri->uprobe);
> +
> + tmp = ri;
> + ri = ri->next;
> + kfree(tmp);
> +
> + if (!chained)
> + break;
> +
> + BUG_ON(!ri);
> + }
> +
> + utask->return_instances = ri;
> +
> + return true;
> +}
> +
> /*
> * Run handler and ask thread to singlestep.
> * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> @@ -1644,8 +1700,15 @@ static void handle_swbp(struct pt_regs *regs)
> int uninitialized_var(is_swbp);
>
> bp_vaddr = uprobe_get_swbp_addr(regs);
> - uprobe = find_active_uprobe(bp_vaddr, &is_swb
Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers
On 04/09, Oleg Nesterov wrote:
>
> > Should we a check here before using top most ri.
> > What if the application had done a longjmp and the trampoline he hit
> > corresponds to something thats below in the stack?
> >
> > Not sure if this what you meant by leaking return instances in your next
> > patch.
>
> Oh yes, this should be documented more explicitly in the changelog of
> this patch or 7/9 (which tries to document the limitations but should
> be more clear).
>
> Currently we do not support longjmp() and we assume that the probed
> function should do the regular return. We should certainly try to improve
> this, but I really think that this should go into the next series.
>
> Because this is nontrivial, needs more discussion, and I'm afraid should
> be per-arch. Even on x86 (which can check the stack) this is not simple,
> in general we can't know how to check that (to simplify) the first frame
> is already invalid. Just for example, we could check regs->sp and detect
> that longjmp() was called but sigaltstack() can easily fool this logic.
>
> Or we can change prepare_uretprobe() to alloc the new slot for the
> trampoline every time (and mark it as "trampoline" for handle_swbp() of
> course), this way we can easily discard the invalid ret_instance's in
> handler_uretprobe(). But this doesn't solve all problems and this is
> not really nice/simple.
>
> In short. I think we should document the limitations more clearly, push
> this functionality, then try to improve things. Do you agree?
IOW. Will you agree with v2 below?
Changes:
- s/handler_uretprobe/handle_trampoline/
- s/handler_uretprobe_chain/handle_uretprobe_chain/
- add the TODO: comments into the changelog and
handle_trampoline().
---
[PATCH v2] uretprobes: Return probe exit, invoke handlers
Uretprobe handlers are invoked when the trampoline is hit, on completion
the trampoline is replaced with the saved return address and the uretprobe
instance deleted.
TODO: handle_trampoline() assumes that ->return_instances is always valid.
We should teach it to handle longjmp() which can invalidate the pending
return_instance's. This is nontrivial, we will try to do this in a separate
series.
Signed-off-by: Anton Arapov
Signed-off-by: Oleg Nesterov
---
kernel/events/uprobes.c | 65 ++-
1 files changed, 64 insertions(+), 1 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index e352e18..b9c4325 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1633,6 +1633,62 @@ static void handler_chain(struct uprobe *uprobe, struct
pt_regs *regs)
up_read(&uprobe->register_rwsem);
}
+static void
+handle_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
+{
+ struct uprobe *uprobe = ri->uprobe;
+ struct uprobe_consumer *uc;
+
+ down_read(&uprobe->register_rwsem);
+ for (uc = uprobe->consumers; uc; uc = uc->next) {
+ if (uc->ret_handler)
+ uc->ret_handler(uc, ri->func, regs);
+ }
+ up_read(&uprobe->register_rwsem);
+}
+
+static bool handle_trampoline(struct pt_regs *regs)
+{
+ struct uprobe_task *utask;
+ struct return_instance *ri, *tmp;
+ bool chained;
+
+ utask = current->utask;
+ if (!utask)
+ return false;
+
+ ri = utask->return_instances;
+ if (!ri)
+ return false;
+
+ /*
+* TODO: we should throw out return_instance's invalidated by
+* longjmp(), currently we assume that the probed function always
+* returns.
+*/
+ instruction_pointer_set(regs, ri->orig_ret_vaddr);
+
+ for (;;) {
+ handle_uretprobe_chain(ri, regs);
+
+ chained = ri->chained;
+ put_uprobe(ri->uprobe);
+
+ tmp = ri;
+ ri = ri->next;
+ kfree(tmp);
+
+ if (!chained)
+ break;
+
+ BUG_ON(!ri);
+ }
+
+ utask->return_instances = ri;
+
+ return true;
+}
+
/*
* Run handler and ask thread to singlestep.
* Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1644,8 +1700,15 @@ static void handle_swbp(struct pt_regs *regs)
int uninitialized_var(is_swbp);
bp_vaddr = uprobe_get_swbp_addr(regs);
- uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
+ if (bp_vaddr == get_trampoline_vaddr()) {
+ if (handle_trampoline(regs))
+ return;
+ pr_warn("uprobe: unable to handle uretprobe pid/tgid=%d/%d\n",
+ current->pid, current->tgid);
+ }
+
+ uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
if (!uprobe) {
if (is_swbp > 0) {
/* No matchi
Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers
On 04/07, Srikar Dronamraju wrote:
>
> > +static void
> > +handler_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
>
> > +{
> > + struct uprobe *uprobe = ri->uprobe;
> > + struct uprobe_consumer *uc;
> > +
> > + down_read(&uprobe->register_rwsem);
> > + for (uc = uprobe->consumers; uc; uc = uc->next) {
> > + if (uc->ret_handler)
> > + uc->ret_handler(uc, ri->func, regs);
> > + }
> > + up_read(&uprobe->register_rwsem);
> > +}
> > +
> > +static bool handler_uretprobe(struct pt_regs *regs)
>
> Nit: can this be renamed to handle_trampoline
Agreed.
And probably handler_uretprobe_chain() should be ret_handler_chain()
to match handler_chain().
> > +{
> > + struct uprobe_task *utask;
> > + struct return_instance *ri, *tmp;
> > + bool chained;
> > +
> > + utask = current->utask;
> > + if (!utask)
> > + return false;
> > +
> > + ri = utask->return_instances;
> > + if (!ri)
> > + return false;
> > +
> > + instruction_pointer_set(regs, ri->orig_ret_vaddr);
>
> Should we a check here before using top most ri.
> What if the application had done a longjmp and the trampoline he hit
> corresponds to something thats below in the stack?
>
> Not sure if this what you meant by leaking return instances in your next
> patch.
Oh yes, this should be documented more explicitly in the changelog of
this patch or 7/9 (which tries to document the limitations but should
be more clear).
Currently we do not support longjmp() and we assume that the probed
function should do the regular return. We should certainly try to improve
this, but I really think that this should go into the next series.
Because this is nontrivial, needs more discussion, and I'm afraid should
be per-arch. Even on x86 (which can check the stack) this is not simple,
in general we can't know how to check that (to simplify) the first frame
is already invalid. Just for example, we could check regs->sp and detect
that longjmp() was called but sigaltstack() can easily fool this logic.
Or we can change prepare_uretprobe() to alloc the new slot for the
trampoline every time (and mark it as "trampoline" for handle_swbp() of
course), this way we can easily discard the invalid ret_instance's in
handler_uretprobe(). But this doesn't solve all problems and this is
not really nice/simple.
In short. I think we should document the limitations more clearly, push
this functionality, then try to improve things. Do you agree?
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/
Re: [PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers
* Anton Arapov [2013-04-03 18:00:36]:
> Uretprobe handlers are invoked when the trampoline is hit, on completion the
> trampoline is replaced with the saved return address and the uretprobe
> instance
> deleted.
>
> v1 changes:
> * pass bp_vaddr to ret_handler()
> * simplify handle_uretprobe()
>
> RFCv6 changes:
> * rework handle_uretprobe()
>
> RFCv5 changes:
> * switch to simply linked list ->return_uprobes
> * rework handle_uretprobe()
>
> RFCv4 changes:
> * check, whether utask is not NULL in handle_uretprobe()
> * get rid of area->rp_trampoline_vaddr
> * minor handle_uretprobe() fixups
>
> RFCv3 changes:
> * protected uprobe with refcounter. See put_uprobe() in handle_uretprobe()
> that reflects increment in prepare_uretprobe()
>
> RFCv2 changes:
> * get rid of ->return_consumers member from struct uprobe, introduce
> ret_handler() in consumer instead
>
> Signed-off-by: Anton Arapov
> ---
> kernel/events/uprobes.c | 60
> -
> 1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 08ecfff..d129c1d 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1609,6 +1609,57 @@ static void handler_chain(struct uprobe *uprobe,
> struct pt_regs *regs)
> up_read(&uprobe->register_rwsem);
> }
>
> +static void
> +handler_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
> +{
> + struct uprobe *uprobe = ri->uprobe;
> + struct uprobe_consumer *uc;
> +
> + down_read(&uprobe->register_rwsem);
> + for (uc = uprobe->consumers; uc; uc = uc->next) {
> + if (uc->ret_handler)
> + uc->ret_handler(uc, ri->func, regs);
> + }
> + up_read(&uprobe->register_rwsem);
> +}
> +
> +static bool handler_uretprobe(struct pt_regs *regs)
Nit: can this be renamed to handle_trampoline
> +{
> + struct uprobe_task *utask;
> + struct return_instance *ri, *tmp;
> + bool chained;
> +
> + utask = current->utask;
> + if (!utask)
> + return false;
> +
> + ri = utask->return_instances;
> + if (!ri)
> + return false;
> +
> + instruction_pointer_set(regs, ri->orig_ret_vaddr);
Should we a check here before using top most ri.
What if the application had done a longjmp and the trampoline he hit
corresponds to something thats below in the stack?
Not sure if this what you meant by leaking return instances in your next
patch.
> +
> + for (;;) {
> + handler_uretprobe_chain(ri, regs);
> +
> + chained = ri->chained;
> + put_uprobe(ri->uprobe);
> +
> + tmp = ri;
> + ri = ri->next;
> + kfree(tmp);
> +
> + if (!chained)
> + break;
> +
> + BUG_ON(!ri);
> + }
> +
> + utask->return_instances = ri;
> +
> + return true;
> +}
> +
> /*
> * Run handler and ask thread to singlestep.
> * Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
> @@ -1620,8 +1671,15 @@ static void handle_swbp(struct pt_regs *regs)
> int uninitialized_var(is_swbp);
>
> bp_vaddr = uprobe_get_swbp_addr(regs);
> - uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> + if (bp_vaddr == get_trampoline_vaddr()) {
> + if (handler_uretprobe(regs))
> + return;
>
> + pr_warn("uprobe: unable to handle uretprobe pid/tgid=%d/%d\n",
> + current->pid, current->tgid);
> + }
> +
> + uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
> if (!uprobe) {
> if (is_swbp > 0) {
> /* No matching uprobe; signal SIGTRAP. */
> --
> 1.8.1.4
>
--
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/
[PATCH v1 6/9] uretprobes: Return probe exit, invoke handlers
Uretprobe handlers are invoked when the trampoline is hit, on completion the
trampoline is replaced with the saved return address and the uretprobe instance
deleted.
v1 changes:
* pass bp_vaddr to ret_handler()
* simplify handle_uretprobe()
RFCv6 changes:
* rework handle_uretprobe()
RFCv5 changes:
* switch to simply linked list ->return_uprobes
* rework handle_uretprobe()
RFCv4 changes:
* check, whether utask is not NULL in handle_uretprobe()
* get rid of area->rp_trampoline_vaddr
* minor handle_uretprobe() fixups
RFCv3 changes:
* protected uprobe with refcounter. See put_uprobe() in handle_uretprobe()
that reflects increment in prepare_uretprobe()
RFCv2 changes:
* get rid of ->return_consumers member from struct uprobe, introduce
ret_handler() in consumer instead
Signed-off-by: Anton Arapov
---
kernel/events/uprobes.c | 60 -
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 08ecfff..d129c1d 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1609,6 +1609,57 @@ static void handler_chain(struct uprobe *uprobe, struct
pt_regs *regs)
up_read(&uprobe->register_rwsem);
}
+static void
+handler_uretprobe_chain(struct return_instance *ri, struct pt_regs *regs)
+{
+ struct uprobe *uprobe = ri->uprobe;
+ struct uprobe_consumer *uc;
+
+ down_read(&uprobe->register_rwsem);
+ for (uc = uprobe->consumers; uc; uc = uc->next) {
+ if (uc->ret_handler)
+ uc->ret_handler(uc, ri->func, regs);
+ }
+ up_read(&uprobe->register_rwsem);
+}
+
+static bool handler_uretprobe(struct pt_regs *regs)
+{
+ struct uprobe_task *utask;
+ struct return_instance *ri, *tmp;
+ bool chained;
+
+ utask = current->utask;
+ if (!utask)
+ return false;
+
+ ri = utask->return_instances;
+ if (!ri)
+ return false;
+
+ instruction_pointer_set(regs, ri->orig_ret_vaddr);
+
+ for (;;) {
+ handler_uretprobe_chain(ri, regs);
+
+ chained = ri->chained;
+ put_uprobe(ri->uprobe);
+
+ tmp = ri;
+ ri = ri->next;
+ kfree(tmp);
+
+ if (!chained)
+ break;
+
+ BUG_ON(!ri);
+ }
+
+ utask->return_instances = ri;
+
+ return true;
+}
+
/*
* Run handler and ask thread to singlestep.
* Ensure all non-fatal signals cannot interrupt thread while it singlesteps.
@@ -1620,8 +1671,15 @@ static void handle_swbp(struct pt_regs *regs)
int uninitialized_var(is_swbp);
bp_vaddr = uprobe_get_swbp_addr(regs);
- uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
+ if (bp_vaddr == get_trampoline_vaddr()) {
+ if (handler_uretprobe(regs))
+ return;
+ pr_warn("uprobe: unable to handle uretprobe pid/tgid=%d/%d\n",
+ current->pid, current->tgid);
+ }
+
+ uprobe = find_active_uprobe(bp_vaddr, &is_swbp);
if (!uprobe) {
if (is_swbp > 0) {
/* No matching uprobe; signal SIGTRAP. */
--
1.8.1.4
--
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/
